Invalid assumption about SocketAddrV{4,6} layout

[djc]

It looks like we're assuming that std::net::Ipv4 matches the libc sockaddr_in (same for IPv6), for example in https://github.com/quinn-rs/quinn/blob/main/quinn/src/platform/unix.rs#L357. However, the standard library never guaranteed this, and in fact is looking at changing this in rust-lang/rust#78802. We should fix our usage.

[est31]

Are older versions affected too? Would it be possible to release point releases for them? Even the very old ones? That would be really nice.

[djc]

Yeah, I went through older releases down to 0.3, which also look affected. I'm not sure it really makes sense to publish bugfix releases for anything older than 0.6, do you really those still get significant usage?

(On the other hand, if you'd be able to lend a hand in fixing/backporting, I wouldn't mind publishing some extra stuff on crates.io.)

[est31]

It would be cool if my older mimas releases still compiled. IDK how many changes a fix requires and how much the piece of code has changed between quinn releases.

[est31]

And yes, I'm willing to help.

[djc]

I'm guessing they would still compile, just get issues at run-time.

Pretty sure the relevant code hasn't changed all that much between releases.

[Matthias247]

FWIW, there seems to be an issue even today - without the Rust changes. we could observe some issues with IPv6 support where an incoming packet lead the endpoint to shut down due to an IO error.

After removing more MaybeUninits from socket layer I figured out that removing the one for socket addresses will break the receive path - decoded socket addresses get malformed. I uploaded a repro in #976

So far I don't understand why this happens, and whether there's some rust-UB issues to do having pointers of sockaddr_storage and sockaddr_in[6].

When I changed the socket address conversion to make use of https://docs.rs/socket2/0.3.19/socket2/struct.SockAddr.html#method.from_raw_parts and https://docs.rs/socket2/0.3.19/socket2/struct.SockAddr.html#method.as_std things seemed ok.

[djc]

I wonder if we could run some simple Quinn tests through Miri.

[Matthias247]

I think it doesn't do any threading. And even if it would, it would likely be too slow and not a great indicator if something is definitely wrong or if the Miri model just doesn't capture it yet.

[Ralith]

MIRI should only be needed for the unsafe platform API stuff, which we definitely could write a standalone test for. In any case, we should just replace this particular code ASAP.

[Matthias247]

A question for the fix will be whether to always convert between std and libc representations on IO calls, or whether to switch to different address types internally which have guarantees about being platform lib compatible (I guess socket2 does). public accessors (e.g. for peer addresses) could still perform a conversion to std.

Converting the representation is definitely not costly. But given that quic library does this thousands of times per second (compared to a TCP stack which does a handful conversions at connection establishment) it might be worthwhile to avoid it.

[djc]

I agree with what appears to be your assessment (that we should try to avoid the conversion on every send/recv).

[Matthias247]

Also seems like socket2 only has SocketAddr types, not IpAddr ones which make guarantees about wrapping in_addr. But maybe there is no issue, since all are plain byte arrays?

[djc]

Or we could ask about adding them to socket2.

[faern]

I don't think you have to worry too much about the performance of the conversion. The conversion takes very little time. If part of any operation that also contains a syscall it's dwarfed by the syscall.

You can see the benchmark results from my initial look at this before I created the PR that will change the std representation. Even having UdpSocket::local_addr in a tight loop suffers no measurable performance difference from having to do the conversion. https://internals.rust-lang.org/t/why-are-socketaddrv4-socketaddrv6-based-on-low-level-sockaddr-in-6/13321/15

You can find my benchmarks here if you are interested in verifying their correctness or apply them to this crate and do your own measurements: https://github.com/faern/socketaddr-benchmark

[Matthias247]

@faern The thing with Quic is that it won't be just 1 conversion per syscall. With sendmmsg/recvmmsg calls and cmsgs (which contain additional addresses) it could easily become 100 conversions per syscall. The cost might indeed still be dwarfed by the one of other operations. But if this library e.g. aims for reaching 10Gbit/s per core (to get on parity with TLS+TCP), the per-packet cost of all operations needs to be minimized, and one has to start somewhere :-)

[faern]

I'd always prefer correctness over speed. If I were you I would fix the SocketAddr casts first and optimize them later (if benchmarks shows they are a bottleneck). I still think you overestimate how much time the conversion take.

[est31]

Made a PR for the latest release: #987

Once that one's merged and tested, one can think about backporting it to older releases.

[faern]

Will there be patch releases to existing 0.x version of quinn? How far back was this invalid assumption made and are there still users of those versions?

[Matthias247]

I don’t think it’s worthwhile to think a lot about old releases. Since the last release there had been a ton of bugfixes and perf improvements that people would miss out on anyway.

[est31]

It's definitely my plan to get this backported to older quinn versions. How far back, no idea.

[faern]

If it's not backported and there are actual users, this would either 1) block rust-lang/rust#78802 from progressing, or 2) cause UB for your users once that lands. But it of course depends on how many users you have. All other affected crates have not backported it all the way to 0.0.0 but that was mostly because the very early releases were very old with virtually no users, and those crates were being yanked anyway when this fix landed.

[djc]

According to crates.io downloads, everything but 0.6.1 is practically unused. In the chart, the next most popular point is Other, which contains everything pre-0.4.0.

[est31]

Can we get a 0.6.x branch? I can then prepare PRs for 0.5.x and 0.6.x.

[djc]

Done.

[est31]

So apparently f3d82d7 does not need backporting for 0.6.x and before, because the code it changed was added in 1ca7149, which wasn't present in 0.6.x.

Only 29d37aa seems to need backporting...

[Ralith]

@faern

rust-lang/rust#78802 states:

SocketAddrV4 only occupies 6 bytes instead of 16 bytes.

Our older versions run

        assert_eq!(
            mem::size_of::<SocketAddrV4>(),
            mem::size_of::<libc::sockaddr_in>()
        );

before invoking any code that relies on the invalid assumption, so if SocketAddrV4's size is changing then I think the worst-case scenario is reasonably traceable panics on startup.

[faern]

It's good that the size is asserted at least. But if the size remains the same and the memory layout changes you will still have a bad time. However, if my PR passes it's very likely that the size will change, yes.

[djc]

Thanks to @est31, I've just published 0.5.4 and 0.6.2 releases that contain the relevant fixes.

[faern]

That's awesome! Thank you. I can now try to move the PR on std as the last known offenders have patches out.

[djc]

FWIW, I also brought up the idea of yanking our buggy versions on Matrix, so we'll likely do that too (though we may take a few days before yanking 0.6.1). Do you think we should also put out a RustSec advisory for quinn, as you've done for the others?

[faern]

Yes. I think adding a RustSec advisory is recommended IMO! It does not hurt, is pretty easy to do and only has upsides. You can just copy what I did for the other crates basically.

Yanking + security advisory is kind of all we can do to try to encourage upgrades. I just don't want there to ever be a situation where anyone argues "There is no reported vulnerability on this version, so I'll keep it".

[djc]

Advisory PR submitted: rustsec/advisory-db#804.