fix(client): prevent out-of-range slice access #1569

mxinden · 2024-01-19T11:43:10Z

The Quic Network Simulator handshakeloss testcase instructs the neqo-client to download from multiple URLs in series. The client takes one URL after the next and tries to request it:

to_request = &remaining[..1];
remaining = &remaining[1..];

This will panic on the last URL, trying to access the first element in the empty slice remaining.

This commit removes the out-of-range access. Instead of using slice operations which hide potential panics, it uses owned collections (VecDeque and Vec) and their safe methods (e.g. pop_front, drain).

Needed for quic-interop/quic-interop-runner#344.
Related to #1552.

The Quic Network Simulator `handshakeloss` testcase instructs the `neqo-client` to download from multiple URLs in series. The client takes one URL after the next and tries to request it: ``` rust to_request = &remaining[..1]; remaining = &remaining[1..]; ``` This will panic on the last URL, trying to access the first element in an empty slice. This commit removes the out-of-range access. Instead of using slice operations which hide potential panics, it uses owned collections (`VecDeque` and `Vec`) and their safe methods (e.g. `pop_front`, `drain`).

codecov-commenter · 2024-01-19T11:48:24Z

Codecov Report

Attention: 30 lines in your changes are missing coverage. Please review.

Comparison is base (a334be2) 87.60% compared to head (526fb0e) 87.61%.

Files	Patch %	Lines
neqo-client/src/main.rs	0.00%	30 Missing ⚠️

Additional details and impacted files

@@           Coverage Diff           @@
##             main    #1569   +/-   ##
=======================================
  Coverage   87.60%   87.61%           
=======================================
  Files         117      117           
  Lines       38333    38326    -7     
=======================================
- Hits        33583    33579    -4     
+ Misses       4750     4747    -3

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

martinthomson · 2024-01-21T23:28:14Z

neqo-client/src/main.rs

-                to_request = remaining;
-                remaining = &[][..];
-            }
+                urls.drain(..).collect()


Suggested change

urls.drain(..).collect()

std::mem::take(urls)

martinthomson · 2024-01-21T23:34:19Z

neqo-client/src/main.rs

