Fix getdents64 use-after-free on concurrent close#155
Conversation
| /* Pin the directory stream so a concurrent close()/dup2() cannot free it | ||
| * while this call is still walking it -- see dir_stream_t in fs.c. | ||
| */ | ||
| dir_stream_t *ds = dir_stream_acquire(fd); |
There was a problem hiding this comment.
dir_stream_acquire pins the stream's lifetime but does not serialize use of the shared DIR*. Two guest threads can pin the same wrapper and concurrently run readdir/telldir/seekdir on ds->dir in the walk below. macOS libc locks the DIR internally so this stays memory-safe after the fix, but the logical interleaving of two concurrent getdents64 on the same fd is undefined -- this matches the pre-fix raw-pointer behavior, so it is not a regression. If defined behavior is wanted, add a per-stream lock held across the telldir..seekdir walk.
There was a problem hiding this comment.
Addressed in a900f6b: dir_stream_t now carries a per-stream mutex, taken in sys_getdents64 right after dir_stream_acquire() and held across the whole telldir/readdir/seekdir walk, so two concurrent getdents64 on the same fd are serialized (matching Linux's f_pos_lock semantics) instead of interleaving on the shared DIR*. The lock is only ever taken by reference holders, so the last-reference destroy path can never tear down a held mutex.
| continue; | ||
| } | ||
| fd_table[gfd].dir = dir; | ||
| void *ds = dir_stream_create(dir); |
There was a problem hiding this comment.
On dir_stream_create failure the child closedir()s and continues, leaving fd_table[gfd] published as FD_DIR with dir=NULL. It does not fault -- sys_getdents64 returns ENOTDIR via dir_stream_acquire -- but the inherited directory fd is then silently unusable for the child's lifetime. Consider marking the slot closed (or failing the fd-table restore) so the failure is not masked as a valid-but-broken FD_DIR.
There was a problem hiding this comment.
Addressed in a900f6b: all three restore-failure paths (dup, fdopendir, dir_stream_create) now close(host_fds[i]) and fd_mark_closed(gfd) instead of leaving the slot published as FD_DIR with dir=NULL, so the child sees EBADF on the fd rather than a silent ENOTDIR-forever slot. Chose slot-close over failing the whole fd-table restore since an OOM on one directory stream should not abort the fork.
sys_getdents64 read fd_table[fd].dir with no lock and no lifetime pin, then looped readdir()/telldir()/seekdir() on it. A sibling close() or dup2()-overwrite of the same fd runs fd_cleanup_entry() -> closedir() outside fd_lock and frees the stream while the read loop is still using it -- the same use-after-free class PR sysprog21#146 fixes for FD_EPOLL's epoll_instance_t, left unaddressed for FD_DIR. Wrap the DIR* in a refcounted dir_stream_t (fd-table's own reference plus one per in-flight sys_getdents64, mirroring epoll_instance_t's acquire/release), so a concurrent close/dup2/fork-restore only drops its own reference instead of unconditionally freeing the stream. Per review, the wrapper also carries a per-stream mutex held across the telldir/readdir/seekdir walk: the refcount alone pins lifetime, but two guest threads walking the shared DIR* concurrently would see an undefined entry split, whereas Linux serializes getdents64 through the struct file's f_pos_lock. The lock is only taken by reference holders, so it is never destroyed while held. Also per review, the fork-restore path now closes the guest fd slot when the child cannot rebuild the directory stream (dup, fdopendir, or dir_stream_create failure) instead of leaving a published FD_DIR with dir=NULL. Such a slot never faults, but every getdents64 on it reported ENOTDIR for the child's lifetime, masking the restore failure as a valid-but-broken fd; a closed slot yields an honest EBADF. Reproduced pre-fix under ASAN (double-free in readdir() racing closedir() from a concurrent dup3) and confirmed clean post-fix with tests/test-getdents-refcount.c, added to the unit suite.
24855c4 to
a900f6b
Compare
|
Thank @Max042004 for contributing! |
sys_getdents64 read fd_table[fd].dir with no lock and no lifetime pin,
then looped readdir()/telldir()/seekdir() on it. A sibling close() or
dup2()-overwrite of the same fd runs fd_cleanup_entry() -> closedir()
outside fd_lock and frees the stream while the read loop is still
using it -- the same use-after-free class PR #146 fixes for
FD_EPOLL's epoll_instance_t, left unaddressed for FD_DIR.
Wrap the DIR* in a refcounted dir_stream_t (fd-table's own reference
plus one per in-flight sys_getdents64, mirroring epoll_instance_t's
acquire/release), so a concurrent close/dup2/fork-restore only drops
its own reference instead of unconditionally freeing the stream.
Reproduced pre-fix under ASAN (double-free in readdir() racing
closedir() from a concurrent dup3) and confirmed clean post-fix with
tests/test-getdents-refcount.c, added to the unit suite.
Summary by cubic
Fixes a getdents64 use-after-free and concurrent-walk races by refcounting and serializing directory streams. Closing, dup2/dup3, or fork-restore no longer frees a stream in use; failed restores now close the fd and return EBADF.
DIR*in refcounteddir_stream_t;fd_table[].dirstores the wrapper and frees on last ref.sys_getdents64now pins the stream and holds a per-stream mutex across the readdir/telldir/seekdir walk.fd_cleanup_entrycallsdir_stream_release; dup/aliasing clones the wrapper; on restore failure (dup/fdopendir/create), the child closes the slot instead of publishingFD_DIRwithdir=NULL.tests/test-getdents-refcount.c(intests/manifest.txt) that reproduced the race pre-fix and now passes.Written for commit a900f6b. Summary will update on new commits.