Conversation
rpaulo
changed the title
0675b5b to
7ff102f
Compare
38d93ae to
038af1d
Compare
| ) throws(QUICError) | ||
| } | ||
|
|
||
| #if Fuzzing |
There was a problem hiding this comment.
What do you think of this broken out into its own file Protector+SecFramerNull.swift or Protector+Fuzzing.swift
| enum KeyType: Equatable { | ||
| case aesGCM | ||
| case chaChaPoly | ||
| #if Fuzzing |
There was a problem hiding this comment.
This causes lots of #if statements throughout the code which I think in general makes a codebase hard to follow and reason about. I'm wondering if we could get away with unconditionally defining the .null case and having strategic fatalErrors or similar which trigger if you use it in a non-fuzzing context. Maybe using an inlinable assertFuzzing() call with an #if !Fuzzing in it?
There was a problem hiding this comment.
I really didn't want to compile it in !Fuzzing because it's somewhat dangerous to have this option.
There was a problem hiding this comment.
Can you expand on that? I'm not sure I follow.
There was a problem hiding this comment.
I want the null fuzzer to never be compiled into production or tests. Only when we are fuzzing. I'm probably splitting hairs but not having the enum available outside of Fuzzing is a bit safer than fatalError().
| } | ||
|
|
||
| if closeError != nil { | ||
| if closeError != nil || state.isTerminal { |
There was a problem hiding this comment.
Is this a bug which fuzzing surfaced? Are some of those included in this PR?
There was a problem hiding this comment.
Yes, I'm moving these fixes to #157 (review)
| private(set) var knownFlows = [QUICStreamID: MultiplexedFlowIdentifier]() | ||
|
|
||
| private(set) var localCIDLength: Int = 0 | ||
| var localCIDLength: Int = 0 |
There was a problem hiding this comment.
In SwiftNIO-family repos there is a convention to annotate access-widens like this with a reason, so that it's clear that this would ideally have narrower access but tests need it e.g.
var localCIDLength: Int = 0 // would be private(set)This isn't NIO, so it's your choice 😄
| @@ -0,0 +1,25 @@ | |||
| name: Nightly fuzzing | |||
There was a problem hiding this comment.
Since this PR was opened I made a main.yml job which runs daily against main, I think this might make sense in there? But on the other hand it would then be rolled in to a top level status, your choice.
There was a problem hiding this comment.
If fuzzing fails, it will be clear that it's the "Fuzzing" job that failed, not the "Main" job, that's why I think they should be separate. Also, I prefer to work with smaller files... :)
rnro
left a comment
rnro
left a comment
There was a problem hiding this comment.
Just a couple of comments to consider but it looks good overall.
…r parsing - Add `Fuzzing` package trait wiring fuzzer/ASan flags and two fuzz targets (`FuzzQUICPackets`, `FuzzTransportParameters`), plus CI and nightly workflow integration with corpus caching - Add no-op `SecFramerNull` protector (FuzzBuild only) so fuzz inputs are treated as already-plaintext on the receive path - Stop processing coalesced frames/packets once the connection is terminal, add `QUICFrame.discard`, guard missing TLS options on INITIAL retransmission, and clear crypto reassembly queues on stop
4 participants
Fuzzingpackage trait wiring fuzzer/ASan flags and two fuzz targets (FuzzQUICPackets,FuzzTransportParameters), plus CI and nightly workflow integration with corpus cachingSecFramerNullprotector (FuzzBuild only) so fuzz inputs are treated as already-plaintext on the receive pathQUICFrame.discard, guard missing TLS options on INITIAL retransmission, and clear crypto reassembly queues on stop