Skip to content

les: implement server handler fuzzer#22237

Closed
rjl493456442 wants to merge 3 commits intoethereum:masterfrom
rjl493456442:les-fuzzer-rework
Closed

les: implement server handler fuzzer#22237
rjl493456442 wants to merge 3 commits intoethereum:masterfrom
rjl493456442:les-fuzzer-rework

Conversation

@rjl493456442
Copy link
Copy Markdown
Member

@rjl493456442 rjl493456442 commented Jan 26, 2021

This PR adds the LES fuzzer for fuzzing server handler.

Please keep the two commits from @zsfelfoldi if we decide to merge this PR.

Comment thread les/server_requests.go Outdated
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Just curious, would it be possible to abstract away the reqID from the method handler, and have that handled on the outside?
Like, reply.SetReqId(reqID) ?

I'm wondering, because for eth, when we implement eth/66, it would be neat to use a similar handling, but only set the reqId on the response in the cases where the protocol is 66.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Well. Not sure it's necessary. REQID exists in the protocol all the time.

@rjl493456442 rjl493456442 force-pushed the les-fuzzer-rework branch 2 times, most recently from cceec68 to fb1f727 Compare January 27, 2021 05:42
Comment thread tests/fuzzers/les/les-fuzzer.go Outdated
tests: update les fuzzer

tests/fuzzer/les: release resources

tests/fuzzer/les: pre-initialize all resources
@rjl493456442
Copy link
Copy Markdown
Member Author

Looks like the fuzzer now works correctly

2021/01/29 10:16:56 workers: 8, corpus: 33 (1h10m ago), crashers: 0, restarts: 1/9818, execs: 6706112 (140/sec), cover: 3073, uptime: 13h16m

Comment thread les/server_handler.go
}
}()
}
r := GetBlockHeadersReq{}
Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

@zsfelfoldi I change this to the struct instead of the pointer. Because I think the pointer is unnecessary.

Copy link
Copy Markdown
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.

I'm a bit torn. You've rearchitected it so the basic pattern is

  1. Unmarshall message.
  2. Call message.Serve(backend, foo, bar...).

I'd prefer that it's not based on the actual messages doing the handling but instead

  1. Unmarshall the message
  2. Lookup the handler based on message id or type
  3. hander( msg, backend, foo, bar, ...)

Comment thread go.sum
Comment on lines +113 to +114
github.com/dvyukov/go-fuzz v0.0.0-20210103155950-6a8e9d1f2415 h1:q1oJaUPdmpDm/VyXosjgPgr6wS7c5iV2p0PwJD73bUI=
github.com/dvyukov/go-fuzz v0.0.0-20210103155950-6a8e9d1f2415/go.mod h1:11Gm+ccJnvAhCNLlf5+cS9KjtbaD5I5zaZpFMsTHWTw=
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

These shouldn't really be needed...?

@zsfelfoldi
Copy link
Copy Markdown
Contributor

@holiman I need a struct type definition for each message type in order to unmarshall so it just seemed logical to define the handler function over the message type structs. What is the drawback? Would you prefer putting the decoding of the actual request inside the handler functions? (that just seemed like unnecessary duplicated code to me because I want to do the same rlp decodind and decode error handling for each message type)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants