Make NULL check in argument parsing the same on all unix platforms

Author: madsmtm

std/src/sys/pal/unix/args.rs

@@ -5,8 +5,9 @@
 
 #![allow(dead_code)] // runtime init functions not used during testing
 
-use crate::ffi::OsString;
+use crate::ffi::{CStr, OsString};
 use crate::fmt;
+use crate::os::unix::ffi::OsStringExt;
 use crate::vec;
 
 /// One-time global initialization.
@@ -16,7 +17,46 @@ pub unsafe fn init(argc: isize, argv: *const *const u8) {
 
 /// Returns the command line arguments
 pub fn args() -> Args {
-    imp::args()
+    let (argc, argv) = imp::argc_argv();
+
+    let mut vec = Vec::with_capacity(argc as usize);
+
+    for i in 0..argc {
+        // SAFETY: `argv` is non-null if `argc` is positive, and it is
+        // guaranteed to be at least as long as `argc`, so reading from it
+        // should be safe.
+        let ptr = unsafe { argv.offset(i).read() };
+
+        // Some C commandline parsers (e.g. GLib and Qt) are replacing already
+        // handled arguments in `argv` with `NULL` and move them to the end.
+        //
+        // Since they can't directly ensure updates to `argc` as well, this
+        // means that `argc` might be bigger than the actual number of
+        // non-`NULL` pointers in `argv` at this point.
+        //
+        // To handle this we simply stop iterating at the first `NULL`
+        // argument. `argv` is also guaranteed to be `NULL`-terminated so any
+        // non-`NULL` arguments after the first `NULL` can safely be ignored.
+        if ptr.is_null() {
+            // NOTE: On Apple platforms, `-[NSProcessInfo arguments]` does not
+            // stop iterating here, but instead `continue`, always iterating
+            // up until it reached `argc`.
+            //
+            // This difference will only matter in very specific circumstances
+            // where `argc`/`argv` have been modified, but in unexpected ways,
+            // so it likely doesn't really matter which option we choose.
+            // See the following PR for further discussion:
+            // <https://github.com/rust-lang/rust/pull/125225>
+            break;
+        }
+
+        // SAFETY: Just checked that the pointer is not NULL, and arguments
+        // are otherwise guaranteed to be valid C strings.
+        let cstr = unsafe { CStr::from_ptr(ptr) };
+        vec.push(OsStringExt::from_vec(cstr.to_bytes().to_vec()));
+    }
+
+    Args { iter: vec.into_iter() }
 }
 
 pub struct Args {
@@ -75,9 +115,7 @@ impl DoubleEndedIterator for Args {
     target_os = "hurd",
 ))]
 mod imp {
-    use super::Args;
-    use crate::ffi::{CStr, OsString};
-    use crate::os::unix::prelude::*;
+    use crate::ffi::c_char;
     use crate::ptr;
     use crate::sync::atomic::{AtomicIsize, AtomicPtr, Ordering};
 
@@ -126,45 +164,19 @@ mod imp {
         init_wrapper
     };
 
-    pub fn args() -> Args {
-        Args { iter: clone().into_iter() }
-    }
-
-    fn clone() -> Vec<OsString> {
-        unsafe {
-            // Load ARGC and ARGV, which hold the unmodified system-provided
-            // argc/argv, so we can read the pointed-to memory without atomics
-            // or synchronization.
-            //
-            // If either ARGC or ARGV is still zero or null, then either there
-            // really are no arguments, or someone is asking for `args()`
-            // before initialization has completed, and we return an empty
-            // list.
-            let argv = ARGV.load(Ordering::Relaxed);
-            let argc = if argv.is_null() { 0 } else { ARGC.load(Ordering::Relaxed) };
-            let mut args = Vec::with_capacity(argc as usize);
-            for i in 0..argc {
-                let ptr = *argv.offset(i) as *const libc::c_char;
-
-                // Some C commandline parsers (e.g. GLib and Qt) are replacing already
-                // handled arguments in `argv` with `NULL` and move them to the end. That
-                // means that `argc` might be bigger than the actual number of non-`NULL`
-                // pointers in `argv` at this point.
-                //
-                // To handle this we simply stop iterating at the first `NULL` argument.
-                //
-                // `argv` is also guaranteed to be `NULL`-terminated so any non-`NULL` arguments
-                // after the first `NULL` can safely be ignored.
-                if ptr.is_null() {
-                    break;
-                }
-
-                let cstr = CStr::from_ptr(ptr);
-                args.push(OsStringExt::from_vec(cstr.to_bytes().to_vec()));
-            }
-
-            args
-        }
+    pub fn argc_argv() -> (isize, *const *const c_char) {
+        // Load ARGC and ARGV, which hold the unmodified system-provided
+        // argc/argv, so we can read the pointed-to memory without atomics or
+        // synchronization.
+        //
+        // If either ARGC or ARGV is still zero or null, then either there
+        // really are no arguments, or someone is asking for `args()` before
+        // initialization has completed, and we return an empty list.
+        let argv = ARGV.load(Ordering::Relaxed);
+        let argc = if argv.is_null() { 0 } else { ARGC.load(Ordering::Relaxed) };
+
+        // Cast from `*mut *const u8` to `*const *const c_char`
+        (argc, argv.cast())
     }
 }
 
@@ -183,16 +195,14 @@ mod imp {
 // of this used `[[NSProcessInfo processInfo] arguments]`.
 #[cfg(target_vendor = "apple")]
 mod imp {
-    use super::Args;
-    use crate::ffi::{c_char, c_int, CStr};
-    use crate::os::unix::prelude::*;
+    use crate::ffi::{c_char, c_int};
 
     pub unsafe fn init(_argc: isize, _argv: *const *const u8) {
         // No need to initialize anything in here, `libdyld.dylib` has already
         // done the work for us.
     }
 
-    pub fn args() -> Args {
+    pub fn argc_argv() -> (isize, *const *const c_char) {
         extern "C" {
             // These functions are in crt_externs.h.
             fn _NSGetArgc() -> *mut c_int;
@@ -212,42 +222,20 @@ mod imp {
         // SAFETY: Same as above.
         let argv = unsafe { _NSGetArgv().read() };
 
-        let mut vec = Vec::with_capacity(argc as usize);
-
-        for i in 0..argc {
-            // SAFETY: `argv` is at least as long as `argc`, so reading from
-            // it should be safe.
-            let ptr = unsafe { argv.offset(i as isize).read() };
-
-            // Entries may have been removed from `argv` by setting them to
-            // NULL, without updating `argc`.
-            if ptr.is_null() {
-                // We continue instead of break here, as an argument may have
-                // been set to `NULL` in the middle, instead of at the end of
-                // the list.
-                //
-                // This is the same as what `-[NSProcessInfo arguments]` does.
-                continue;
-            }
-
-            // SAFETY: Just checked that the pointer is not NULL, and
-            // arguments are otherwise guaranteed to be valid C strings.
-            let cstr = unsafe { CStr::from_ptr(ptr) };
-            vec.push(OsStringExt::from_vec(cstr.to_bytes().to_vec()));
-        }
-
-        Args { iter: vec.into_iter() }
+        // Cast from `*mut *mut c_char` to `*const *const c_char`
+        (argc as isize, argv.cast())
     }
 }
 
 #[cfg(any(target_os = "espidf", target_os = "vita"))]
 mod imp {
-    use super::Args;
+    use crate::ffi::c_char;
+    use crate::ptr;
 
     #[inline(always)]
     pub unsafe fn init(_argc: isize, _argv: *const *const u8) {}
 
-    pub fn args() -> Args {
-        Args { iter: Vec::new().into_iter() }
+    pub fn argc_argv() -> (isize, *const *const c_char) {
+        (0, ptr::null())
     }
 }