Skip to content

Commit 31fd98c

Browse files
Merge pull request from GHSA-c827-hfw6-qwvm
* Fix `rustix::fs::Dir` to avoid unbounded buffer growth. Fix `Dir`'s buffer size computation to avoid resizing past a fixed upper limit. This prevents it from growing without bound, such as in the case of `Dir::rewind` for repeated iterations with the same `Dir`. * Don't let `Dir` continue to try to iterate after a failure. * Handle `io::Errno::INTR` gracefully. * Write a more detailed comment on the buffer growth policy. * Also mention that no buffer can ever be big enough for everything. * Add tests against over-allocation & stuck iterator * Rm `dir_iterator_does_not_overallocate` unit test in favour of docs * Extend `test_dir` to cover `rewind`. * Consistently handle directory removal as ending the stream. libc implementations of directory iteration handle directory removal by just ending the stream. In the linux_raw backend, this looks like `ENOENT` from `getdents64`, so change the code to check for `ENOENT` and end the stream. This requires changing the `dir_iterator_does_not_get_stuck_on_io_error` test to no longer expect a failure, so it's now renamed to `dir_iterator_handles_dir_removal`. To test the error case, add a new `dir_iterator_handles_io_errors` test which uses `dup2` to induce an error, in both the linux_raw and libc backends. This exposes the fact that the libc `Dir` implementation was also assuming that users would stop iterating after hitting a failure, so add a `any_errors` flag to the libc backend as well. * Add a test for removing the directory after doing `read_from`. * In the libc backend, handle `ENOENT` when opening ".". --------- Co-authored-by: cyqsimon <[email protected]>
1 parent 4bb2f4b commit 31fd98c

File tree

3 files changed

+224
-26
lines changed

3 files changed

+224
-26
lines changed

src/backend/libc/fs/dir.rs

Lines changed: 74 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -30,8 +30,13 @@ use core::ptr::NonNull;
3030
use libc_errno::{errno, set_errno, Errno};
3131

3232
/// `DIR*`
33-
#[repr(transparent)]
34-
pub struct Dir(NonNull<c::DIR>);
33+
pub struct Dir {
34+
/// The `libc` `DIR` pointer.
35+
libc_dir: NonNull<c::DIR>,
36+
37+
/// Have we seen any errors in this iteration?
38+
any_errors: bool,
39+
}
3540

