fix: Don't recurse when parsing SCONE#3460
Merged
Merged
Conversation
Replace recursion in `decode_inner` with a loop. A crafted datagram of back-to-back SCONE packets can otherwise overflow the stack. Fixes mozilla#3458
larseggert
requested review from
KershawChang,
martinthomson and
mxinden
as code owners
Contributor
There was a problem hiding this comment.
Pull request overview
This PR hardens QUIC packet parsing in neqo-transport by removing recursion when skipping SCONE packets, preventing stack overflow from crafted inputs (per OSS-Fuzz #3458).
Changes:
- Replaces recursive re-entry in
Public::decode_inner()(when encountering SCONE) with an iterative loop. - Preserves existing SCONE “ignore” behavior while ensuring parsing continues safely on remaining bytes.
- Adds a regression test that feeds a long chain of SCONE packets to validate no stack overflow occurs.
You can also share your feedback on Copilot code review. Take the survey.
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #3460 +/- ##
==========================================
- Coverage 94.29% 94.17% -0.13%
==========================================
Files 127 131 +4
Lines 38680 39021 +341
Branches 38680 39021 +341
==========================================
+ Hits 36474 36747 +273
- Misses 1365 1424 +59
- Partials 841 850 +9
Flags with carried forward coverage won't be shown. Click here to find out more.
|
Collaborator
Author
|
@martinthomson or would a simpler approach be to allow only one SCONE indication in a packet? |
Contributor
Failed Interop TestsQUIC Interop Runner, client vs. server, differences relative to
All resultsSucceeded Interop TestsQUIC Interop Runner, client vs. server neqo-pr as client
neqo-pr as server
Unsupported Interop TestsQUIC Interop Runner, client vs. server neqo-pr as client
neqo-pr as server
|
Contributor
Benchmark resultsSignificant performance differences relative to 2fbd329. transfer/1-conn/1-100mb-resp (aka. Download)/mtu-1504: 💔 Performance has regressed by +2.0157%. time: [204.22 ms 204.60 ms 205.00 ms]
thrpt: [487.81 MiB/s 488.76 MiB/s 489.66 MiB/s]
change:
time: [+1.6390% +2.0157% +2.3763] (p = 0.00 < 0.05)
thrpt: [-2.3212% -1.9759% -1.6125]
Performance has regressed.
Found 4 outliers among 100 measurements (4.00%)
1 (1.00%) low mild
2 (2.00%) high mild
1 (1.00%) high severeAll resultstransfer/1-conn/1-100mb-resp (aka. Download)/mtu-1504: 💔 Performance has regressed by +2.0157%. time: [204.22 ms 204.60 ms 205.00 ms]
thrpt: [487.81 MiB/s 488.76 MiB/s 489.66 MiB/s]
change:
time: [+1.6390% +2.0157% +2.3763] (p = 0.00 < 0.05)
thrpt: [-2.3212% -1.9759% -1.6125]
Performance has regressed.
Found 4 outliers among 100 measurements (4.00%)
1 (1.00%) low mild
2 (2.00%) high mild
1 (1.00%) high severetransfer/1-conn/10_000-parallel-1b-resp (aka. RPS)/mtu-1504: No change in performance detected. time: [285.09 ms 286.96 ms 288.82 ms]
thrpt: [34.623 Kelem/s 34.849 Kelem/s 35.077 Kelem/s]
change:
time: [-0.6680% +0.3438% +1.4595] (p = 0.53 > 0.05)
thrpt: [-1.4386% -0.3426% +0.6725]
No change in performance detected.transfer/1-conn/1-1b-resp (aka. HPS)/mtu-1504: No change in performance detected. time: [38.557 ms 38.722 ms 38.903 ms]
thrpt: [25.705 B/s 25.825 B/s 25.936 B/s]
change:
time: [-0.4973% +0.1460% +0.7203] (p = 0.64 > 0.05)
thrpt: [-0.7151% -0.1458% +0.4998]
No change in performance detected.
Found 10 outliers among 100 measurements (10.00%)
3 (3.00%) high mild
7 (7.00%) high severetransfer/1-conn/1-100mb-req (aka. Upload)/mtu-1504: Change within noise threshold. time: [205.96 ms 206.56 ms 207.37 ms]
thrpt: [482.23 MiB/s 484.11 MiB/s 485.54 MiB/s]
change:
time: [+0.9163% +1.2987% +1.7758] (p = 0.00 < 0.05)
thrpt: [-1.7448% -1.2820% -0.9080]
Change within noise threshold.
Found 3 outliers among 100 measurements (3.00%)
1 (1.00%) high mild
2 (2.00%) high severestreams/walltime/1-streams/each-1000-bytes: Change within noise threshold. time: [582.80 µs 584.85 µs 587.25 µs]
change: [-1.5092% -0.9411% -0.3108] (p = 0.00 < 0.05)
Change within noise threshold.
Found 8 outliers among 100 measurements (8.00%)
8 (8.00%) high severestreams/walltime/1000-streams/each-1-bytes: Change within noise threshold. time: [12.380 ms 12.409 ms 12.449 ms]
change: [+0.1834% +0.4695% +0.7795] (p = 0.00 < 0.05)
Change within noise threshold.
Found 1 outliers among 100 measurements (1.00%)
1 (1.00%) high severestreams/walltime/1000-streams/each-1000-bytes: Change within noise threshold. time: [44.614 ms 44.657 ms 44.701 ms]
change: [+0.3846% +0.5858% +0.7670] (p = 0.00 < 0.05)
Change within noise threshold.
Found 3 outliers among 100 measurements (3.00%)
3 (3.00%) high mildtransfer/walltime/pacing-false/varying-seeds: Change within noise threshold. time: [78.419 ms 78.509 ms 78.612 ms]
change: [-2.9660% -2.8088% -2.6519] (p = 0.00 < 0.05)
Change within noise threshold.
Found 2 outliers among 100 measurements (2.00%)
2 (2.00%) high severetransfer/walltime/pacing-true/varying-seeds: No change in performance detected. time: [80.082 ms 80.245 ms 80.441 ms]
change: [-0.4766% -0.2120% +0.0788] (p = 0.13 > 0.05)
No change in performance detected.
Found 14 outliers among 100 measurements (14.00%)
3 (3.00%) high mild
11 (11.00%) high severetransfer/walltime/pacing-false/same-seed: Change within noise threshold. time: [78.057 ms 78.152 ms 78.275 ms]
change: [-3.2581% -3.0508% -2.8618] (p = 0.00 < 0.05)
Change within noise threshold.
Found 4 outliers among 100 measurements (4.00%)
1 (1.00%) high mild
3 (3.00%) high severetransfer/walltime/pacing-true/same-seed: Change within noise threshold. time: [80.083 ms 80.223 ms 80.382 ms]
change: [-0.6675% -0.4298% -0.1741] (p = 0.00 < 0.05)
Change within noise threshold.
Found 11 outliers among 100 measurements (11.00%)
3 (3.00%) high mild
8 (8.00%) high severeDownload data for |
Contributor
Client/server transfer resultsPerformance differences relative to 2fbd329. Transfer of 33554432 bytes over loopback, min. 100 runs. All unit-less numbers are in milliseconds.
Table above only shows statistically significant changes. See all results below. All resultsTransfer of 33554432 bytes over loopback, min. 100 runs. All unit-less numbers are in milliseconds.
Download data for |
Merged
mxinden
approved these changes
Mar 18, 2026
Member
mxinden
left a comment
mxinden
left a comment
There was a problem hiding this comment.
In favor of looping instead of recursing.
Collaborator
Author
|
@martinthomson suggest to land this and then revise if/when SCONE changes? |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Replace recursion in
decode_innerwith a loop. A crafted datagram of back-to-back SCONE packets can otherwise overflow the stack.Fixes #3458