Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

What should happen when an FD is closed while a thread is blocked on it? #4051

Open
RalfJung opened this issue Nov 22, 2024 · 22 comments
Open

Comments

@RalfJung
Copy link
Member

For both epoll and eventfd, the question arises what happens when an FD is closed while a thread is blocked on it. Currently, we throw an "unsupported" error. That's not a great solution -- we should figure out what POSIX guarantees in that case, and implement that.

So, what does POSIX say?

Cc @tiif

@oli-obk
Copy link
Contributor

oli-obk commented Nov 22, 2024

POSIX says nothing on this. In theory it counts as a race condition, as you can't ever say another thread has blocked on this FD already (there's no general way to ensure that has happened yet, it may be right before the thread actually executes the blocking op).

In practice, once actually blocked, the thread just never wakes.

So... beyond going to reporting a race condition error instead of an unsupported error, I see no good course of action. We picked unsupported as it's unspecified what the behaviour is, and in practice it's definitely not "return the same result as if the FD were closed before the read/write call was done"

@RalfJung
Copy link
Member Author

you can't ever say another thread has blocked on this FD already (there's no general way to ensure that has happened yet, it may be right before the thread actually executes the blocking op).

I can't parse this, what do you mean?

reporting a race condition error

That would mean making it UB. The question is, is it UB?

@RalfJung
Copy link
Member Author

@the8472 do you know if there is any required behavior from POSIX here?

"Never wake up the thread" does seem like a reasonable implementation we could copy. Maybe we should issue some sort of warning though? We don't have much in the way of warnings in Miri currently, but after all this is almost certainly a bug.

@oli-obk
Copy link
Contributor

oli-obk commented Nov 22, 2024

I can't parse this, what do you mean?

When thread A is running on a code path that will inevitably block on FD, and thread B is closing FD, we are in an unclear state wrt what happens to thread A. Depending on the scheduler it

  • is already blocked
  • it will invoke the blocking op after thread B closes FD
  • it's already accessing FD internals while B is closing FD

It's unspecified if the third option can happen.

The first option leads to the thread staying blocked forever in practice (this may also depend on the type of FD, pipes seem in theory more likely to unblock than e.g. a file)

The second option is well specified

@oli-obk
Copy link
Contributor

oli-obk commented Nov 22, 2024

I decided we should pick unsupported as this is always a bug according to all stackoverflow and other forum answers I could find. These often state that it's a race condition, but without any references. On a more high level, a FD should only be closed if there are no outstanding uses of it. The docs of close say the word "race condition"

   Multithreaded processes and close()
   It is probably unwise to close file descriptors while they may be
   in use by system calls in other threads in the same process.
   Since a file descriptor may be reused, there are some obscure
   race conditions that may cause unintended side effects.

   Furthermore, consider the following scenario where two threads
   are performing operations on the same file descriptor:

   (1)  One thread is blocked in an I/O system call on the file
        descriptor.  For example, it is trying to write(2) to a pipe
        that is already full, or trying to read(2) from a stream
        socket which currently has no available data.

   (2)  Another thread closes the file descriptor.

   The behavior in this situation varies across systems.  On some
   systems, when the file descriptor is closed, the blocking system
   call returns immediately with an error.

   On Linux (and possibly some other systems), the behavior is
   different: the blocking I/O system call holds a reference to the
   underlying open file description, and this reference keeps the
   description open until the I/O system call completes.  (See
   open(2) for a discussion of open file descriptions.)  Thus, the
   blocking system call in the first thread may successfully
   complete after the close() in the second thread.

https://www.man7.org/linux/man-pages/man2/close.2.html

@RalfJung
Copy link
Member Author

RalfJung commented Nov 22, 2024 via email

@bjorn3
Copy link
Member

bjorn3 commented Nov 22, 2024

The epoll man page says the following:

Will closing a file descriptor cause it to be removed from all epoll interest lists?