@@ -832,7 +832,7 @@ fn handle_test(
    local_addr: SocketAddr,
    remote_addr: SocketAddr,
    hostname: &str,
-    urls: &[Url],
+    urls: Vec<Url>,


I would prefer if you changed this to a VecDeque. You can see that this argument is transformed into a VecDeque for processing anyway, so you might as well save a transformation.

(At first, this change looked to be gratuitous. The change works without it. But I think that you are right to change to a pass-by-value in this case as the test functions all basically rely on having ownership of the list of URLs.)

I missed this. Thank you.

ad02584

martinthomson · 2024-01-21T23:36:11Z

neqo-client/src/main.rs

    for url in &args.urls {
        let entry = urls_by_origin.entry(url.origin()).or_default();
-        entry.push(url.clone());
+        entry.push_back(url.clone());
    }


Looking at this, I find this loop annoy, because it could produce what the next loop needs (host, port) -> Url more directly.

See a0975fc.

martinthomson · 2024-01-21T23:37:44Z

neqo-client/src/main.rs

-                remaining = &remaining[1..];
-                if args.resume && first && remaining.is_empty() {
+            let to_request = if (args.resume && first) || args.download_in_series {
+                if args.resume && first && urls.len() < 2 {


Another annoying thing. This check could be made outside of the loop, in which case you wouldn't need first.

Cool. Yep. Simplifies things.

25dd1a8

…t-of-bound

mxinden · 2024-01-22T14:44:04Z

@martinthomson thank you for the review. Can you take another look?

martinthomson

Very nice. Thanks for indulging me. That new per-origin rollup is much cleaner.

mxinden · 2024-01-23T11:01:26Z

Great. Any time @martinthomson!

QUIC Interop Runner testcases `resumption` and `0-rtt` expect the client to download the first file, close the connection, and then download the remaining files on a second connection. https://github.com/quic-interop/quic-interop-runner/tree/master#test-cases Thus `neqo-client`, when `args.resume` is `true` must only take a single URL on the **first** loop iteration. On the second iteration it must take all remaining URLs. Regression introduced in mozilla#1569.

QUIC Interop Runner testcases `resumption` and `0-rtt` expect the client to download the first file, close the connection, and then download the remaining files on a second connection. https://github.com/quic-interop/quic-interop-runner/tree/master#test-cases Thus `neqo-client`, when `args.resume` is `true` must only take a single URL on the **first** loop iteration. On the second iteration it must take all remaining URLs. Regression introduced in #1569.

There are two server implementations based on neqo: 1. https://github.com/mozilla/neqo/tree/main/neqo-bin/src/server - http3 and http09 implementation - used for manual testing and QUIC Interop 2. https://searchfox.org/mozilla-central/source/netwerk/test/http3server/src/main.rs - used to test Firefox I assume one was once an exact copy of the other. Both implement their own I/O, event loop, ... Since then, the two implementations diverged significantly. Especially (1) saw a lot of improvements in recent months: - mozilla#1564 - mozilla#1569 - mozilla#1578 - mozilla#1581 - mozilla#1604 - mozilla#1612 - mozilla#1676 - mozilla#1692 - mozilla#1707 - mozilla#1708 - mozilla#1727 - mozilla#1753 - mozilla#1756 - mozilla#1766 - mozilla#1772 - mozilla#1786 - mozilla#1787 - mozilla#1788 - mozilla#1794 - mozilla#1806 - mozilla#1808 - mozilla#1848 - mozilla#1866 At this point, bugs in (2) are hard to fix, see e.g. mozilla#1801. This commit merges (2) into (1), thus removing all duplicate logic and having (2) benefit from all the recent improvements to (1).

* refactor(bin): introduce server/http3.rs and server/http09.rs The QUIC Interop Runner requires an http3 and http09 implementation for both client and server. The client code is already structured into an http3 and an http09 implementation since #1727. This commit does the same for the server side, i.e. splits the http3 and http09 implementation into separate Rust modules. * refactor: merge mozilla-central http3 server into neqo-bin There are two server implementations based on neqo: 1. https://github.com/mozilla/neqo/tree/main/neqo-bin/src/server - http3 and http09 implementation - used for manual testing and QUIC Interop 2. https://searchfox.org/mozilla-central/source/netwerk/test/http3server/src/main.rs - used to test Firefox I assume one was once an exact copy of the other. Both implement their own I/O, event loop, ... Since then, the two implementations diverged significantly. Especially (1) saw a lot of improvements in recent months: - #1564 - #1569 - #1578 - #1581 - #1604 - #1612 - #1676 - #1692 - #1707 - #1708 - #1727 - #1753 - #1756 - #1766 - #1772 - #1786 - #1787 - #1788 - #1794 - #1806 - #1808 - #1848 - #1866 At this point, bugs in (2) are hard to fix, see e.g. #1801. This commit merges (2) into (1), thus removing all duplicate logic and having (2) benefit from all the recent improvements to (1). * Move firefox.rs to mozilla-central * Reduce HttpServer trait functions * Extract constructor * Remove unused deps * Remove clap color feature Nice to have. Adds multiple dependencies. Hard to justify for mozilla-central.

mxinden requested review from KershawChang and martinthomson as code owners January 19, 2024 11:43

martinthomson reviewed Jan 21, 2024

View reviewed changes

mxinden added 4 commits January 22, 2024 12:46

Pass VecDeque

ad02584

Fold two loops into iterator chain

a0975fc

Move first check outside of loop and refactor into while

25dd1a8

Merge branch 'main' of https://github.com/mozilla/neqo into client-ou…

526fb0e

…t-of-bound

martinthomson approved these changes Jan 23, 2024

View reviewed changes

martinthomson merged commit ed13307 into mozilla:main Jan 23, 2024
9 checks passed

mxinden mentioned this pull request Jan 23, 2024

fix(client): only take single url on (args.resume && first) #1578

Merged

mxinden mentioned this pull request May 4, 2024

refactor: enable mozilla-central http3server to use neqo-bin #1878

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(client): prevent out-of-range slice access #1569

fix(client): prevent out-of-range slice access #1569

mxinden commented Jan 19, 2024

codecov-commenter commented Jan 19, 2024 •

edited

Loading

martinthomson Jan 21, 2024

mxinden Jan 22, 2024

martinthomson Jan 21, 2024

mxinden Jan 22, 2024

martinthomson Jan 21, 2024

mxinden Jan 22, 2024

martinthomson Jan 21, 2024

mxinden Jan 22, 2024

mxinden commented Jan 22, 2024

martinthomson left a comment

mxinden commented Jan 23, 2024

fix(client): prevent out-of-range slice access #1569

fix(client): prevent out-of-range slice access #1569

Conversation

mxinden commented Jan 19, 2024

codecov-commenter commented Jan 19, 2024 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mxinden commented Jan 22, 2024

martinthomson left a comment

Choose a reason for hiding this comment

mxinden commented Jan 23, 2024

codecov-commenter commented Jan 19, 2024 •

edited

Loading