fix fdtables race conditions and off-by-one boundary check (#1057)

close_virtualfd: mutate in place through the DashMap guard instead of
copy-modify-reinsert, which had a window where concurrent fd opens
could be silently overwritten.

_increment_fdcount: use entry().or_insert(0) instead of get_mut/insert
to prevent two threads from both inserting 1 when they race on a
missing key.

set_specific_virtualfd: change > to >= for FD_PER_PROCESS_MAX bounds
check to prevent index 1024 on a [_; 1024] array.

Fixes #1051

Author: rennergade

src/fdtables/src/dashmaparrayglobal.rs

@@ -205,7 +205,7 @@ pub fn get_specific_virtual_fd(
     // Note that, I need to use the FD_PER_PROCESS_MAX setting because this
     // is also how I'm tracking how many values you have open.  If this
     // changed, then these constants could be decoupled...
-    if requested_virtualfd > FD_PER_PROCESS_MAX {
+    if requested_virtualfd >= FD_PER_PROCESS_MAX {
         return Err(threei::Errno::EBADF as u64);
     }
 
@@ -404,24 +404,23 @@ pub fn close_virtualfd(cageid:u64, virtfd:u64) -> Result<(),threei::RetVal> {
 
     assert!(check_cage_exists(cageid),"Unknown cageid in fdtable access");
 
-    // derefing this so I don't hold a lock and deadlock close handlers
-    let mut myfdrow = *FDTABLE.get_mut(&cageid).unwrap();
-
-
-    if myfdrow[virtfd as usize].is_some() {
-        let entry = myfdrow[virtfd as usize];
-
-        // Zero out this entry before calling the close handler...
-        myfdrow[virtfd as usize] = None;
+    // Mutate in place under the guard, extract the entry, then drop the
+    // guard before calling the close handler (which may re-enter fdtables).
+    let entry = {
+        let mut guard = FDTABLE.get_mut(&cageid).unwrap();
+        let entry = guard[virtfd as usize];
+        guard[virtfd as usize] = None;
+        entry
+        // guard drops here — lock released
+    };
 
-        // Re-insert the modified myfdrow since I've been modifying a copy
-        FDTABLE.insert(cageid, myfdrow.clone());
-        
-        // always _decrement last as it may call the user handler...
-        _decrement_fdcount(entry.unwrap());
-        return Ok(());
+    match entry {
+        Some(e) => {
+            _decrement_fdcount(e);
+            Ok(())
+        }
+        None => Err(threei::Errno::EBADFD as u64),
     }
-    Err(threei::Errno::EBADFD as u64)
 }
 
 
@@ -501,12 +500,9 @@ fn _increment_fdcount(entry:FDTableEntry) {
 
     let mytuple = (entry.fdkind, entry.underfd);
 
-    // Get a mutable reference to the entry so we can update it.
-    if let Some(mut count) = FDCOUNT.get_mut(&mytuple) {
-        *count += 1;
-    } else {
-        FDCOUNT.insert(mytuple, 1);
-    }
+    // Atomic increment-or-insert via entry API to avoid race where two
+    // threads both see None and both insert 1.
+    *FDCOUNT.entry(mytuple).or_insert(0) += 1;
 }
 
 

src/fdtables/src/dashmapvecglobal.rs

@@ -204,7 +204,7 @@ pub fn get_specific_virtual_fd(
     // Note that, I need to use the FD_PER_PROCESS_MAX setting because this
     // is also how I'm tracking how many values you have open.  If this
     // changed, then these constants could be decoupled...
-    if requested_virtualfd > FD_PER_PROCESS_MAX {
+    if requested_virtualfd >= FD_PER_PROCESS_MAX {
         return Err(threei::Errno::EBADF as u64);
     }
 
@@ -402,23 +402,23 @@ pub fn close_virtualfd(cageid:u64, virtfd:u64) -> Result<(),threei::RetVal> {
 
     assert!(FDTABLE.contains_key(&cageid),"Unknown cageid in fdtable access");
 
-    // cloning this so I don't hold a lock and deadlock close handlers
-    let mut myfdrow = FDTABLE.get_mut(&cageid).unwrap().clone();
-
-
-    if myfdrow[virtfd as usize].is_some() {
-        let entry = myfdrow[virtfd as usize];
-
-        // Zero out this entry before calling the close handler...
-        myfdrow[virtfd as usize] = None;
-
-        FDTABLE.insert(cageid, myfdrow.clone());
+    // Mutate in place under the guard, extract the entry, then drop the
+    // guard before calling the close handler (which may re-enter fdtables).
+    let entry = {
+        let mut guard = FDTABLE.get_mut(&cageid).unwrap();
+        let entry = guard[virtfd as usize];
+        guard[virtfd as usize] = None;
+        entry
+        // guard drops here — lock released
+    };
 
-        // always _decrement last as it may call the user handler...
-        _decrement_fdcount(entry.unwrap());
-        return Ok(());
+    match entry {
+        Some(e) => {
+            _decrement_fdcount(e);
+            Ok(())
+        }
+        None => Err(threei::Errno::EBADFD as u64),
     }
-    Err(threei::Errno::EBADFD as u64)
 }
 
 
@@ -498,12 +498,9 @@ fn _increment_fdcount(entry:FDTableEntry) {
 
     let mytuple = (entry.fdkind, entry.underfd);
 
-    // Get a mutable reference to the entry so we can update it.
-    if let Some(mut count) = FDCOUNT.get_mut(&mytuple) {
-        *count += 1;
-    } else {
-        FDCOUNT.insert(mytuple, 1);
-    }
+    // Atomic increment-or-insert via entry API to avoid race where two
+    // threads both see None and both insert 1.
+    *FDCOUNT.entry(mytuple).or_insert(0) += 1;
 }