Yes, but be aware of the following point. A file descriptor is a reference to an open file description (see open(2)). Whenever a file descriptor is duplicated via dup(2), dup2(2), fcntl(2)
F_DUPFD, or fork(2), a new file descriptor referring to the same open file description is created. An open file description continues to exist until all file descriptors referring to it have
been closed.

A file descriptor is removed from an interest list only after all the file descriptors referring to the underlying open file description have been closed. This means that even after a file
descriptor that is part of an interest list has been closed, events may be reported for that file descriptor if other file descriptors referring to the same underlying file description remain open.
To prevent this happening, the file descriptor must be explicitly removed from the interest list (using epoll_ctl(2) EPOLL_CTL_DEL) before it is duplicated. Alternatively, the application must
ensure that all file descriptors are closed (which may be difficult if file descriptors were duplicated behind the scenes by library functions that used dup(2) or fork(2)).

In other words the fd will be removed from the interest list if there is no dup of the fd still around. If it is the last fd in the interest list, epoll_wait will block until another thread inserts a new fd in the interest list and this new fd triggers an event I think, just like when calling epoll_wait when the interest list was empty from the start.

@RalfJung
Copy link
Member Author

The epoll case I was talking about above is about what happens when the epoll FD itself is closed while someone blocks on it.

What happens when an FD in the interest list is closed, is a separate question, but I think that is already handled properly. epoll identifies interest by the pair of FD number and underlying file description object; if the object is closed then even if the number gets reused the interest will never be triggered.

@RalfJung
Copy link
Member Author

@tiif do we have a test for this situation of "the FD registered with epoll got closed but there's a duplicate FD for the same file description so events are still reported"? I guess that means events are reported with the old FD number, which is actually quite confusing since it won't refer to the relevant object any more...

@the8472
Copy link
Member

the8472 commented Nov 22, 2024

@the8472 do you know if there is any required behavior from POSIX here?

epoll is a linux-specific api. poll is the posix standard one, but I haven't seen any explicit documentation whether a wakeup will be triggered by a concurrent close or not.

The epoll case I was talking about above is about what happens when the epoll FD itself is closed while someone blocks on it.

File descriptions are refcounted resources (on linux). So I assume that closing the file descriptor will only decrease the count by one but the ongoing polling syscall will own another token and so the underlying description will remain open at least until the syscall returns.

Since epollfd is linux-specific the portability of those semantics shouldn't be relevant here.

@oli-obk
Copy link
Contributor

oli-obk commented Nov 22, 2024

I really don't want to replicate per-FD-kind behaviour on close while blocked, with platform dependent shenanigans. It's why I opted for the unsupported, since we do not support the (potentially guaranteed) platform or FD specific behaviour. The situations are always an edge case that you probably don't want to encounter anyway. We could add a new Undesirable error to clearly signal that difference to regular unsupported which usually mean we invite adding an impl for. A race condition error signals too much that this is UB, when it may be defined for some platforms and some FDs

@RalfJung
Copy link
Member Author

I don't think we should have per-FD-type logic, either.

We could raise a "deadlock" error, since it does mean that this thread will definitely never be woken up.

So I assume that closing the file descriptor will only decrease the count by one but the ongoing polling syscall will own another token and so the underlying description will remain open at least until the syscall returns.

So that would mean even if the epoll FD is closed, future wakeups on threads waiting on that FD still happen, since the underlying data is still present. Makes sense. We should probably implement that, then.

@tiif
Copy link
Contributor

tiif commented Nov 22, 2024

do we have a test for this situation of "the FD registered with epoll got closed but there's a duplicate FD for the same file description so events are still reported"?

yes

