Skip to content

Commit e4e847f

Browse files
stevenenglerstevenengler
committed
Added DescriptorHandle to restrict fd handle values
The descriptor table shouldn't ever allow descriptors to be registered with a handle larger than `libc::c_int::MAX`. The old code also had a loop that could wrap around unexpectedly in release builds. The public API to `DescriptorTable` now uses `DescriptorHandle` objects. The `register_descriptor` and `register_descriptor_with_min_fd` methods also now return results if the descriptor could not be added.
1 parent 97324b8 commit e4e847f

7 files changed

Lines changed: 189 additions & 56 deletions

File tree

‎src/main/host/descriptor/descriptor_table.rs‎

Lines changed: 132 additions & 22 deletions
Original file line numberDiff line numberDiff line change
@@ -5,9 +5,12 @@ use std::collections::{BTreeSet, HashMap};
55

66
use log::*;
77

8+
/// POSIX requires fds to be assigned as `libc::c_int`, so we can't allow any fds larger than this.
9+
pub const FD_MAX: u32 = i32::MAX as u32;
10+
811
/// Map of file handles to file descriptors. Typically owned by a Process.
912
pub struct DescriptorTable {
10-
descriptors: HashMap<u32, Descriptor>,
13+
descriptors: HashMap<DescriptorHandle, Descriptor>,
1114

1215
// Indices less than `next_index` known to be available.
1316
available_indices: BTreeSet<u32>,
@@ -26,9 +29,14 @@ impl DescriptorTable {
2629
}
2730
}
2831

