Skip to content

Commit

Permalink
Fix null-terminated string bug in serialize_dirent64() and deserializ…
Browse files Browse the repository at this point in the history
…e_dirent64()

Summary:
I was trying to get hermit to work for `mkfs.ext3` when I kept running into a peculiar error.

It seem like when we run `mkfs` with hermit, we successfully create the partition, but it fails to perform some sort of clean up task: P1201825018

Looking at the ASAN bug trace, it seems like the offender is this line: https://www.internalfb.com/phabricator/paste/view/P1201825018?lines=25

This brings us to this line:
https://www.internalfb.com/code/fbsource/[83ba118d4900598aaa2adc4befd8b09450225915]/fbcode/hermetic_infra/hermit/detcore/src/dirents.rs?lines=101-105

In the `readdir` [man page](https://man7.org/linux/man-pages/man3/readdir.3.html), it states that the name is *null-terminated*:

```
           struct dirent {
               ino_t          d_ino;       /* Inode number */
               off_t          d_off;       /* Not an offset; see below */
               unsigned short d_reclen;    /* Length of this record */
               unsigned char  d_type;      /* Type of file; not supported
                                              by all filesystem types */
               char           d_name[256]; /* Null-terminated filename */
           };
```

Moreover, it seems that there is an explicit callout against using the length or `sizeof()` methods to identify the size of this character:

> Warning: applications should avoid any dependence on the size of
> the d_name field.  POSIX defines it as char d_name[], a character
> array of unspecified size, with at most NAME_MAX characters
> preceding the terminating null byte ('\0').

A fix for this should be, then, to just use `strlen()` unless I'm mistaken

Reviewed By: wangbj

Differential Revision: D55495680

fbshipit-source-id: c428260b41079d8a50e37628aadd12ee49036d72
  • Loading branch information
CookieComputing authored and facebook-github-bot committed Mar 29, 2024
1 parent a7c7eb6 commit 727691f
Showing 1 changed file with 11 additions and 4 deletions.
15 changes: 11 additions & 4 deletions detcore/src/dirents.rs
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@
use std::cmp::Ordering;
use std::ptr;

use libc::strlen;
use serde::Deserialize;
use serde::Serialize;

Expand All @@ -21,6 +22,9 @@ pub struct Dirent64<'a> {
pub(crate) ty: u8,
pub(crate) reclen: u16,
/// null terminated c string, NB: multiple null bytes are expected!
/// We do not use the std::ffi::CStr because it enforces a check that
/// there are no interior null bytes, while Linux's own implementation
/// seems to provide multiple null bytes
pub(crate) name: &'a [u8],
}

Expand Down Expand Up @@ -99,9 +103,12 @@ pub unsafe fn serialize_dirents64(dents: &[Dirent64], bytes: &mut [u8]) -> usize
j += std::mem::size_of::<u8>() as isize;
let reclen = ent.reclen;
ptr::copy_nonoverlapping(
ent.name.as_ptr() as *const u8,
ent.name.as_ptr(),
dirp.offset(j),
ent.name.len(),
// NOTE: name is null-terminated with the man page
// explicitly warning against other methods to read
// d_name besides strlen(): https://man7.org/linux/man-pages/man3/readdir.3.html
strlen(ent.name.as_ptr() as *const i8),
);
k += reclen as isize;
i += 1;
Expand Down Expand Up @@ -158,9 +165,9 @@ pub unsafe fn serialize_dirents(dents: &[Dirent64], bytes: &mut [u8]) -> usize {
j += std::mem::size_of::<u16>() as isize;
let reclen = ent.reclen;
ptr::copy_nonoverlapping(
ent.name.as_ptr() as *const u8,
ent.name.as_ptr(),
dirp.offset(j),
ent.name.len(),
strlen(ent.name.as_ptr() as *const i8),
);
j = reclen as isize - 1;
ptr::write(dirp.offset(j).cast::<u8>(), ent.ty);
Expand Down

0 comments on commit 727691f

Please sign in to comment.