// When a certain file descriptor registered with epoll is closed, but the underlying file description
// is not closed, notification should still be provided.
//
// This is a quirk of epoll being described in https://man7.org/linux/man-pages/man7/epoll.7.html
// A file descriptor is removed from an interest list only after all the file descriptors
// referring to the underlying open file description have been closed.
fn test_not_fully_closed_fd() {
// Create an epoll instance.
let epfd = unsafe { libc::epoll_create1(0) };
assert_ne!(epfd, -1);
// Create an eventfd instance.
let flags = libc::EFD_NONBLOCK | libc::EFD_CLOEXEC;
let fd = unsafe { libc::eventfd(0, flags) };
// Dup the fd.
let newfd = unsafe { libc::dup(fd) };
assert_ne!(newfd, -1);
// Register eventfd with EPOLLIN | EPOLLOUT | EPOLLET
let mut ev = libc::epoll_event { events: EPOLL_IN_OUT_ET, u64: u64::try_from(fd).unwrap() };
let res = unsafe { libc::epoll_ctl(epfd, libc::EPOLL_CTL_ADD, fd, &mut ev) };
assert_eq!(res, 0);
// Close the original fd that being used to register with epoll.
let res = unsafe { libc::close(fd) };
assert_eq!(res, 0);
// Notification should still be provided because the file description is not closed.
let expected_event = u32::try_from(libc::EPOLLOUT).unwrap();
let expected_value = fd as u64;
check_epoll_wait::<1>(epfd, &[(expected_event, expected_value)]);
// Write to the eventfd instance to produce notification.
let sized_8_data: [u8; 8] = 1_u64.to_ne_bytes();
let res = unsafe { libc::write(newfd, sized_8_data.as_ptr() as *const libc::c_void, 8) };
assert_eq!(res, 8);
// Close the dupped fd.
let res = unsafe { libc::close(newfd) };
assert_eq!(res, 0);
// No notification should be provided.
check_epoll_wait::<1>(epfd, &[]);
}

@tiif
Copy link
Contributor

tiif commented Nov 22, 2024

So that would mean even if the epoll FD is closed, future wakeups on threads waiting on that FD still happen, since the underlying data is still present. Makes sense. We should probably implement that, then.

If we plan to support "closing fd while blocking on it" operation for epoll, should we do the same for socketpair/pipe and eventfd?

@RalfJung
Copy link
Member Author

Those are different situations though -- one is blocking on read/write, the other is blocking on a dedicated operation (epoll_wait) that merely uses the FD for bookkeeping. epoll_wait does not get woken up by actions on its own FD, it gets woken up by actions on other FDs.

@tiif
Copy link
Contributor

tiif commented Nov 22, 2024

epoll_wait does not get woken up by actions on its own FD, it gets woken up by actions on other FDs.

I expect it'd be similar for socketpair/pipe too. If the thread blocked because it tried to read from an empty socketpair/pipe, writing to it's peer fd would unblock it.

@RalfJung
Copy link
Member Author

RalfJung commented Nov 22, 2024 via email

@tiif
Copy link
Contributor

tiif commented Nov 23, 2024

The blocked fd could be closed too, the case that I am thinking of is:

  1. thread1 blocks on read for one of the socketpair fd, fd1
  2. thread2 tries to close fd1
  3. thread3 writes to the peer fd of fd1, and this can unblock thread1

which of the following should happen here?

  1. throw error for the close in step2
  2. allow the close and allow thread1 to be unblocked
  3. throw miri error as we don't want to support this behavior

@RalfJung
Copy link
Member Author

RalfJung commented Nov 23, 2024 via email

@tiif
Copy link
Contributor

tiif commented Nov 23, 2024

Sure, this makes sense. It would be nice if we could throw a more specific error than deadlock if we encounter this scenario. The reason is if someone manages to make this work on a certain platform in a real system, but it fails in miri with a deadlock error, it'd be helpful to know that it encountered this specific scenario. This way, we can rule out the possibility of it being a miri implementation bug early on.

@RalfJung
Copy link
Member Author

RalfJung commented Nov 23, 2024 via email

@tiif
Copy link
Contributor

tiif commented Nov 24, 2024

And then for epoll - we should probably make the Weak ref into an Rc so that the unblock callback can definitely access the epoll instance?

Yea, and perhaps we need to close it again to properly destruct it from the global machine state after we finish using it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

5 participants