rust icon indicating copy to clipboard operation
rust copied to clipboard

Do not invoke accept() on an already-established connection

Open ppentchev opened this issue 1 year ago • 1 comments

Hi,

First of all, thanks a lot for writing and maintaining the Rust reference implementation of varlink!

Here's a proposed fix for #73; the Listener::new() method already records the fact that this listener is actually a connection that has already been established via socket activation, and lets the Listener::accept() method honor that flag.

I have written a trivial varlink server - my varlink-hello GitLab project - to demonstrate the need for this change. Without it, with the stock varlink 11.0.1 crate, the varlink-hello program fails, since Listener::accept() attempts to invoke the accept() method of the "Unix listener" which is actually an already-connected socket.

Now, there are three things I don't completely like about this patch:

  • I'm not overly fond of the use of unsafe { ... } in Listener::accept(). A cleaner way to do that would be to add two new values to the Listener enum, e.g. TCPAccepted and UNIXAccepted, and make Listener::new() create a TcpListener or a UnixListener directly, in its own unsafe { ... } blocks. This might cause a bit of code churn; let me know if I should do it.
  • I currently have no way of testing the Windows implementation; I'd be very grateful if somebody could try to build the varlink-hello program under Windows and see if a) it fails with the currently-released varlink crate, and b) this change fixes it.
  • I am not entirely sure how to write tests for this change. An obvious way would be to copy the test_listen() function in varlink/src/tests.rs, create a socketpair, and go for it, but the problem is that to really test it, the LISTEN_FDS, LISTEN_FDNAMES, and LISTEN_PID environment variables need to be set, which might influence other tests run either at the same time or later. I'd be grateful for any ideas on how to approach this; writing a new binary crate that will only be used for testing almost seems like a viable approach, even though Cargo currently does not support test-only binary crates.

Thanks in advance for your time, and keep up the great work!

G'luck, Peter

ppentchev avatar Oct 27 '24 21:10 ppentchev

JFTR, I edited the initial comment to add the third thing I'm not completely sure about.

ppentchev avatar Oct 27 '24 21:10 ppentchev