Skip to content

Commit

Permalink
Fix broken tests after toolchain update
Browse files Browse the repository at this point in the history 
Summary:
These tests could potentially do unaligned pointer reads because the test data is not guaranteed to be aligned.

There is also now a debug assertion in `CString::from_vec_with_nul_unchecked` that ensures the NUL byte is at the end of the `Vec`. Names in dirents may be padded out to keep them aligned.

Reviewed By: dtolnay

Differential Revision: D53597638

fbshipit-source-id: ad6018564d6afd4e8f5de4390d63f64e6c5cadf7
  • Loading branch information
@jasonwhite @facebook-github-bot
jasonwhite authored and facebook-github-bot committed Feb 9, 2024
1 parent 9c5eb61 commit 1422082
Show file tree
Hide file tree
Showing 2 changed files with 38 additions and 32 deletions.
42 changes: 22 additions & 20 deletions detcore/src/dirents.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -9,41 +9,40 @@
//! Linux dirent64 helpers

use std::cmp::Ordering;
use std::ffi::CString;
use std::ptr;

use serde::Deserialize;
use serde::Serialize;

#[derive(Debug, Clone, PartialEq, Eq, Deserialize, Serialize)]
pub struct Dirent64 {
pub struct Dirent64<'a> {
pub(crate) ino: u64,
pub(crate) off: i64,
pub(crate) ty: u8,
pub(crate) reclen: u16,
/// null terminated c string, NB: multiple null bytes are expected!
pub(crate) name: CString,
pub(crate) name: &'a [u8],
}

