panic: unexpected error during closedir "Bad file descriptor" for unix::fs::Dir

[ConradIrwin]

At Zed we see 2-5 panics per week across our userbase from dropping a Dir. It is possible that this issue is macOS specific (as all of our telemetry is from macOS today), but we are unfortunately not able to reproduce this panic internally.

The problem for us is that this assertion sometimes fails with unexpected error during closedir: Os { code: 9, kind: Uncategorized, message: "Bad file descriptor" }.

I expect that the standard library should not panic on situations that do happen in reality. In particular if you are trying to close a bad file descriptor, that seems "fine". For comparison a failure to close a file during the drop of a file descriptor is silently ignored as I would expect.

Not panicking is obviously not ideal; but in the context of a long-running interactive app like a text editor, we want to be able to take the trade off of higher uptime over lower visibility.

For more discussion of possible solutions to fallible drops, #98338 has some good ideas, and I think if any are implemented we should do an equivalent thing for directories too (I particularly like the idea of a manual close that returns a result, and which causes the Drop to become a no-op). In the meantime, I think we should make rust programs crash less by ignoring this failure.

Meta

rustc --version --verbose:

rustc 1.77.2 (25ef9e3d8 2024-04-09)
binary: rustc
commit-hash: 25ef9e3d85d934b27d9dada2f9dd52b1dc63bb04
commit-date: 2024-04-09
host: aarch64-apple-darwin
release: 1.77.2
LLVM version: 17.0.6

(Though this was happening in earlier versions of rust too)

[workingjubilee]

@ConradIrwin Can you discuss the tracebacks for this, or does your telemetry lack sufficiently sophisticated crash reporting to make that possible?

[workingjubilee]

I expect that the standard library should not panic on situations that do happen in reality.

I know you meant something slightly aside from the reading of this, but I do expect the standard library to panic on indexing a slice with its length, or greater than. And people write code like that every day. So clearly the criteria has to be something more like

• whether it is reasonable to proceed anyways (arguable?)
• whether it is fixable (arguably not, if we get an error as joyously "descriptive" as EBADF)

[ConradIrwin]

Fair: I do expect panics on programmer error or in a situation where it's unsafe to continue. I am relatively (but it's obviously hard to be 100%) sure this is not programmer error, and I'm convinced that it'd be just fine if the program kept running (c.f. how this error is ignored when dropping a file descriptor).

The full back-trace is below. All line numbers in Zed code are relative to v0.131.6 and in rust 1.77.1. I'm not sure why the top line number is missed by symbolicate, but the panic message narrows it down to this assertion

I believe the reason this panics at on line 4018 of worktree.rs in Zed's code is that child_paths goes out of scope, which was retaining a ReadDir returned from std::fs::read_dir() (via smol) here. I can't see any shenanigans on this codepath that would lead us to to invalidate the file descriptor, which leads me to believe that "this just happens sometimes" when interacting with the filesystem. (Though we are holding this across async/await, so it seems possible that something else is affecting the system's state, but it's very unclear as to what would cause this kind of a problem).

