From 727691fd8eb563be9f57382c3434107690d042e1 Mon Sep 17 00:00:00 2001
From: Kevin Hui <kevinhui@meta.com>
Date: Fri, 29 Mar 2024 15:00:31 -0700
Subject: [PATCH] Fix null-terminated string bug in serialize_dirent64() and
 deserialize_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
---
 detcore/src/dirents.rs | 15 +++++++++++----
 1 file changed, 11 insertions(+), 4 deletions(-)

diff --git a/detcore/src/dirents.rs b/detcore/src/dirents.rs
index eea32df..e634a66 100644
--- a/detcore/src/dirents.rs
+++ b/detcore/src/dirents.rs
@@ -11,6 +11,7 @@
 use std::cmp::Ordering;
 use std::ptr;
 
+use libc::strlen;
 use serde::Deserialize;
 use serde::Serialize;
 
@@ -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],
 }
 
@@ -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;
@@ -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);