Do not invoke accept() on an already-established connection#105
Do not invoke accept() on an already-established connection#105ppentchev wants to merge 1 commit intovarlink:masterfrom
Conversation
Listener::new() already records the fact that this listener is actually a connection that has already been established via socket activation. Let Listener::accept() honor that flag. Fixes varlink#73.
|
JFTR, I edited the initial comment to add the third thing I'm not completely sure about. |
| } | ||
| } | ||
|
|
||
| pub const fn is_already_accepted(&self) -> bool { |
There was a problem hiding this comment.
While I know the rest of the code is not consistent about this, having at least a minimal documentation string I think is a good idea for public APIs.
| Ok(Box::new(s)) | ||
| &Listener::TCP(Some(ref l), accepted) => { | ||
| if accepted { | ||
| unsafe { Ok(Box::new(TcpStream::from_raw_fd(l.as_raw_fd()))) } |
There was a problem hiding this comment.
Without digging into this code a lot more, there's a reason those functions are unsafe and that reason is exactly what looks like a bug here: The previous listener still thinks it owns the file descriptor.
We can avoid this by deconstructing into an OwnedFd like Box::new(TcpStream::from(l.into::<OwnedFd>())) or so.
But I think we need to back up on this, I'll comment on the original ticket.
There was a problem hiding this comment.
Pull Request Overview
This PR fixes issue #73 by preventing the accept() method from being called on sockets that are already established connections (e.g., via socket activation). When a Listener is created from an already-connected socket, it should not attempt to call accept() but instead should use the existing connection directly.
Key changes:
- Added
is_already_accepted()method to check if a listener represents an already-connected socket - Modified
accept()methods to handle already-accepted connections by usingfrom_raw_fd()instead of callingaccept() - Updated main listen loop to exit early for already-accepted listeners
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
| if accepted { | ||
| unsafe { Ok(Box::new(TcpStream::from_raw_fd(l.as_raw_fd()))) } | ||
| } else { |
There was a problem hiding this comment.
Using from_raw_fd() transfers ownership of the file descriptor without validation. This could lead to use-after-free or double-close issues if the original listener is used elsewhere. Consider using a safer approach like cloning the file descriptor with dup() first.
| if *accepted { | ||
| unsafe { Ok(Box::new(UnixStream::from_raw_fd(l.as_raw_fd()))) } |
There was a problem hiding this comment.
Inconsistent dereferencing pattern: TCP case uses accepted directly while UNIX case uses *accepted. Both should use the same pattern for consistency.
| if accepted { | ||
| unsafe { Ok(Box::new(TcpStream::from_raw_fd(l.as_raw_fd()))) } | ||
| } else { |
There was a problem hiding this comment.
This code is duplicated from the Windows version above (lines 205-210). Consider extracting this logic into a helper method to reduce code duplication.
| if *accepted { | ||
| unsafe { Ok(Box::new(UnixStream::from_raw_fd(l.as_raw_fd()))) } | ||
| } else { |
There was a problem hiding this comment.
This code is duplicated from both the Windows version and the TCP case above. The same dereferencing inconsistency and duplication issues apply here.
2 participants
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 theListener::accept()method honor that flag.I have written a trivial
varlinkserver - my varlink-hello GitLab project - to demonstrate the need for this change. Without it, with the stockvarlink 11.0.1crate, thevarlink-helloprogram fails, sinceListener::accept()attempts to invoke theaccept()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:
unsafe { ... }inListener::accept(). A cleaner way to do that would be to add two new values to theListenerenum, e.g.TCPAcceptedandUNIXAccepted, and makeListener::new()create aTcpListeneror aUnixListenerdirectly, in its ownunsafe { ... }blocks. This might cause a bit of code churn; let me know if I should do it.varlink-helloprogram under Windows and see if a) it fails with the currently-releasedvarlinkcrate, and b) this change fixes it.test_listen()function invarlink/src/tests.rs, create a socketpair, and go for it, but the problem is that to really test it, theLISTEN_FDS,LISTEN_FDNAMES, andLISTEN_PIDenvironment 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