fix: Only try SNI slicing at offset 0 #2436
Conversation
And also check that the SNI length makes sense. This should fix the spurios failures for `compatible_upgrade_large_initial`.
larseggert
requested review from
KershawChang,
martinthomson and
mxinden
as code owners
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #2436 +/- ##
==========================================
- Coverage 95.28% 95.27% -0.02%
==========================================
Files 114 115 +1
Lines 37111 37188 +77
Branches 37111 37188 +77
==========================================
+ Hits 35363 35432 +69
- Misses 1742 1750 +8
Partials 6 6 ☔ View full report in Codecov by Sentry. |
Failed Interop TestsQUIC Interop Runner, client vs. server, differences relative to db807a9. neqo-latest as client
neqo-latest as server
All resultsSucceeded Interop TestsQUIC Interop Runner, client vs. server neqo-latest as client
neqo-latest as server
Unsupported Interop TestsQUIC Interop Runner, client vs. server neqo-latest as client
neqo-latest as server
|
neqo-transport/src/shuffle.rs
Outdated
| let len = buf.len(); | ||
|
|
||
| assert!(buf[len - 23] == 0x00 && buf[len - 22] == 0x0c); // Check Server Name List length | ||
| // Set Server Name List length to 0 |
There was a problem hiding this comment.
It's what rustfmt generates...
There was a problem hiding this comment.
I'm more concerned about the constants you have littered around. 22 and 23 here, 15 and 39 above. Still, I can't see how that would be easy to fix.
Benchmark resultsPerformance differences relative to d21c121. decode 4096 bytes, mask ff: No change in performance detected. time: [12.317 µs 12.356 µs 12.401 µs]
change: [-0.4865% +0.0248% +0.5095%] (p = 0.93 > 0.05)
decode 1048576 bytes, mask ff: No change in performance detected. time: [2.8369 ms 2.8500 ms 2.8668 ms]
change: [-1.3138% -0.1710% +0.7399%] (p = 0.78 > 0.05)
decode 4096 bytes, mask 7f: No change in performance detected. time: [20.842 µs 20.893 µs 20.950 µs]
change: [-0.4852% -0.0816% +0.3468%] (p = 0.70 > 0.05)
decode 1048576 bytes, mask 7f: No change in performance detected. time: [4.5388 ms 4.5499 ms 4.5625 ms]
change: [-0.4903% -0.0912% +0.3116%] (p = 0.65 > 0.05)
decode 4096 bytes, mask 3f: No change in performance detected. time: [8.2694 µs 8.3006 µs 8.3379 µs]
change: [-1.2261% -0.5416% +0.1235%] (p = 0.13 > 0.05)
decode 1048576 bytes, mask 3f: No change in performance detected. time: [1.5874 ms 1.5929 ms 1.5998 ms]
change: [-0.5193% +0.0011% +0.6042%] (p = 0.98 > 0.05)
coalesce_acked_from_zero 1+1 entries: No change in performance detected. time: [91.397 ns 91.739 ns 92.075 ns]
change: [-1.0561% -0.1376% +0.6514%] (p = 0.78 > 0.05)
coalesce_acked_from_zero 3+1 entries: No change in performance detected. time: [109.72 ns 110.03 ns 110.36 ns]
change: [-0.4000% -0.0167% +0.3604%] (p = 0.94 > 0.05)
coalesce_acked_from_zero 10+1 entries: No change in performance detected. time: [109.19 ns 109.46 ns 109.85 ns]
change: [-1.1485% -0.2899% +0.4251%] (p = 0.52 > 0.05)
coalesce_acked_from_zero 1000+1 entries: No change in performance detected. time: [93.519 ns 98.999 ns 110.82 ns]
change: [-0.6119% +2.1434% +6.3312%] (p = 0.35 > 0.05)
RxStreamOrderer::inbound_frame(): No change in performance detected. time: [112.27 ms 112.41 ms 112.65 ms]
change: [-0.1823% +0.0176% +0.2537%] (p = 0.89 > 0.05)
SentPackets::take_ranges: No change in performance detected. time: [5.2666 µs 5.4236 µs 5.5907 µs]
change: [-1.4466% +1.5877% +4.8881%] (p = 0.32 > 0.05)
transfer/pacing-false/varying-seeds: Change within noise threshold. time: [34.296 ms 34.360 ms 34.427 ms]
change: [-0.7381% -0.4754% -0.2037%] (p = 0.00 < 0.05)
transfer/pacing-true/varying-seeds: Change within noise threshold. time: [34.317 ms 34.371 ms 34.425 ms]
change: [-1.0936% -0.8680% -0.6506%] (p = 0.00 < 0.05)
transfer/pacing-false/same-seed: Change within noise threshold. time: [34.331 ms 34.387 ms 34.443 ms]
change: [-0.6402% -0.4159% -0.2103%] (p = 0.00 < 0.05)
transfer/pacing-true/same-seed: No change in performance detected. time: [34.772 ms 34.838 ms 34.918 ms]
change: [-0.0637% +0.2185% +0.5077%] (p = 0.14 > 0.05)
1-conn/1-100mb-resp/mtu-1504 (aka. Download)/client: No change in performance detected. time: [858.04 ms 868.21 ms 878.66 ms]
thrpt: [113.81 MiB/s 115.18 MiB/s 116.55 MiB/s]
change:
time: [-1.7169% -0.0437% +1.6344%] (p = 0.96 > 0.05)
thrpt: [-1.6081% +0.0437% +1.7469%]
1-conn/10_000-parallel-1b-resp/mtu-1504 (aka. RPS)/client: No change in performance detected. time: [317.56 ms 320.89 ms 324.22 ms]
thrpt: [30.844 Kelem/s 31.164 Kelem/s 31.490 Kelem/s]
change:
time: [-1.0825% +0.4369% +1.9325%] (p = 0.57 > 0.05)
thrpt: [-1.8958% -0.4350% +1.0943%]
1-conn/1-1b-resp/mtu-1504 (aka. HPS)/client: Change within noise threshold. time: [25.325 ms 25.467 ms 25.611 ms]
thrpt: [39.046 elem/s 39.266 elem/s 39.486 elem/s]
change:
time: [-2.0365% -1.1938% -0.2958%] (p = 0.01 < 0.05)
thrpt: [+0.2967% +1.2082% +2.0788%]
1-conn/1-100mb-resp/mtu-1504 (aka. Upload)/client: No change in performance detected. time: [1.8331 s 1.8522 s 1.8714 s]
thrpt: [53.437 MiB/s 53.989 MiB/s 54.553 MiB/s]
change:
time: [-1.3182% +0.0496% +1.4620%] (p = 0.95 > 0.05)
thrpt: [-1.4410% -0.0496% +1.3358%]
Client/server transfer resultsPerformance differences relative to d21c121. Transfer of 33554432 bytes over loopback, 30 runs. All unit-less numbers are in milliseconds.
|
neqo-transport/src/shuffle.rs
Outdated
| let len = buf.len(); | ||
|
|
||
| assert!(buf[len - 23] == 0x00 && buf[len - 22] == 0x0c); // Check Server Name List length | ||
| // Set Server Name List length to 0 |
There was a problem hiding this comment.
I'm more concerned about the constants you have littered around. 22 and 23 here, 15 and 39 above. Still, I can't see how that would be easy to fix.
neqo-transport/src/crypto.rs
Outdated
| @@ -1575,7 +1575,7 @@ impl CryptoStreams { | |||
| let Some((offset, data)) = cs.tx.next_bytes() else { | |||
| return; | |||
| }; | |||
| let written = if sni_slicing { | |||
| let written = if sni_slicing && offset == 0 { | |||
There was a problem hiding this comment.
I'm struggling a bit how to structure that test. Got any suggestions?
There was a problem hiding this comment.
So in our ordinary setup with splitting, we'll send out two packets. If we lose the right one of those (I think we reorder, so we'd want to lose the first), we'll have delivered the first part of the handshake data. Then, we'll be forced to retransmit, which means that the offset will be non-zero when we hit this code again.
To verify that we've not split, we can verify that we get just one frame, not two. Does that work?
There was a problem hiding this comment.
It almost works, except for the verification bit, because that RTX'ed chunk of crypto frame will not look like a valid CH and so the slicing logic will always exit and leave one slice, even if it was called. We'd need to feed in something at a non-zero offset that is a valid-looking CH. Hm...
There was a problem hiding this comment.
Right, so the test would not have failed prior to you adding this check.
Except that it would have, just not reliably. And I think that's OK. If the test won't fail without this check, I'm sure that cargo mutants will complain and we can dismiss that, but I'd rather have the test and not have it reliably able to catch the problem than not have anything.
Thinking more on this, the odds of hitting this issue is probably higher than you would think. We look for a more or less fixed ClientHello shape. There's a few things where we skip vectors, but there we only need to ensure that we don't overrun the buffer, so there will be pure noise that will pass those tests. The real check is where we hit the SNI extension parsing. If we have something that is [0, 0, x, y] where x*256+y is between 3 and the remaining length of the packet, we will treat that as SNI. (We do not check the SNI type or inner length, so this is looser than you might think.) We also get a few swipes at that, jumping forward every time we see [?, ?, x, y].
So while there's a good chance that we'll skip past the end of the buffer and believe there is no SNI, the odds of us hitting this sequence is not as low as I had thought. It's not as high as I'd like, but with a padding extension in play still, it's going to be non-trivial.
Co-authored-by: Martin Thomson <[email protected]> Signed-off-by: Lars Eggert <[email protected]>
Co-authored-by: Martin Thomson <[email protected]> Signed-off-by: Lars Eggert <[email protected]>
Co-authored-by: Martin Thomson <[email protected]> Signed-off-by: Lars Eggert <[email protected]>
| ) | ||
| while let Some((offset, data)) = cs.tx.next_bytes() { | ||
| let written = if sni_slicing && offset == 0 { | ||
| qdebug!("XXX SNI slicing enabled"); |
| @@ -145,4 +147,19 @@ mod tests { | |||
| let buf = [1; 1]; | |||
| assert!(super::find_sni(&buf).is_none()); | |||
| } | |||
|
|
|||
| #[test] | |||
There was a problem hiding this comment.
I'd stick some commentary in here to cover this better.
And also check that the SNI length makes sense.
This should fix the spurios failures for
compatible_upgrade_large_initial.