Add fuzzer for SIP parser #41
Conversation
| @@ -27,11 +38,11 @@ func ParseUri(uriStr string, uri *sip.Uri) (err error) { | |||
| func uriStateSIP(uri *sip.Uri, s string) (uriFSM, string, error) { | |||
| switch s[0] { | |||
| case 'S', 's': | |||
| if s[3] == 'S' || s[3] == 's' { | |||
There was a problem hiding this comment.
I think that maybe we should require sip/sips to be present, per comment above. This maybe would then simplify check
There was a problem hiding this comment.
Updated the code to check for sip: and sips:.
There was a problem hiding this comment.
ok this maybe now decreases performance.
We need also to check upper Case. SIP, SIPS, that is why old implementation was looking both.
I mostly wanted to remove default case, but keep this small check.
This is how is done in some other implements, compromise performance and full check.
There was a problem hiding this comment.
We need also to check upper Case. SIP, SIPS, that is why old implementation was looking both.
strings.EqualFold does case-insensitive matching. I added test cases to verify that we match both sip: and SIP:.
I don't think a huge amount of performance is lost here.
oystedal
force-pushed
the
oystedal/fuzzing
branch
from
0fb7484
to
0720818
Compare
…rsion in via header
| @@ -0,0 +1,2 @@ | |||
| go test fuzz v1 | |||
There was a problem hiding this comment.
I guess this was failing examples, but now they are fixed.
Do we really need to hold on this?
Files are just very autogenerated and I guess they can be created with fuzz test every time (if not fixed already)
There was a problem hiding this comment.
These files become test cases that help verify that the same kind of crash is not reintroduced into the parser later, so it's a free test case as such. If you undo one of my changes and run go test you'll see that the test run fails.
These files are based on crashing cases that the fuzzer has found, they will not be generated just from new runs of the parser after the crash has been fixed.
I agree that the fuzzer ends up generating very quirky examples. Part of the reason is that it works hard to make the test cases as minimal as possible.
|
@oystedal forgot to mention. Very cool, thnx for contributing. |
|
merged. @oystedal |
Fuzzing input received from the network is a best practice for security and correctness. Go 1.18 added native support for fuzzing, see https://go.dev/doc/tutorial/fuzz and https://go.dev/security/fuzz.
The first commit introduces the fuzzer, which can be executed with
go test -fuzz=Fuzz. For now, this starts off with a trivial SIP message. This can be expanded in the future by adding additional input strings as needed.The subsequent commits are fixes to various issues found during execution of the fuzzer. Note that each of the files under
parser/testdata/fuzzautomatically become tests that are executed when runninggo test. This ensures that previously discovered crashes are not reintroduced with new changes.I have executed the fuzzer for a little over two hours after fixing these issues without finding any additional issues.