fuzzing: add raft fuzzer from cncf-fuzzing #55
Conversation
| return zap.New(core).WithOptions(options...) | ||
| } | ||
|
|
||
| func FuzzStep(f *testing.F) { |
There was a problem hiding this comment.
Tried running this in IDE. Test failed and it took ~3mins. Is this expected? Runtime seems too long.
/Users/bk/.gvm/gos/go1.19.2/bin/go test -json go.etcd.io/raft/v3 -fuzz ^\QFuzzStep\E$ -run ^$
=== FUZZ FuzzStep
warning: starting with empty corpus
fuzz: elapsed: 0s, execs: 0 (0/sec), new interesting: 0 (total: 0)
fuzz: elapsed: 3s, execs: 49356 (16446/sec), new interesting: 27 (total: 27)
fuzz: elapsed: 6s, execs: 153467 (34706/sec), new interesting: 32 (total: 32)
fuzz: elapsed: 9s, execs: 194647 (13726/sec), new interesting: 34 (total: 34)
fuzz: elapsed: 12s, execs: 214509 (6623/sec), new interesting: 35 (total: 35)
fuzz: elapsed: 15s, execs: 339183 (41546/sec), new interesting: 37 (total: 37)
fuzz: elapsed: 18s, execs: 404951 (21930/sec), new interesting: 37 (total: 37)
fuzz: elapsed: 21s, execs: 560220 (51739/sec), new interesting: 38 (total: 38)
fuzz: elapsed: 24s, execs: 653020 (30933/sec), new interesting: 42 (total: 42)
fuzz: elapsed: 27s, execs: 706390 (17791/sec), new interesting: 42 (total: 42)
fuzz: elapsed: 30s, execs: 772088 (21898/sec), new interesting: 42 (total: 42)
fuzz: elapsed: 33s, execs: 772088 (0/sec), new interesting: 42 (total: 42)
fuzz: elapsed: 36s, execs: 772088 (0/sec), new interesting: 42 (total: 42)
fuzz: elapsed: 39s, execs: 869852 (32588/sec), new interesting: 43 (total: 43)
fuzz: elapsed: 42s, execs: 869852 (0/sec), new interesting: 43 (total: 43)
fuzz: elapsed: 45s, execs: 869852 (0/sec), new interesting: 43 (total: 43)
fuzz: elapsed: 48s, execs: 1036345 (55479/sec), new interesting: 44 (total: 44)
fuzz: elapsed: 51s, execs: 1036345 (0/sec), new interesting: 44 (total: 44)
fuzz: elapsed: 54s, execs: 1036345 (0/sec), new interesting: 44 (total: 44)
fuzz: elapsed: 57s, execs: 1036345 (0/sec), new interesting: 44 (total: 44)
fuzz: elapsed: 1m0s, execs: 1036345 (0/sec), new interesting: 44 (total: 44)
fuzz: elapsed: 1m3s, execs: 1048211 (3955/sec), new interesting: 52 (total: 52)
fuzz: elapsed: 1m6s, execs: 1056240 (2677/sec), new interesting: 61 (total: 61)
fuzz: elapsed: 1m9s, execs: 1069088 (4283/sec), new interesting: 64 (total: 64)
fuzz: elapsed: 1m12s, execs: 1074428 (1780/sec), new interesting: 67 (total: 67)
fuzz: elapsed: 1m15s, execs: 1074428 (0/sec), new interesting: 67 (total: 67)
fuzz: elapsed: 1m18s, execs: 1103712 (9764/sec), new interesting: 72 (total: 72)
fuzz: elapsed: 1m21s, execs: 1111263 (2517/sec), new interesting: 72 (total: 72)
fuzz: elapsed: 1m24s, execs: 1122321 (3686/sec), new interesting: 74 (total: 74)
fuzz: elapsed: 1m27s, execs: 1223408 (33686/sec), new interesting: 77 (total: 77)
fuzz: elapsed: 1m30s, execs: 1240341 (5646/sec), new interesting: 78 (total: 78)
fuzz: elapsed: 1m33s, execs: 1250383 (3347/sec), new interesting: 78 (total: 78)
fuzz: elapsed: 1m36s, execs: 1257633 (2417/sec), new interesting: 78 (total: 78)
fuzz: elapsed: 1m39s, execs: 1331387 (24577/sec), new interesting: 80 (total: 80)
fuzz: elapsed: 1m42s, execs: 1469927 (46189/sec), new interesting: 80 (total: 80)
fuzz: elapsed: 1m45s, execs: 1503247 (11107/sec), new interesting: 81 (total: 81)
fuzz: elapsed: 1m48s, execs: 1545766 (14170/sec), new interesting: 81 (total: 81)
fuzz: elapsed: 1m51s, execs: 1550464 (1566/sec), new interesting: 81 (total: 81)
fuzz: elapsed: 1m54s, execs: 1558434 (2657/sec), new interesting: 83 (total: 83)
fuzz: elapsed: 1m57s, execs: 1558434 (0/sec), new interesting: 83 (total: 83)
fuzz: elapsed: 2m0s, execs: 1558434 (0/sec), new interesting: 83 (total: 83)
fuzz: elapsed: 2m3s, execs: 1558434 (0/sec), new interesting: 83 (total: 83)
fuzz: elapsed: 2m6s, execs: 1566754 (2773/sec), new interesting: 83 (total: 83)
fuzz: elapsed: 2m9s, execs: 1583562 (5603/sec), new interesting: 85 (total: 85)
fuzz: elapsed: 2m12s, execs: 1599171 (5204/sec), new interesting: 86 (total: 86)
fuzz: elapsed: 2m15s, execs: 1615828 (5553/sec), new interesting: 87 (total: 87)
fuzz: elapsed: 2m18s, execs: 1628572 (4248/sec), new interesting: 87 (total: 87)
fuzz: elapsed: 2m21s, execs: 1639432 (3619/sec), new interesting: 87 (total: 87)
fuzz: elapsed: 2m24s, execs: 1647428 (2666/sec), new interesting: 87 (total: 87)
fuzz: elapsed: 2m27s, execs: 1652544 (1706/sec), new interesting: 88 (total: 88)
fuzz: elapsed: 2m30s, execs: 1667946 (5134/sec), new interesting: 90 (total: 90)
fuzz: elapsed: 2m33s, execs: 1683152 (5069/sec), new interesting: 90 (total: 90)
fuzz: elapsed: 2m36s, execs: 1695981 (4276/sec), new interesting: 90 (total: 90)
fuzz: elapsed: 2m39s, execs: 1708469 (4162/sec), new interesting: 90 (total: 90)
fuzz: elapsed: 2m42s, execs: 1726815 (6117/sec), new interesting: 91 (total: 91)
fuzz: elapsed: 2m45s, execs: 1747196 (6792/sec), new interesting: 91 (total: 91)
fuzz: minimizing 71-byte failing input file
fuzz: elapsed: 2m47s, minimizing
--- FAIL: FuzzStep (167.36s)
--- FAIL: FuzzStep (0.00s)
testing.go:1356: panic: need non-empty snapshot
goroutine 43783 [running]:
runtime/debug.Stack()
/Users/bk/.gvm/gos/go1.19.2/src/runtime/debug/stack.go:24 +0xdb
testing.tRunner.func1()
/Users/bk/.gvm/gos/go1.19.2/src/testing/testing.go:1356 +0x1f2
panic({0x1930fe0, 0xc00a217580})
/Users/bk/.gvm/gos/go1.19.2/src/runtime/panic.go:884 +0x212
go.etcd.io/raft/v3.catchPanics()
/Users/bk/github/raft/fuzz_test.go:101 +0x2bd
panic({0x1930fe0, 0x1aaa838})
/Users/bk/.gvm/gos/go1.19.2/src/runtime/panic.go:884 +0x212
go.etcd.io/raft/v3.(*raft).maybeSendAppend(0xc00a2ce300, 0x2, 0x1)
/Users/bk/github/raft/raft.go:608 +0xf51
go.etcd.io/raft/v3.(*raft).sendAppend(...)
/Users/bk/github/raft/raft.go:560
go.etcd.io/raft/v3.(*raft).bcastAppend.func1(0x2, 0xc00a2d4630?)
/Users/bk/github/raft/raft.go:662 +0x10b
go.etcd.io/raft/v3/tracker.(*ProgressTracker).Visit(0xc00a2ce348, 0xc006998850)
/Users/bk/github/raft/tracker/tracker.go:211 +0x2ea
go.etcd.io/raft/v3.(*raft).bcastAppend(0xc00a2ce300)
/Users/bk/github/raft/raft.go:658 +0x79
go.etcd.io/raft/v3.stepLeader(0xc00a2ce300, {0x4, 0x1, 0x1, 0x1, 0x0, 0x1, {0x0, 0x0, 0x0}, ...})
/Users/bk/github/raft/raft.go:1432 +0xfaa
go.etcd.io/raft/v3.(*raft).Step(0xc00a2ce300, {0x4, 0x1, 0x1, 0x1, 0x0, 0x1, {0x0, 0x0, 0x0}, ...})
/Users/bk/github/raft/raft.go:1156 +0x26d5
go.etcd.io/raft/v3.(*raft).stepOrSend(0xc00a2ce300, {0xc00a2d2280, 0x1, 0x0?})
/Users/bk/github/raft/raft_test.go:85 +0x2e5
go.etcd.io/raft/v3.(*raft).advanceMessagesAfterAppend(0xc00a2ce300)
/Users/bk/github/raft/raft_test.go:72 +0x6b
go.etcd.io/raft/v3.(*raft).readMessages(...)
/Users/bk/github/raft/raft_test.go:60
go.etcd.io/raft/v3.FuzzStep.func1(0x0?, {0xc00a0fa168, 0x14, 0x18})
/Users/bk/github/raft/fuzz_test.go:146 +0xab3
reflect.Value.call({0x1937200?, 0x1a0c5e8?, 0x13?}, {0x19ce3e8, 0x4}, {0xc00a2d43f0, 0x2, 0x2?})
/Users/bk/.gvm/gos/go1.19.2/src/reflect/value.go:584 +0x8c5
reflect.Value.Call({0x1937200?, 0x1a0c5e8?, 0x51b?}, {0xc00a2d43f0?, 0x1d04f78?, 0x1dc6700?})
/Users/bk/.gvm/gos/go1.19.2/src/reflect/value.go:368 +0xbc
testing.(*F).Fuzz.func1.1(0x1176e7f?)
/Users/bk/.gvm/gos/go1.19.2/src/testing/fuzz.go:337 +0x231
testing.tRunner(0xc00a2a7d40, 0xc00a2c5560)
/Users/bk/.gvm/gos/go1.19.2/src/testing/testing.go:1446 +0x10b
created by testing.(*F).Fuzz.func1
/Users/bk/.gvm/gos/go1.19.2/src/testing/fuzz.go:324 +0x5b9
Failing input written to testdata/fuzz/FuzzStep/7950126c9b19fb7cd77014af3e74a042cc85896da08e7de8423f8b45893a040a
To re-run:
go test -run=FuzzStep/7950126c9b19fb7cd77014af3e74a042cc85896da08e7de8423f8b45893a040a
FAIL
exit status 1
FAIL go.etcd.io/raft/v3 167.784s
Process finished with the exit code 1
There was a problem hiding this comment.
That stacktrace looks normal. The fuzzer runs into this panic:
Line 608 in 0c22de0
There was a problem hiding this comment.
We need to make sure that this code works at least in CI. Please provide the tuning required for fuzzer to run on workflows.
go.mod
Outdated
| @@ -3,16 +3,21 @@ module go.etcd.io/raft/v3 | |||
| go 1.19 | |||
|
|
|||
| require ( | |||
| github.com/AdaLogics/go-fuzz-headers v0.0.0-20230106234847-43070de90fa1 | |||
There was a problem hiding this comment.
there were concerns about adding this dependency etcd-io/etcd#14674 (comment)
There was a problem hiding this comment.
Correct. At the moment there is no replacement for the GenerateStruct method, so it cannot be excluded.
There was a problem hiding this comment.
How big is the code for GenerateStruct? could we just copy it until there is a replacement?
There was a problem hiding this comment.
How big is the code for
GenerateStruct? could we just copy it until there is a replacement?
Around 1000 lines in total as far as I can tell.
There was a problem hiding this comment.
Should I ask why we need for code generating tarballs? https://github.com/AdaLogics/go-fuzz-headers/blob/43070de90fa134c9ea55cd6de4308a2ae59658d3/consumer.go#L484-L835
Please just copy code that you need to GeneateStruct. This library is just adds needless dependencies
There was a problem hiding this comment.
the tarball APIs are not related to GenerateStruct.
Would you be fine with using github.com/AdaLogics/go-fuzz-headers if it didn't pull in transitive dependencies?
There was a problem hiding this comment.
@AdamKorcz the lesser dependencies we add here, the lesser gets into k8s go.mod. Yes please take a whack at pulling in the absolute minimum and we can revisit this PR then.
I should say so, since the deps that the previous PR bumped are added in this PR and not bumped. The issue with the deps in the last PR was related to bumping existing libraries. |
|
Can you add a Github workflow for running the fuzzer each PR? Example https://github.com/etcd-io/etcd/blob/b1e14c7d0aa596e0337c5165025bddca344d1a0a/.github/workflows/fuzzing.yaml |
AdamKorcz
force-pushed
the
fuzz1
branch
3 times, most recently
from
948bc0b
to
764f6d6
Compare
|
@serathius I have minimized the dependencies. Could you have a look? |
There was a problem hiding this comment.
The dependencies seem good - but now we're using what looks a lot like a fork that might one day accidentally go away, at which point the module would no longer build unless dependencies were cached. This isn't as much a problem for the repo - we can just rip out the dependency at that point since it's always going to be a testing-only one - but it would be a significant annoyance for developers who are working with an older (and then defunct) version of the library.
It's difficult for me to make a call here. If the code were inlined into the raft repo (licenses permitting), i.e. "just" put into fuzz_test.go or a util package, this problem would go away.
I am currently working on a release in my fork that will get merged upstream with 1-2 weeks. Could we use the upstream repo with a |
I don't want to hold up the train, but if it's only for 1-2 weeks, does it make sense to wait? We can pull upstream in directly and merge this PR then. I also understand the desire to just get it in, and make the upstream adjustment separately. That's fine by me too, but I'd want @serathius or one of the other maintainers to support it as well. |
|
I would wait until the change is properly upstreamed, I don't understand why after almost 3 quarters of fuzzer development (context) we decide to cut corners now. cc @jeefy I would even go further and want to avoid any dependency. I don't understand why we need whole library with random dependencies just for one struct that has less than 500 lines (code in consumers.go, half of it is for unrelated tar handling). why we can't just copy the one necessary struct and be done with it. |
Maintenance and duplication are the main downsides I can see from adding the utilities from I think we can circumvent the The fuzzers from cncf-fuzzing use |
Signed-off-by: AdamKorcz <[email protected]>
Signed-off-by: AdamKorcz <[email protected]>
Signed-off-by: AdamKorcz <[email protected]>
Signed-off-by: AdamKorcz <[email protected]>
Signed-off-by: AdamKorcz <[email protected]>
Signed-off-by: AdamKorcz <[email protected]>
Signed-off-by: AdamKorcz <[email protected]>
|
Changed to the upstream |
Signed-off-by: AdamKorcz <[email protected]>
|
+1 @serathius |
Adds the fuzzer from etcd-io/etcd#14674.
It is the same fuzzer but added here since the raft dir no longer exists in etcd core.