Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

tests/fuzzers: add nodestatemachine fuzzer (wip) #21970

Closed
wants to merge 6 commits into from

Conversation

zsfelfoldi
Copy link
Contributor

This PR is based on #21935 and implements a NodeStateMachine fuzzer.

@zsfelfoldi
Copy link
Contributor Author

Note: when using strings as test field types there are crashes that appear to be inside the fuzzer:

panic: runtime error: invalid memory address or nil pointer dereference
[signal SIGSEGV: segmentation violation code=0x1 addr=0x0 pc=0x48998e]

goroutine 1 [running]:
go-fuzz-dep.serialize(0x0, 0x0, 0x7a93a0, 0xc000010940, 0xc000178429, 0x27, 0x27, 0xc000012001)
	go-fuzz-dep/sonar.go:150 +0x26e
go-fuzz-dep.Sonar(0x7a93a0, 0xc000010940, 0x0, 0x0, 0x28dd00)
	go-fuzz-dep/sonar.go:35 +0x10e
github.com/ethereum/go-ethereum/p2p/nodestate.(*NodeStateMachine).setField.func2(...)
	/home/fefe/go/src/github.com/ethereum/go-ethereum/p2p/nodestate/nodestate.go:900
github.com/ethereum/go-ethereum/p2p/nodestate.(*NodeStateMachine).setField(0xc000190380, 0xc0000d8930, 0x7, 0xc00013d640, 0x7a93a0, 0xc000010940, 0x7fd2b726a008, 0x0)
	/home/fefe/go/src/github.com/ethereum/go-ethereum/p2p/nodestate/nodestate.go:900 +0x44f
github.com/ethereum/go-ethereum/p2p/nodestate.(*NodeStateMachine).SetField(0xc000190380, 0xc0000d8930, 0x7, 0xc00013d640, 0x7a93a0, 0xc000010940, 0x0, 0x0)
	/home/fefe/go/src/github.com/ethereum/go-ethereum/p2p/nodestate/nodestate.go:868 +0x142
github.com/ethereum/go-ethereum/tests/fuzzers/nodestatemachine.(*fuzzer).fuzz.func4(0xc0000d88c0, 0xc0000d8800)
	/home/fefe/go/src/github.com/ethereum/go-ethereum/tests/fuzzers/nodestatemachine/nodestatemachine-fuzzer.go:410 +0xa7b
github.com/ethereum/go-ethereum/tests/fuzzers/nodestatemachine.(*fuzzer).fuzz.func8(0xc000154340, 0x10, 0x20, 0xc000016e00)
	/home/fefe/go/src/github.com/ethereum/go-ethereum/tests/fuzzers/nodestatemachine/nodestatemachine-fuzzer.go:453 +0x101
github.com/ethereum/go-ethereum/tests/fuzzers/nodestatemachine.(*fuzzer).fuzz(0xc00017de38, 0xc0000852c0)
	/home/fefe/go/src/github.com/ethereum/go-ethereum/tests/fuzzers/nodestatemachine/nodestatemachine-fuzzer.go:468 +0x1340
github.com/ethereum/go-ethereum/tests/fuzzers/nodestatemachine.Fuzz(0x7fd2b4c19000, 0x122, 0x122, 0x3)
	/home/fefe/go/src/github.com/ethereum/go-ethereum/tests/fuzzers/nodestatemachine/nodestatemachine-fuzzer.go:32 +0xd3
go-fuzz-dep.Main(0xc00017df48, 0x1, 0x1)
	go-fuzz-dep/main.go:36 +0x1ad
main.main()
	github.com/ethereum/go-ethereum/tests/fuzzers/nodestatemachine/go.fuzz.main/main.go:15 +0x52
exit status 2

In nodestate.go:900 there is a simple interface to interface equality check that randomly triggers panics inside go-fuzz-dep/sonar.go. The crashers are not useful here, the effect is not deterministic (while the tested scenarios are otherwise 100% deterministic). When I replaced string types as test field values to uint16 the error disappeared. (From the perspective of the test the actual types are not important, I just wanted to use two different types in order to provoke some type errors)

@holiman
Copy link
Contributor

holiman commented Dec 8, 2020

Unless the fuzzer is very specialized, like needs very particular inputs in order to get any good coverage, adding the corpus to git doesn't really make sense, IMO, so I'd remove that

Copy link
Contributor

@holiman holiman left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Heh, I mean, the obviously code does something -- but I'd have to spend a day to figure out what it's doing :)

Comment on lines +298 to +303
u := u
optype := u % 5
u /= 5
nn := n
shift := u % 4
u /= 4
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What is this? What's happening?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I tried to use the quasi-random input string economically :) This part takes two bytes of input and transtlates it to a single operation. These are then assigned to byte values. Subscription callbacks, ns.Operation batches and the "top level" activity are composed as sequences of these single operations. It is similar to what randomInt does, I just do the modulus thing multiple times. Actually I could have used multiple randomInt calls (one for each choice) but I thought this is more efficient since the total amount of information needed to construct a single operation is less than two bytes (so I'm not re-using information, just not wasting it either).

}
for i := range ops {
b := f.randomByte()
if b+byte(i*137) < 4 {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What's the magic of 137?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The only magic here is that it is an odd number and therefore a relative prime to 256 :) So the result of the modulo 256 multiplication sweeps the entire byte range and < 4 can work as a simple quasi-random condition. What I wanted here is to make the set of single operations variable in size so if the first byte of the pair satisfies a quasi-random condition then it is considered the end of the set. The chance of satisfying the condition is 1/64 so the average size of the set is 64.

l = append(l, f.randomByte())
}
if len(l) == 0 || !f.exhausted {
return -1
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think 0 is beter than -1 here. -1 is for "don't ever use this again, even if it did increase coverage"

for i, o := range l {
oplist[r*len(l)+i] = o + b
}
b += 81
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

magic 81?

for ptr < len(list) {
op := list[ptr]
ptr++
if op+byte(*nodeIdx)*111 < 4 {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Magic 111?

@holiman
Copy link
Contributor

holiman commented Dec 27, 2020

Unless the fuzzer is very specialized, like needs very particular inputs in order to get any good coverage, adding the corpus to git doesn't really make sense, IMO, so I'd remove that

The oss-fuzz fuzzing stores the discovered corpus in the cloud anyway, and I think it'll rediscover + surpass your corpus material pretty quickly.

@zsfelfoldi zsfelfoldi closed this Mar 17, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants