fix(iroh-gossip): connection loop misuses tokio::select! leading to read errors

[Frando]

Description

The connection loop of iroh-gossip misused tokio-select by selecting over a future that is not cancellation safe. This means that if the timings are bad, the message reading future would be aborted midway in a message, and then restart by reading a length, which would then yield some random number because it would be reading some random bytes in the middle of a message. This means it would lead to random connection drops.

Breaking Changes

Backport from #2570

Notes & open questions

Change checklist

• Self-review.
• Documentation updates following the style guide, if relevant.
• Tests if relevant.
• All breaking changes documented.

[github-actions]

Documentation for this PR has been generated and is available at: https://n0-computer.github.io/iroh/pr/2572/docs/iroh/

Last updated: 2024-08-01T14:35:35Z

[dignifiedquire]

I wonder if we have this bug in other parts of the stack as well, iroh-net relay handshakes comes to mind

[matheus23]

We use tokio_util::codec::Framed everywhere else we do something like this. Perhaps we should do so here, too?
Framed implements Sink and Stream both in a cancel-safe manner, according to its docs.

I wonder if we have this bug in other parts of the stack as well, iroh-net relay handshakes comes to mind

Erm. Now I do, too. What exactly are you referring to, though?
We have iroh-net::relay::codec::read_frame, which is only used in recv_client_key, which is (IIUC), never used in a tokio::select!.
Is it something else you're thinking about?

[Frando]

Yeah, using framed makes sense here likely.