Panic `unexpected error during closedir: Os { code: 9, kind: Uncategorized, message: "Bad file descriptor" }` on thread 38 (com.apple.root.user-initiated-qos)
/rustc/7cf61ebde7b22796c69757901dd346d0fe70bd97/library/std/src/sys/pal/unix/fs.rs:0:	_$LT$std..sys..pal..unix..fs..Dir$u20$as$u20$core..ops..drop..Drop$GT$::drop::h84b244fb776f88f3
/rustc/7cf61ebde7b22796c69757901dd346d0fe70bd97/library/core/src/ptr/mod.rs:507:	core::ptr::drop_in_place::<std::path::PathBuf>
/rustc/7cf61ebde7b22796c69757901dd346d0fe70bd97/library/core/src/ptr/mod.rs:507:	core::ptr::drop_in_place::<std::sys::pal::unix::fs::InnerReadDir>
/rustc/7cf61ebde7b22796c69757901dd346d0fe70bd97/library/alloc/src/sync.rs:1756:	<alloc::sync::Arc<std::sys::pal::unix::fs::InnerReadDir>>::drop_slow
/rustc/7cf61ebde7b22796c69757901dd346d0fe70bd97/library/core/src/mem/mod.rs:394:	core::mem::size_of_val_raw::<dyn futures_core::stream::Stream<Item = core::result::Result<std::path::PathBuf, anyhow::Error>> + core::marker::Send>
/rustc/7cf61ebde7b22796c69757901dd346d0fe70bd97/library/core/src/alloc/layout.rs:201:	<core::alloc::layout::Layout>::for_value_raw::<dyn futures_core::stream::Stream<Item = core::result::Result<std::path::PathBuf, anyhow::Error>> + core::marker::Send>
/rustc/7cf61ebde7b22796c69757901dd346d0fe70bd97/library/alloc/src/boxed.rs:1241:	<alloc::boxed::Box<dyn futures_core::stream::Stream<Item = core::result::Result<std::path::PathBuf, anyhow::Error>> + core::marker::Send> as core::ops::drop::Drop>::drop
/rustc/7cf61ebde7b22796c69757901dd346d0fe70bd97/library/core/src/ptr/mod.rs:507:	core::ptr::drop_in_place::<alloc::boxed::Box<dyn futures_core::stream::Stream<Item = core::result::Result<std::path::PathBuf, anyhow::Error>> + core::marker::Send>>
/rustc/7cf61ebde7b22796c69757901dd346d0fe70bd97/library/core/src/ptr/mod.rs:507:	core::ptr::drop_in_place::<core::pin::Pin<alloc::boxed::Box<dyn futures_core::stream::Stream<Item = core::result::Result<std::path::PathBuf, anyhow::Error>> + core::marker::Send>>>
crates/worktree/src/worktree.rs:4018:	<worktree::BackgroundScanner>::scan_dir::{closure#0}
crates/worktree/src/worktree.rs:3754:	<worktree::BackgroundScanner>::scan_dirs::{closure#0}::{closure#0}::{closure#0}
crates/gpui/src/executor.rs:457:	<gpui::executor::Scope>::spawn::<<worktree::BackgroundScanner>::scan_dirs::{closure#0}::{closure#0}::{closure#0}>::{closure#0}
/rustc/7cf61ebde7b22796c69757901dd346d0fe70bd97/library/core/src/task/poll.rs:51:	<core::task::poll::Poll<()>>::map::<core::result::Result<(), alloc::boxed::Box<dyn core::any::Any + core::marker::Send>>, core::result::Result<(), alloc::boxed::Box<dyn core::any::Any + core::marker::Send>>::Ok>
/Users/administrator/.cargo/registry/src/index.crates.io-6f17d22bba15001f/async-task-4.7.0/src/raw.rs:557:	<async_task::raw::RawTask<core::pin::Pin<alloc::boxed::Box<dyn core::future::future::Future<Output = ()> + core::marker::Send>>, (), <gpui::executor::BackgroundExecutor>::spawn_internal<()>::{closure#0}, ()>>::run
libdispatch.dylib	_dispatch_client_callout
libdispatch.dylib	_dispatch_root_queue_drain
libdispatch.dylib	_dispatch_worker_thread2
libsystem_pthread.dylib	_pthread_wqthread
libsystem_pthread.dylib	start_wqthread

[workingjubilee]

Thank you for sharing!

I am not necessarily opposed to this change, I just felt it would be useful to know a bit more context.

Hmm. I wonder if, in another thread that is not bound by the executor, but does wait until after the worktree code has started up, destroying one of the directories in question would do it? That would make sense, and would explain why we get such a lovely "error" from the OS. That would be a classic case of "force majeure" that we typically do want to ignore, yes.

[the8472]

For comparison a failure to close a file during the drop of a file descriptor is silently ignored as I would expect.

This is different because closing regular files can emit IO errors for which there is no good recovery strategy because it is unclear after calling close whether the FD is already closed successfully or not. The recommended strategy for error handling in those cases is to call sync_all on files first before closing them.

EBADF is very different. It indicates that something closed a file descriptor it did not own, which is a violation of IO safety. Unless you have some evidence that this is just macos weirdness I would expect this to indicate a very serious bug in userspace which can lead to data corruption and UB due to file descriptor confusion.

This error should not be suppressed.

Arguably we should also not silently ignore EBADF for OwnedFd and only suppress other errors such as EIO.

And maybe we should add a fn close(self) -> Result<()> API to allow non-panicking error handling, though even then EBADF shouldn't be silently ignored and the application should be aborted to prevent data from being accidentally clobbered.

[the8472]

Caveat: closedir operates on a DIR*, not on a fd directly. So it's possible that the C library returns EBADF for something that's not an invalid file descriptor but some other invalid state in that wrapping struct.
Is the macos C library open source? Then we can check there. If not it'd require reproducing the issue and tracing syscalls to tell the difference.

[joboet]

It is, though as with everything Apple, it's a mess.

Errors can only occur from the close call, the rest of closedir just does some reclaiming. Most of the kernel is open-source as well, so one could try to figure out if anything except the fd lookup could cause the EBADF, but I'm not motivated enough to do that.

[the8472]

Thanks for looking it up. I think that is enough evidence that there's probably some library-UB going on in zed and we shouldn't try to silence it.

[joboet]

The only place in zed itself which calls as_raw_fd is this code for getting the PID of the child process running in the integrated terminal. While it's definitely very fishy (it's not tied to the lifetime of the PTY), it doesn't close anything so I doubt that this issue is caused there.

As for dependencies, it might help to get a complete backtrace to see which ReadDir::drop specifically is causing this.

[the8472]

It's not necessarily readdir that's causing this, it could be the victim of something else going rogue and closing the wrong fd numbers.

[joboet]

It must be... Though I'm hoping that there is enough locality between the close and the read_dir to point us in the direction.