29-
/// Add the descriptor at an unused index, and return the index.
30-
fn add(&mut self, descriptor: Descriptor, min_index: u32) -> u32 {
31-
let idx = if let Some(idx) = self.available_indices.range(min_index..).next() {
32+
/// Add the descriptor at an unused index, and return the index. If the descriptor could not be
33+
/// added, the descriptor is returned in the `Err`.
34+
fn add(
35+
&mut self,
36+
descriptor: Descriptor,
37+
min_index: DescriptorHandle,
38+
) -> Result<DescriptorHandle, Descriptor> {
39+
let idx = if let Some(idx) = self.available_indices.range(min_index.val()..).next() {
3240
// Un-borrow from `available_indices`.
3341
let idx = *idx;
3442
// Take from `available_indices`
@@ -38,16 +46,31 @@ impl DescriptorTable {
3846
} else {
3947
// Start our search at either the next likely available index or the minimum index,
4048
// whichever is larger.
41-
let mut idx = std::cmp::max(self.next_index, min_index);
49+
let mut idx = std::cmp::max(self.next_index, min_index.val());
50+
51+
// Check if this index out of range.
52+
if idx > FD_MAX {
53+
return Err(descriptor);
54+
}
4255

4356
// Only update next_index if we started at it, otherwise there may be other
4457
// available indexes lower than idx.
4558
let should_update_next_index = idx == self.next_index;
4659

4760
// Skip past any indexes that are in use. This can happen after
4861
// calling `set` with a value greater than `next_index`.
49-
while self.descriptors.contains_key(&idx) {
62+
while self
63+
.descriptors
64+
.contains_key(&DescriptorHandle::new(idx).unwrap())
65+
{
5066
trace!("Skipping past in-use index {}", idx);
67+
68+
// Check if the next index is out of range.
69+
if idx >= FD_MAX {
70+
return Err(descriptor);
71+
}
72+
73+
// Won't overflow because of the check above.
5174
idx += 1;
5275
}
5376

@@ -60,10 +83,12 @@ impl DescriptorTable {
6083
idx
6184
};
6285

86+
let idx = DescriptorHandle::new(idx).unwrap();
87+
6388
let prev = self.descriptors.insert(idx, descriptor);
64-
debug_assert!(prev.is_none(), "Already a descriptor at {}", idx);
89+
assert!(prev.is_none(), "Already a descriptor at {}", idx);
6590

66-
idx
91+
Ok(idx)
6792
}
6893

6994
// Call after inserting to `available_indices`, to free any that are contiguous
@@ -82,23 +107,24 @@ impl DescriptorTable {
82107
}
83108

84109
/// Get the descriptor at `idx`, if any.
85-
pub fn get(&self, idx: u32) -> Option<&Descriptor> {
110+
pub fn get(&self, idx: DescriptorHandle) -> Option<&Descriptor> {
86111
self.descriptors.get(&idx)
87112
}
88113

89114
/// Get the descriptor at `idx`, if any.
90-
pub fn get_mut(&mut self, idx: u32) -> Option<&mut Descriptor> {
115+
pub fn get_mut(&mut self, idx: DescriptorHandle) -> Option<&mut Descriptor> {
91116
self.descriptors.get_mut(&idx)
92117
}
93118

94-
/// Insert a descriptor at `index`. If a descriptor is already present at
95-
/// that index, it is unregistered from that index and returned.
96-
fn set(&mut self, index: u32, descriptor: Descriptor) -> Option<Descriptor> {
119+
/// Insert a descriptor at `index`. If a descriptor is already present at that index, it is
120+
/// unregistered from that index and returned.
121+
#[must_use]
122+
fn set(&mut self, index: DescriptorHandle, descriptor: Descriptor) -> Option<Descriptor> {
97123
// We ensure the index is no longer in `self.available_indices`. We *don't* ensure
98124
// `self.next_index` is > `index`, since that'd require adding the indices in between to
99125
// `self.available_indices`. It uses less memory and is no more expensive to iterate when
100126
// *using* `self.available_indices` instead.
101-
self.available_indices.remove(&index);
127+
self.available_indices.remove(&index.val());
102128

103129
if let Some(prev) = self.descriptors.insert(index, descriptor) {
104130
trace!("Overwriting index {}", index);
@@ -109,29 +135,45 @@ impl DescriptorTable {
109135
}
110136
}
111137

112-
/// Register a descriptor and return its fd handle.
113-
pub fn register_descriptor(&mut self, desc: Descriptor) -> u32 {
114-
self.add(desc, 0)
138+
/// Register a descriptor and return its fd handle. Equivalent to
139+
/// `register_descriptor_with_min_fd(desc, 0)`. If the descriptor could not be added, the
140+
/// descriptor is returned in the `Err`.
141+
pub fn register_descriptor(
142+
&mut self,
143+
desc: Descriptor,
144+
) -> Result<DescriptorHandle, Descriptor> {
145+
const ZERO: DescriptorHandle = match DescriptorHandle::new(0) {
146+
Some(x) => x,
147+
None => unreachable!(),
148+
};
149+
self.add(desc, ZERO)
115150
}
116151

117-
/// Register a descriptor and return its fd handle.
118-
pub fn register_descriptor_with_min_fd(&mut self, desc: Descriptor, min_fd: u32) -> u32 {
152+
/// Register a descriptor and return its fd handle. If the descriptor could not be added, the
153+
/// descriptor is returned in the `Err`.
154+
pub fn register_descriptor_with_min_fd(
155+
&mut self,
156+
desc: Descriptor,
157+
min_fd: DescriptorHandle,
158+
) -> Result<DescriptorHandle, Descriptor> {
119159
self.add(desc, min_fd)
120160
}
121161

122162
/// Register a descriptor with a given fd handle and return the descriptor that it replaced.
163+
#[must_use]
123164
pub fn register_descriptor_with_fd(
124165
&mut self,
125166
desc: Descriptor,
126-
new_fd: u32,
167+
new_fd: DescriptorHandle,
127168
) -> Option<Descriptor> {
128169
self.set(new_fd, desc)
129170
}
130171

131172
/// Deregister the descriptor with the given fd handle and return it.
132-
pub fn deregister_descriptor(&mut self, fd: u32) -> Option<Descriptor> {
173+
#[must_use]
174+
pub fn deregister_descriptor(&mut self, fd: DescriptorHandle) -> Option<Descriptor> {
133175
let maybe_descriptor = self.descriptors.remove(&fd);
134-
self.available_indices.insert(fd);
176+
self.available_indices.insert(fd.val());
135177
self.trim_tail();
136178
maybe_descriptor
137179
}
@@ -164,3 +206,71 @@ impl Default for DescriptorTable {
164206
Self::new()
165207
}
166208
}
209+
210+
/// A handle for a file descriptor.
211+
#[derive(Copy, Clone, Debug, Hash, PartialEq, Eq, PartialOrd, Ord)]
212+
pub struct DescriptorHandle(u32);
213+
214+
impl DescriptorHandle {
215+
/// Returns `Some` if `fd` is less than [`FD_MAX`]. Can be used in `const` contexts.
216+
pub const fn new(fd: u32) -> Option<Self> {
217+
if fd > FD_MAX {
218+
return None;
219+
}
220+
221+
Some(DescriptorHandle(fd))
222+
}
223+
224+
pub fn val(&self) -> u32 {
225+
self.0
226+
}
227+
}
228+
229+
impl std::fmt::Display for DescriptorHandle {
230+
fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
231+
self.0.fmt(f)
232+
}
233+
}
234+
235+
impl From<DescriptorHandle> for u32 {
236+
fn from(x: DescriptorHandle) -> u32 {
237+
x.0
238+
}
239+
}
240+
241+
impl From<DescriptorHandle> for i32 {
242+
fn from(x: DescriptorHandle) -> i32 {
243+
const _: () = assert!(FD_MAX <= i32::MAX as u32);
244+
// the constructor makes sure this won't panic
245+
x.0.try_into().unwrap()
246+
}
247+
}
248+
249+
impl TryFrom<u32> for DescriptorHandle {
250+
type Error = DescriptorHandleError;
251+
fn try_from(x: u32) -> Result<Self, Self::Error> {
252+
DescriptorHandle::new(x).ok_or(DescriptorHandleError())
253+
}
254+
}
255+
256+
impl TryFrom<i32> for DescriptorHandle {
257+
type Error = DescriptorHandleError;
258+
fn try_from(x: i32) -> Result<Self, Self::Error> {
259+
x.try_into()
260+
.ok()
261+
.and_then(DescriptorHandle::new)
262+
.ok_or(DescriptorHandleError())
263+
}
264+
}
265+
266+
/// The handle is not valid.
267+
#[derive(Copy, Clone, Debug, PartialEq, Eq)]
268+
pub struct DescriptorHandleError();
269+
270+
impl std::fmt::Display for DescriptorHandleError {
271+
fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
272+
write!(f, "Not a valid descriptor handle")
273+
}
274+
}
275+
276+
impl std::error::Error for DescriptorHandleError {}

‎src/main/host/process.rs‎

Lines changed: 18 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -32,7 +32,7 @@ use crate::host::syscall::formatter::FmtOptions;
3232
use crate::utility::callback_queue::CallbackQueue;
3333
use crate::utility::pathbuf_to_nul_term_cstring;
3434

35-
use super::descriptor::descriptor_table::DescriptorTable;
35+
use super::descriptor::descriptor_table::{DescriptorHandle, DescriptorTable};
3636
use super::host::Host;
3737
use super::memory_manager::{MemoryManager, ProcessMemoryRef, ProcessMemoryRefMut};
3838
use super::syscall::formatter::StraceFmtMode;
@@ -422,16 +422,24 @@ impl Process {
422422
assert!(!self.is_running());
423423

424424
self.open_stdio_file_helper(
425-
libc::STDIN_FILENO as u32,
425+
libc::STDIN_FILENO.try_into().unwrap(),
426426
"/dev/null".into(),
427427
OFlag::O_RDONLY,
428428
);
429429

430430
let name = self.output_file_name(host, "stdout");
431-
self.open_stdio_file_helper(libc::STDOUT_FILENO as u32, name, OFlag::O_WRONLY);
431+
self.open_stdio_file_helper(
432+
libc::STDOUT_FILENO.try_into().unwrap(),
433+
name,
434+
OFlag::O_WRONLY,
435+
);
432436

433437
let name = self.output_file_name(host, "stderr");
434-
self.open_stdio_file_helper(libc::STDERR_FILENO as u32, name, OFlag::O_WRONLY);
438+
self.open_stdio_file_helper(
439+
libc::STDERR_FILENO.try_into().unwrap(),
440+
name,
441+
OFlag::O_WRONLY,
442+
);
435443

436444
// Create the main thread and add it to our thread list.
437445
let tid = self.thread_group_leader_id();
@@ -713,7 +721,7 @@ impl Process {
713721

714722
fn open_stdio_file_helper(
715723
&self,
716-
fd: u32,
724+
fd: DescriptorHandle,
717725
path: PathBuf,
718726
access_mode: OFlag,
719727
) -> *mut cshadow::RegularFile {
@@ -1444,9 +1452,10 @@ mod export {
14441452
proc.borrow(h.root())
14451453
.descriptor_table_borrow_mut()
14461454
.register_descriptor(*desc)
1455+
.unwrap()
14471456
})
14481457
.unwrap();
1449-
fd.try_into().unwrap()
1458+
fd.into()
14501459
}
14511460

14521461
/// Get a temporary reference to a descriptor.
@@ -1457,7 +1466,7 @@ mod export {
14571466
) -> *const Descriptor {
14581467
let proc = unsafe { proc.as_ref().unwrap() };
14591468

1460-
let handle: u32 = match handle.try_into() {
1469+
let handle = match handle.try_into() {
14611470
Ok(i) => i,
14621471
Err(_) => {
14631472
log::debug!("Attempted to get a descriptor with handle {}", handle);
@@ -1482,7 +1491,7 @@ mod export {
14821491
) -> *mut Descriptor {
14831492
let proc = unsafe { proc.as_ref().unwrap() };
14841493

1485-
let handle: u32 = match handle.try_into() {
1494+
let handle = match handle.try_into() {
14861495
Ok(i) => i,
14871496
Err(_) => {
14881497
log::debug!("Attempted to get a descriptor with handle {}", handle);
@@ -1511,7 +1520,7 @@ mod export {
15111520
) -> *mut cshadow::LegacyFile {
15121521
let proc = unsafe { proc.as_ref().unwrap() };
15131522

1514-
let handle: u32 = match handle.try_into() {
1523+
let handle = match handle.try_into() {
15151524
Ok(i) => i,
15161525
Err(_) => {
15171526
log::debug!("Attempted to get a descriptor with handle {}", handle);

‎src/main/host/syscall/handler/eventfd.rs‎

Lines changed: 3 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -78,10 +78,11 @@ impl SyscallHandler {
7878
let fd = ctx
7979
.process
8080
.descriptor_table_borrow_mut()
81-
.register_descriptor(desc);
81+
.register_descriptor(desc)
82+
.or(Err(Errno::ENFILE))?;
8283

8384
log::trace!("eventfd() returning fd {}", fd);
8485

85-
Ok(fd.into())
86+
Ok(fd.val().into())
8687
}
8788
}

‎src/main/host/syscall/handler/fcntl.rs‎

Lines changed: 9 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -3,6 +3,7 @@ use crate::host::context::ThreadContext;
33
use crate::host::descriptor::{CompatFile, DescriptorFlags, File, FileStatus};
44
use crate::host::syscall::handler::SyscallHandler;
55
use crate::host::syscall_types::{SysCallArgs, SysCallReg, SyscallResult};
6+
67
use log::warn;
78
use nix::errno::Errno;
89
use nix::fcntl::OFlag;
@@ -138,18 +139,22 @@ impl SyscallHandler {
138139
}
139140
libc::F_DUPFD => {
140141
let min_fd: i32 = args.args[2].into();
141-
let min_fd: u32 = min_fd.try_into().map_err(|_| nix::errno::Errno::EINVAL)?;
142+
let min_fd = min_fd.try_into().or(Err(Errno::EINVAL))?;
142143

143144
let new_desc = desc.dup(DescriptorFlags::empty());
144-
let new_fd = desc_table.register_descriptor_with_min_fd(new_desc, min_fd);
145+
let new_fd = desc_table
146+
.register_descriptor_with_min_fd(new_desc, min_fd)
147+
.or(Err(Errno::EINVAL))?;
145148
SysCallReg::from(i32::try_from(new_fd).unwrap())
146149
}
147150
libc::F_DUPFD_CLOEXEC => {
148151
let min_fd: i32 = args.args[2].into();
149-
let min_fd: u32 = min_fd.try_into().map_err(|_| nix::errno::Errno::EINVAL)?;
152+
let min_fd = min_fd.try_into().or(Err(Errno::EINVAL))?;
150153

151154
let new_desc = desc.dup(DescriptorFlags::CLOEXEC);
152-
let new_fd = desc_table.register_descriptor_with_min_fd(new_desc, min_fd);
155+
let new_fd = desc_table
156+
.register_descriptor_with_min_fd(new_desc, min_fd)
157+
.or(Err(Errno::EINVAL))?;
153158
SysCallReg::from(i32::try_from(new_fd).unwrap())
154159
}
155160
libc::F_GETPIPE_SZ => {

0 commit comments

Comments
 (0)