// sort by name, but "." < ".." <= ..
#[allow(clippy::non_canonical_partial_ord_impl)]
impl PartialOrd for Dirent64 {
impl<'a> PartialOrd for Dirent64<'a> {
fn partial_cmp(&self, other: &Self) -> Option<Ordering> {
if self.name.as_bytes()[..2] == b".\0"[..] {
if self.name == b".\0" {
Some(Ordering::Less)
} else if other.name.as_bytes()[..2] == b".\0"[..] {
} else if other.name == b".\0" {
Some(Ordering::Greater)
} else if self.name.as_bytes()[..3] == b"..\0"[..] {
} else if self.name == b"..\0" {
Some(Ordering::Less)
} else if other.name.as_bytes()[..3] == b"..\0"[..] {
} else if other.name == b"..\0" {
Some(Ordering::Greater)
} else {
self.name.partial_cmp(&other.name)
self.name.partial_cmp(other.name)
}
}
}

impl Ord for Dirent64 {
impl<'a> Ord for Dirent64<'a> {
fn cmp(&self, other: &Self) -> Ordering {
self.partial_cmp(other).unwrap()
}
Expand All @@ -68,13 +67,12 @@ pub unsafe fn deserialize_dirents64(bytes: &[u8]) -> Vec<Dirent64> {
let ty = ptr::read(dirp.offset(j).cast::<u8>());
j += std::mem::size_of::<u8>() as isize;
let name = std::slice::from_raw_parts(dirp.offset(j), 1 + reclen as usize - j as usize);
let name = Vec::from(name);
let ent = Dirent64 {
ino,
off,
reclen,
ty,
name: CString::from_vec_with_nul_unchecked(name),
name,
};
res.push(ent);
k += reclen as isize;
Expand Down Expand Up @@ -103,7 +101,7 @@ pub unsafe fn serialize_dirents64(dents: &[Dirent64], bytes: &mut [u8]) -> usize
ptr::copy_nonoverlapping(
ent.name.as_ptr() as *const u8,
dirp.offset(j),
ent.name.as_bytes().len(),
ent.name.len(),
);
k += reclen as isize;
i += 1;
Expand All @@ -128,15 +126,14 @@ pub unsafe fn deserialize_dirents(bytes: &[u8]) -> Vec<Dirent64> {
let reclen = ptr::read(dirp.offset(j).cast::<u16>());
j += std::mem::size_of::<u16>() as isize;
let name = std::slice::from_raw_parts(dirp.offset(j), reclen as usize - j as usize);
let name = Vec::from(name);
j = reclen as isize - 1;
let ty = ptr::read(dirp.offset(j).cast::<u8>());
let ent = Dirent64 {
ino,
off,
reclen,
ty,
name: CString::from_vec_with_nul_unchecked(name),
name,
};
res.push(ent);
k += reclen as isize;
Expand All @@ -163,7 +160,7 @@ pub unsafe fn serialize_dirents(dents: &[Dirent64], bytes: &mut [u8]) -> usize {
ptr::copy_nonoverlapping(
ent.name.as_ptr() as *const u8,
dirp.offset(j),
ent.name.as_bytes().len(),
ent.name.len(),
);
j = reclen as isize - 1;
ptr::write(dirp.offset(j).cast::<u8>(), ent.ty);
Expand All @@ -177,8 +174,11 @@ pub unsafe fn serialize_dirents(dents: &[Dirent64], bytes: &mut [u8]) -> usize {
mod test {
use super::*;

#[repr(align(8))]
struct Aligned<T>(T);

// getdents from my homedir
const HOME_DIRENTS: &[u8] = &[
const HOME_DIRENTS: &[u8] = &Aligned([
0xf2, 0x5b, 0x79, 0x00, 0x00, 0x00, 0x00, 0x00, 0x01, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00,
0x00, 0x18, 0x00, 0x2e, 0x00, 0x00, 0x00, 0x00, 0x04, 0x1f, 0x11, 0x79, 0x00, 0x00, 0x00,
0x00, 0x00, 0x02, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x18, 0x00, 0x2e, 0x2e, 0x00,
Expand Down Expand Up @@ -390,10 +390,11 @@ mod test {
0x36, 0xf2, 0xde, 0x04, 0x00, 0x00, 0x00, 0x00, 0xff, 0xff, 0xff, 0x7f, 0x00, 0x00, 0x00,
0x00, 0x20, 0x00, 0x2e, 0x76, 0x69, 0x6d, 0x69, 0x6e, 0x66, 0x6f, 0x00, 0x00, 0x00, 0x00,
0x00, 0x08,
];
])
.0;

// getdents64 from my homedir
const HOME_DIRENTS64: &[u8] = &[
const HOME_DIRENTS64: &[u8] = &Aligned([
0xf2, 0x5b, 0x79, 0x00, 0x00, 0x00, 0x00, 0x00, 0x01, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00,
0x00, 0x18, 0x00, 0x04, 0x2e, 0x00, 0x00, 0x00, 0x00, 0x1f, 0x11, 0x79, 0x00, 0x00, 0x00,
0x00, 0x00, 0x02, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x18, 0x00, 0x04, 0x2e, 0x2e,
Expand Down Expand Up @@ -605,7 +606,8 @@ mod test {
0x57, 0xc8, 0xdb, 0x04, 0x00, 0x00, 0x00, 0x00, 0xff, 0xff, 0xff, 0x7f, 0x00, 0x00, 0x00,
0x00, 0x20, 0x00, 0x08, 0x2e, 0x6c, 0x65, 0x73, 0x73, 0x68, 0x73, 0x74, 0x00, 0x00, 0x00,
0x00, 0x00,
];
])
.0;

#[test]
fn dents_can_deserialize_serialize() {
Expand Down
28 changes: 16 additions & 12 deletions detcore/src/syscalls/files.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -966,24 +966,26 @@ impl<T: RecordOrReplay> Detcore<T> {
return Ok(0);
}

let mut cached_bytes = vec![0; nb as usize];
cached_bytes.reserve_exact(128);
let mut dents_bytes = vec![0; nb as usize];
dents_bytes.reserve_exact(128);

guest
.memory()
.read_exact(dirent.cast(), cached_bytes.as_mut_slice())?;
.read_exact(dirent.cast(), dents_bytes.as_mut_slice())?;

let mut dents = unsafe { deserialize_dirents(&cached_bytes) };
let mut dents = unsafe { deserialize_dirents(&dents_bytes) };
dents.sort();
for dent in &mut dents {
let (d_ino, _) = determinize_inode(guest, dent.ino).await;
dent.ino = d_ino;
}
let _ = unsafe { serialize_dirents(&dents, cached_bytes.as_mut_slice()) };

let mut dents_bytes = vec![0; dents_bytes.len()];
let _ = unsafe { serialize_dirents(&dents, &mut dents_bytes) };

guest
.memory()
.write_exact(dirent.cast(), cached_bytes.as_slice())?;
.write_exact(dirent.cast(), dents_bytes.as_slice())?;
Ok(nb)
}

Expand All @@ -1004,24 +1006,26 @@ impl<T: RecordOrReplay> Detcore<T> {
return Ok(0);
}

let mut cached_bytes = vec![0; nb as usize];
cached_bytes.reserve_exact(128);
let mut dents_bytes = vec![0; nb as usize];
dents_bytes.reserve_exact(128);

guest
.memory()
.read_exact(dirent.cast(), cached_bytes.as_mut_slice())?;
.read_exact(dirent.cast(), dents_bytes.as_mut_slice())?;

let mut dents = unsafe { deserialize_dirents64(&cached_bytes) };
let mut dents = unsafe { deserialize_dirents64(&dents_bytes) };
dents.sort();
for dent in &mut dents {
let (d_ino, _) = determinize_inode(guest, dent.ino).await;
dent.ino = d_ino;
}
let _ = unsafe { serialize_dirents64(&dents, cached_bytes.as_mut_slice()) };

let mut dents_bytes = vec![0; dents_bytes.len()];
let _ = unsafe { serialize_dirents64(&dents, &mut dents_bytes) };

guest
.memory()
.write_exact(dirent.cast(), cached_bytes.as_slice())?;
.write_exact(dirent.cast(), dents_bytes.as_slice())?;
Ok(nb)
}
}
Expand Down

0 comments on commit 1422082

Please sign in to comment.