raft: fix out-of-bounds in maybeAppend#13603
Conversation
spzala
left a comment
There was a problem hiding this comment.
@AdamKorcz thank you so much for the fix! Running the CI.
raft/log.go
Outdated
| l.logger.Panicf("entry %d conflict with committed entry [committed(%d)]", ci, l.committed) | ||
| default: | ||
| offset := index + 1 | ||
| if ci-offset>uint64(len(ents)) { |
There was a problem hiding this comment.
Suggesting a space around > .. it may also fail static check.
There was a problem hiding this comment.
"gofmt -w filename" can format the source code automatically. But I wonder why the CI did not raise an error on this. We may need to enhance the CI. Just raised an issue to track this.
@AdamKorcz, could you please point me to the fuzz test case which discovers this issue? I am thinking it might not possible to see this issue in an e2e test case.
There was a problem hiding this comment.
It did fail with this error https://github.com/etcd-io/etcd/runs/4828699828?check_suite_focus=true#step:5:137
There was a problem hiding this comment.
@AdamKorcz once you fix the space related static error, we should be good..some tests are flaky and not related to changes. Thanks!
Fixes an OOB in
maybeAppend.The fix is to throw a
l.logger.Panicfin similar fashion as here and here.OSS-fuzz issue: https://oss-fuzz.com/testcase-detail/6604212964818944