fix: clear stale attach FDs before OwnedFd drop - #47
Conversation
| return; | ||
| }; | ||
|
|
||
| let fds = match remote.socket_type { |
There was a problem hiding this comment.
I'm not sure this is the right approach.
First I think that the RemoteSocket should not re-create tagged unions (Rust enums) with the socket_type field to match on, but use the type system (other fields omitted, probably would be a wrapper inner type):
enum RemoteSocket {
Console(Fd),
Terminal(Fd),
...
}That would mean that instead two vectors console_fds and terminal_fds only a single vector can be iterated based on type matching. But that would be still possible now.
However, it looks like the two "forward lists" are needed to work around the borrow checker.
That leads me to that I'm also concerned about mixing OwnedFd and RawFd if it's avoidable and I think that having the OwnedFd wrapped in a refcounted smart pointer like Rc (or Arc<_>) could be the right approach if the lifetime is unclear and/or ownership is complicated - it makes sure that the OwnedFd is not dropped when it's still being used and you can use Weak<_> references that do not prevent collection and their validity is checked at runtime.
The forward lists (if needed) could then be something like Vec<Weak<OwnedFd>> (actually more like Vec<Weak<RemoteSocket>>).
I hope it's at least somewhat clear what I mean (I rewritten it like 10 times while learning more about the design).
I don't still have a whole picture of the full FD lifetimes/ownership and their whole flow. Maybe it would be best if I try to refactor it myself to see what actual problems there are.
There was a problem hiding this comment.
After a discussion, let's address this comment outside the scope of this PR not to block green tests. I'll create an issue for that and take it.
690ed03 to
eb1f63b
Compare
Attach/terminal peers were left in console_fds/terminal_fds after EOF or HUP, so later writev/write could target a recycled FD number. Remove the raw FD from the forwarding lists before dropping the owning Socket. Signed-off-by: Jan Kaluza <jkaluza@redhat.com>
eb1f63b to
b38ed0c
Compare
|
The tests failures are fixes in the two opened PRs. I've just rebased to latest main. Merging, since this has a LGTM. |
2 participants
Attach/terminal peers were left in console_fds/terminal_fds after EOF or HUP, so later writev/write could target a recycled FD number. Remove the raw FD from the forwarding lists before dropping the owning Socket.