3641
impl Dir {
3742
/// Construct a `Dir` that reads entries from the given directory
@@ -43,20 +48,35 @@ impl Dir {
4348

4449
#[inline]
4550
fn _read_from(fd: BorrowedFd<'_>) -> io::Result<Self> {
51+
let mut any_errors = false;
52+
4653
// Given an arbitrary `OwnedFd`, it's impossible to know whether the
4754
// user holds a `dup`'d copy which could continue to modify the
4855
// file description state, which would cause Undefined Behavior after
4956
// our call to `fdopendir`. To prevent this, we obtain an independent
5057
// `OwnedFd`.
5158
let flags = fcntl_getfl(fd)?;
52-
let fd_for_dir = openat(fd, cstr!("."), flags | OFlags::CLOEXEC, Mode::empty())?;
59+
let fd_for_dir = match openat(fd, cstr!("."), flags | OFlags::CLOEXEC, Mode::empty()) {
60+
Ok(fd) => fd,
61+
Err(io::Errno::NOENT) => {
62+
// If "." doesn't exist, it means the directory was removed.
63+
// We treat that as iterating through a directory with no
64+
// entries.
65+
any_errors = true;
66+
crate::io::dup(fd)?
67+
}
68+
Err(err) => return Err(err),
69+
};
5370

5471
let raw = owned_fd(fd_for_dir);
5572
unsafe {
5673
let libc_dir = c::fdopendir(raw);
5774

5875
if let Some(libc_dir) = NonNull::new(libc_dir) {
59-
Ok(Self(libc_dir))
76+
Ok(Self {
77+
libc_dir,
78+
any_errors,
79+
})
6080
} else {
6181
let err = io::Errno::last_os_error();
6282
let _ = c::close(raw);
@@ -68,20 +88,27 @@ impl Dir {
6888
/// `rewinddir(self)`
6989
#[inline]
7090
pub fn rewind(&mut self) {
71-
unsafe { c::rewinddir(self.0.as_ptr()) }
91+
self.any_errors = false;
92+
unsafe { c::rewinddir(self.libc_dir.as_ptr()) }
7293
}
7394

7495
/// `readdir(self)`, where `None` means the end of the directory.
7596
pub fn read(&mut self) -> Option<io::Result<DirEntry>> {
97+
// If we've seen errors, don't continue to try to read anyting further.
98+
if self.any_errors {
99+
return None;
100+
}
101+
76102
set_errno(Errno(0));
77-
let dirent_ptr = unsafe { libc_readdir(self.0.as_ptr()) };
103+
let dirent_ptr = unsafe { libc_readdir(self.libc_dir.as_ptr()) };
78104
if dirent_ptr.is_null() {
79105
let curr_errno = errno().0;
80106
if curr_errno == 0 {
81107
// We successfully reached the end of the stream.
82108
None
83109
} else {
84110
// `errno` is unknown or non-zero, so an error occurred.
111+
self.any_errors = true;
85112
Some(Err(io::Errno(curr_errno)))
86113
}
87114
} else {
@@ -120,7 +147,7 @@ impl Dir {
120147
/// `fstat(self)`
121148
#[inline]
122149
pub fn stat(&self) -> io::Result<Stat> {
123-
fstat(unsafe { BorrowedFd::borrow_raw(c::dirfd(self.0.as_ptr())) })
150+
fstat(unsafe { BorrowedFd::borrow_raw(c::dirfd(self.libc_dir.as_ptr())) })
124151
}
125152

126153
/// `fstatfs(self)`
@@ -134,14 +161,14 @@ impl Dir {
134161
)))]
135162
#[inline]
136163
pub fn statfs(&self) -> io::Result<StatFs> {
137-
fstatfs(unsafe { BorrowedFd::borrow_raw(c::dirfd(self.0.as_ptr())) })
164+
fstatfs(unsafe { BorrowedFd::borrow_raw(c::dirfd(self.libc_dir.as_ptr())) })
138165
}
139166

140167
/// `fstatvfs(self)`
141168
#[cfg(not(any(solarish, target_os = "haiku", target_os = "redox", target_os = "wasi")))]
142169
#[inline]
143170
pub fn statvfs(&self) -> io::Result<StatVfs> {
144-
fstatvfs(unsafe { BorrowedFd::borrow_raw(c::dirfd(self.0.as_ptr())) })
171+
fstatvfs(unsafe { BorrowedFd::borrow_raw(c::dirfd(self.libc_dir.as_ptr())) })
145172
}
146173

147174
/// `fchdir(self)`
@@ -150,7 +177,7 @@ impl Dir {
150177
#[cfg_attr(doc_cfg, doc(cfg(feature = "process")))]
151178
#[inline]
152179
pub fn chdir(&self) -> io::Result<()> {
153-
fchdir(unsafe { BorrowedFd::borrow_raw(c::dirfd(self.0.as_ptr())) })
180+
fchdir(unsafe { BorrowedFd::borrow_raw(c::dirfd(self.libc_dir.as_ptr())) })
154181
}
155182
}
156183

@@ -163,7 +190,7 @@ unsafe impl Send for Dir {}
163190
impl Drop for Dir {
164191
#[inline]
165192
fn drop(&mut self) {
166-
unsafe { c::closedir(self.0.as_ptr()) };
193+
unsafe { c::closedir(self.libc_dir.as_ptr()) };
167194
}
168195
}
169196

@@ -179,7 +206,7 @@ impl Iterator for Dir {
179206
impl fmt::Debug for Dir {
180207
fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
181208
f.debug_struct("Dir")
182-
.field("fd", unsafe { &c::dirfd(self.0.as_ptr()) })
209+
.field("fd", unsafe { &c::dirfd(self.libc_dir.as_ptr()) })
183210
.finish()
184211
}
185212
}
@@ -293,3 +320,38 @@ fn check_dirent_layout(dirent: &c::dirent) {
293320
}
294321
);
295322
}
323+
324+
#[test]
325+
fn dir_iterator_handles_io_errors() {
326+
// create a dir, keep the FD, then delete the dir
327+
let tmp = tempfile::tempdir().unwrap();
328+
let fd = crate::fs::openat(
329+
crate::fs::CWD,
330+
tmp.path(),
331+
crate::fs::OFlags::RDONLY | crate::fs::OFlags::CLOEXEC,
332+
crate::fs::Mode::empty(),
333+
)
334+
.unwrap();
335+
336+
let file_fd = crate::fs::openat(
337+
&fd,
338+
tmp.path().join("test.txt"),
339+
crate::fs::OFlags::WRONLY | crate::fs::OFlags::CREATE,
340+
crate::fs::Mode::RWXU,
341+
)
342+
.unwrap();
343+
344+
let mut dir = Dir::read_from(&fd).unwrap();
345+
346+
// Reach inside the `Dir` and replace its directory with a file, which
347+
// will cause the subsequent `readdir` to fail.
348+
unsafe {
349+
let raw_fd = c::dirfd(dir.libc_dir.as_ptr());
350+
let mut owned_fd: crate::fd::OwnedFd = crate::fd::FromRawFd::from_raw_fd(raw_fd);
351+
crate::io::dup2(&file_fd, &mut owned_fd).unwrap();
352+
core::mem::forget(owned_fd);
353+
}
354+
355+
assert!(matches!(dir.next(), Some(Err(_))));
356+
assert!(matches!(dir.next(), None));
357+
}

src/backend/linux_raw/fs/dir.rs

Lines changed: 82 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -18,9 +18,17 @@ pub struct Dir {
1818
/// The `OwnedFd` that we read directory entries from.
1919
fd: OwnedFd,
2020

21+
/// Have we seen any errors in this iteration?
22+
any_errors: bool,
23+
24+
/// Should we rewind the stream on the next iteration?
25+
rewind: bool,
26+
27+
/// The buffer for `linux_dirent64` entries.
2128
buf: Vec<u8>,
29+
30+
/// Where we are in the buffer.
2231
pos: usize,
23-
next: Option<u64>,
2432
}
2533

2634
impl Dir {
@@ -38,25 +46,39 @@ impl Dir {
3846

3947
Ok(Self {
4048
fd: fd_for_dir,
49+
any_errors: false,
50+
rewind: false,
4151
buf: Vec::new(),
4252
pos: 0,
43-
next: None,
4453
})
4554
}
4655

4756
/// `rewinddir(self)`
4857
#[inline]
4958
pub fn rewind(&mut self) {
59+
self.any_errors = false;
60+
self.rewind = true;
5061
self.pos = self.buf.len();
51-
self.next = Some(0);
5262
}
5363

5464
/// `readdir(self)`, where `None` means the end of the directory.
5565
pub fn read(&mut self) -> Option<io::Result<DirEntry>> {
56-
if let Some(next) = self.next.take() {
57-
match crate::backend::fs::syscalls::_seek(self.fd.as_fd(), next as i64, SEEK_SET) {
66+
// If we've seen errors, don't continue to try to read anyting further.
67+
if self.any_errors {
68+
return None;
69+
}
70+
71+
// If a rewind was requested, seek to the beginning.
72+
if self.rewind {
73+
self.rewind = false;
74+
match io::retry_on_intr(|| {
75+
crate::backend::fs::syscalls::_seek(self.fd.as_fd(), 0, SEEK_SET)
76+
}) {
5877
Ok(_) => (),
59-
Err(err) => return Some(Err(err)),
78+
Err(err) => {
79+
self.any_errors = true;
80+
return Some(Err(err));
81+
}
6082
}
6183
}
6284

@@ -78,7 +100,7 @@ impl Dir {
78100
if self.buf.len() - self.pos < size_of::<linux_dirent64>() {
79101
match self.read_more()? {
80102
Ok(()) => (),
81-
Err(e) => return Some(Err(e)),
103+
Err(err) => return Some(Err(err)),
82104
}
83105
}
84106

@@ -136,14 +158,31 @@ impl Dir {
136158
}
137159

138160
fn read_more(&mut self) -> Option<io::Result<()>> {
139-
let og_len = self.buf.len();
140-
// Capacity increment currently chosen by wild guess.
141-
self.buf
142-
.resize(self.buf.capacity() + 32 * size_of::<linux_dirent64>(), 0);
143-
let nread = match crate::backend::fs::syscalls::getdents(self.fd.as_fd(), &mut self.buf) {
161+
// The first few times we're called, we allocate a relatively small
162+
// buffer, because many directories are small. If we're called more,
163+
// use progressively larger allocations, up to a fixed maximum.
164+
//
165+
// The specific sizes and policy here have not been tuned in detail yet
166+
// and may need to be adjusted. In doing so, we should be careful to
167+
// avoid unbounded buffer growth. This buffer only exists to share the
168+
// cost of a `getdents` call over many entries, so if it gets too big,
169+
// cache and heap usage will outweigh the benefit. And ultimately,
170+
// directories can contain more entries than we can allocate contiguous
171+
// memory for, so we'll always need to cap the size at some point.
172+
if self.buf.len() < 1024 * size_of::<linux_dirent64>() {
173+
self.buf.reserve(32 * size_of::<linux_dirent64>());
174+
}
175+
self.buf.resize(self.buf.capacity(), 0);
176+
let nread = match io::retry_on_intr(|| {
177+
crate::backend::fs::syscalls::getdents(self.fd.as_fd(), &mut self.buf)
178+
}) {
144179
Ok(nread) => nread,
180+
Err(io::Errno::NOENT) => {
181+
self.any_errors = true;
182+
return None;
183+
}
145184
Err(err) => {
146-
self.buf.resize(og_len, 0);
185+
self.any_errors = true;
147186
return Some(Err(err));
148187
}
149188
};
@@ -225,3 +264,33 @@ impl DirEntry {
225264
self.d_ino
226265
}
227266
}
267+
268+
#[test]
269+
fn dir_iterator_handles_io_errors() {
270+
// create a dir, keep the FD, then delete the dir
271+
let tmp = tempfile::tempdir().unwrap();
272+
let fd = crate::fs::openat(
273+
crate::fs::CWD,
274+
tmp.path(),
275+
crate::fs::OFlags::RDONLY | crate::fs::OFlags::CLOEXEC,
276+
crate::fs::Mode::empty(),
277+
)
278+
.unwrap();
279+
280+
let file_fd = crate::fs::openat(
281+
&fd,
282+
tmp.path().join("test.txt"),
283+
crate::fs::OFlags::WRONLY | crate::fs::OFlags::CREATE,
284+
crate::fs::Mode::RWXU,
285+
)
286+
.unwrap();
287+
288+
let mut dir = Dir::read_from(&fd).unwrap();
289+
290+
// Reach inside the `Dir` and replace its directory with a file, which
291+
// will cause the subsequent `getdents64` to fail.
292+
crate::io::dup2(&file_fd, &mut dir.fd).unwrap();
293+
294+
assert!(matches!(dir.next(), Some(Err(_))));
295+
assert!(matches!(dir.next(), None));
296+
}

0 commit comments

Comments
 (0)