fix: Abort on RX of illegal stream control/data frame #2269
Conversation
Abort the connection if we receive a stream control or data frame for a stream that is ours to initiate but we haven't yet. Fixes mozilla#1457
larseggert
requested review from
KershawChang,
martinthomson and
mxinden
as code owners
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #2269 +/- ##
==========================================
+ Coverage 93.26% 93.29% +0.03%
==========================================
Files 113 113
Lines 36736 36743 +7
Branches 36736 36743 +7
==========================================
+ Hits 34260 34278 +18
+ Misses 1685 1682 -3
+ Partials 791 783 -8 ☔ View full report in Codecov by Sentry. |
Failed Interop TestsQUIC Interop Runner, client vs. server, differences relative to 3001a3a. 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
|
Benchmark resultsPerformance differences relative to 3001a3a. decode 4096 bytes, mask ff: No change in performance detected. time: [11.863 µs 11.907 µs 11.955 µs]
change: [-0.8015% -0.2596% +0.1811%] (p = 0.33 > 0.05)
decode 1048576 bytes, mask ff: No change in performance detected. time: [3.0999 ms 3.1146 ms 3.1344 ms]
change: [-0.7800% -0.1106% +0.6190%] (p = 0.77 > 0.05)
decode 4096 bytes, mask 7f: No change in performance detected. time: [19.793 µs 19.850 µs 19.914 µs]
change: [-0.4499% +0.1803% +0.9863%] (p = 0.70 > 0.05)
decode 1048576 bytes, mask 7f: No change in performance detected. time: [5.1674 ms 5.1800 ms 5.1931 ms]
change: [-0.3159% +0.0386% +0.3831%] (p = 0.82 > 0.05)
decode 4096 bytes, mask 3f: No change in performance detected. time: [6.8938 µs 6.9176 µs 6.9487 µs]
change: [-0.0720% +0.2477% +0.6357%] (p = 0.19 > 0.05)
decode 1048576 bytes, mask 3f: Change within noise threshold. time: [1.7581 ms 1.7596 ms 1.7624 ms]
change: [-1.1084% -0.6414% -0.1788%] (p = 0.00 < 0.05)
coalesce_acked_from_zero 1+1 entries: Change within noise threshold. time: [98.320 ns 98.625 ns 98.943 ns]
change: [-1.2795% -0.7355% -0.1716%] (p = 0.01 < 0.05)
coalesce_acked_from_zero 3+1 entries: No change in performance detected. time: [116.62 ns 116.99 ns 117.41 ns]
change: [-1.7072% -0.8205% +0.0533%] (p = 0.06 > 0.05)
coalesce_acked_from_zero 10+1 entries: No change in performance detected. time: [116.39 ns 116.90 ns 117.50 ns]
change: [-0.7090% +0.0542% +1.2521%] (p = 0.93 > 0.05)
coalesce_acked_from_zero 1000+1 entries: No change in performance detected. time: [97.397 ns 97.522 ns 97.659 ns]
change: [-0.9579% -0.1305% +0.6837%] (p = 0.77 > 0.05)
RxStreamOrderer::inbound_frame(): Change within noise threshold. time: [111.42 ms 111.55 ms 111.77 ms]
change: [+0.2168% +0.3507% +0.5489%] (p = 0.00 < 0.05)
SentPackets::take_ranges: No change in performance detected. time: [5.5491 µs 5.7333 µs 5.9269 µs]
change: [-3.3931% -0.2058% +2.8206%] (p = 0.90 > 0.05)
transfer/pacing-false/varying-seeds: Change within noise threshold. time: [30.456 ms 31.010 ms 31.545 ms]
change: [+2.6499% +4.9762% +7.5992%] (p = 0.00 < 0.05)
transfer/pacing-true/varying-seeds: Change within noise threshold. time: [32.468 ms 33.013 ms 33.548 ms]
change: [+1.2374% +3.7078% +6.1906%] (p = 0.00 < 0.05)
transfer/pacing-false/same-seed: Change within noise threshold. time: [25.707 ms 26.318 ms 26.919 ms]
change: [-0.0276% +3.4210% +6.6871%] (p = 0.04 < 0.05)
transfer/pacing-true/same-seed: Change within noise threshold. time: [29.266 ms 30.100 ms 30.918 ms]
change: [+0.1254% +4.4562% +8.6628%] (p = 0.04 < 0.05)
1-conn/1-100mb-resp/mtu-1504 (aka. Download)/client: 💚 Performance has improved. time: [868.52 ms 877.55 ms 886.72 ms]
thrpt: [112.77 MiB/s 113.95 MiB/s 115.14 MiB/s]
change:
time: [-4.3887% -2.9423% -1.5277%] (p = 0.00 < 0.05)
thrpt: [+1.5514% +3.0315% +4.5902%]
1-conn/10_000-parallel-1b-resp/mtu-1504 (aka. RPS)/client: No change in performance detected. time: [313.60 ms 316.84 ms 320.06 ms]
thrpt: [31.244 Kelem/s 31.562 Kelem/s 31.888 Kelem/s]
change:
time: [-1.3608% +0.1134% +1.5447%] (p = 0.88 > 0.05)
thrpt: [-1.5212% -0.1132% +1.3796%]
1-conn/1-1b-resp/mtu-1504 (aka. HPS)/client: Change within noise threshold. time: [33.388 ms 33.557 ms 33.736 ms]
thrpt: [29.642 elem/s 29.800 elem/s 29.951 elem/s]
change:
time: [-1.7381% -0.9060% -0.1014%] (p = 0.04 < 0.05)
thrpt: [+0.1015% +0.9143% +1.7688%]
1-conn/1-100mb-resp/mtu-1504 (aka. Upload)/client: 💚 Performance has improved. time: [1.6976 s 1.7150 s 1.7326 s]
thrpt: [57.718 MiB/s 58.310 MiB/s 58.908 MiB/s]
change:
time: [-3.9737% -2.5333% -1.0640%] (p = 0.00 < 0.05)
thrpt: [+1.0754% +2.5992% +4.1381%]
Client/server transfer resultsTransfer of 33554432 bytes over loopback.
|
Co-authored-by: Max Inden <[email protected]> Signed-off-by: Lars Eggert <[email protected]>
There was a problem hiding this comment.
Looking at this more, I'm really not sure that this is safe.
There are cases where the frame is outright illegal. But the code you routinely use here looks like this:
if let (_, Some(rs)) = self.obtain_stream(...)? {
// happy path
} else {
return Err(Error::WHATEVER);
}The problem is that valid frames can arrive late. As @mxinden points out, this results in the stream being removed. The return value would then be (None, None), which will hit the error case. We'll kill connections that are completely valid in that case.
I think that you want a different test for whether the frame is valid than what you have described here. My expectation is that obtain_stream() will be able to catch some cases (where flow control doesn't allow the stream to be created), but not others (like whether you want the receive stream where there shouldn't be one).
A more targeted function (or pair of functions) might do better because it could know that you want send or receive and abort if that part of the stream cannot exist.
That you have not hit this case suggests to me that we need a test that uses and retires streams fully, while sending a bunch of frames that are valid, but very late.
Co-authored-by: Martin Thomson <[email protected]> Signed-off-by: Lars Eggert <[email protected]>
|
@mxinden @martinthomson agree. Have been trying to craft a test that hits those error cases, but no luck so far. |
|
There now is a |
|
OK, I think I have tests now that hits the new code paths for all frame types. |
Co-authored-by: Max Inden <[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]>
Co-authored-by: Max Inden <[email protected]> Signed-off-by: Lars Eggert <[email protected]>
Abort the connection if we receive a stream control or data frame for a stream that is ours to initiate but we haven't yet. Don't abort if it is such a frame that arrives very late for a stream that we did use.
Fixes #1457