_ctypes, signal, _multiprocessing: the #1357 review findings

.gitignore

@@ -114,5 +114,6 @@ CLAUDE.md
 
 # The scratch files `lib-python/3/test` writes into the working directory
 # (`test.support.TESTFN` is `@test_<pid>_tmp<suffix>`), left behind whenever a
-# run dies before its `tearDown`.
-/@test_*_tmp*
+# run dies before its `tearDown`.  Unanchored: the working directory is
+# wherever the run was started from, not necessarily the repo root.
+@test_*_tmp*

pyre/pyre-interpreter/src/builtins.rs

@@ -6871,6 +6871,21 @@ pub(crate) fn clear_crt_errno() {
     }
 }
 
+/// Put back an errno read earlier, so a call made in between leaves the cell
+/// as the surrounding code left it.  Windows-only for the same reason
+/// [`clear_crt_errno`] is.
+#[cfg(windows)]
+pub(crate) fn set_crt_errno(value: i32) {
+    #[cfg(feature = "host_env")]
+    {
+        rustpython_host_env::os::set_errno(value);
+    }
+    #[cfg(not(feature = "host_env"))]
+    {
+        let _ = value;
+    }
+}
+
 /// The errno the last C runtime call reported.
 pub(crate) fn crt_errno() -> i32 {
     #[cfg(all(windows, feature = "host_env"))]

pyre/pyre-interpreter/src/module/_ctypes/cdata.rs

@@ -505,11 +505,13 @@ fn value_setter(args: &[PyObjectRef]) -> Result<PyObjectRef, crate::PyError> {
     let _roots = pyre_object::gc_roots::push_roots();
     let value_slot = pyre_object::gc_roots::shadow_stack_len();
     pyre_object::gc_roots::pin_root(value);
-    release_bstr_slot(&tc, cdata_addr(obj).unwrap_or(0));
     let mut bytes = encode_value_into(&tc, value, obj, "0")?;
     if unsafe { crate::baseobjspace::lookup_in_type(cls, "_swappedbytes_") }.is_some() {
         bytes.reverse();
     }
+    // `BSTR_set` frees what the slot held only once the new string exists, so
+    // a conversion that refuses its value leaves the previous one readable.
+    release_bstr_slot(&tc, cdata_addr(obj).unwrap_or(0));
     cdata_write(obj, 0, &bytes);
     if matches!(tc.as_str(), "z" | "Z" | "O") {
         let d = crate::baseobjspace::getdict_native(obj);
@@ -1299,6 +1301,10 @@ pub(super) fn decode_slot(tc: &str, bytes: &[u8]) -> PyObjectRef {
 /// every caller has — the object's buffer for a `value` store, that buffer
 /// plus the field offset for a struct or array slot, and the item address for
 /// a pointer store.
+///
+/// Call it only once the replacement bytes exist: `X_set` allocates first and
+/// frees the previous contents immediately before the store, so a value it
+/// refuses leaves the slot as it was.
 #[cfg(windows)]
 pub(super) fn release_bstr_slot(tc: &str, addr: usize) {
     if tc != "X" || addr == 0 {
@@ -1468,6 +1474,9 @@ pub(super) fn encode_value(tc: &str, obj: PyObjectRef) -> Result<Vec<u8>, crate:
                 let bstr = unsafe {
                     windows_sys::Win32::Foundation::SysAllocStringLen(units.as_ptr(), len)
                 };
+                if bstr.is_null() {
+                    return Err(crate::PyError::memory_error(""));
+                }
                 bstr as usize
             } else {
                 return Err(crate::PyError::type_error(format!(

pyre/pyre-interpreter/src/module/_ctypes/funcptr.rs

@@ -1109,6 +1109,11 @@ fn build_callargs(
         return Ok(plain(passed.to_vec()));
     }
     let mut out = plain(Vec::with_capacity(argtypes.len()));
+    // `out_parameter` instantiates the argtype, which is arbitrary Python, so
+    // every value already collected lives in a root slot across it; the list
+    // is read back out of those slots once the loop is done.
+    let _roots = pyre_object::gc_roots::push_roots();
+    let base = pyre_object::gc_roots::shadow_stack_len();
     let mut index = 0;
     for (i, &at) in argtypes.iter().enumerate() {
         let malformed =
@@ -1135,7 +1140,7 @@ fn build_callargs(
             // A locale id never comes from the call.
             PARAMFLAG_FIN_FLCID => defval.unwrap_or_else(|| pyre_object::w_int_new(0)),
             PARAMFLAG_FOUT => {
-                out.outmask |= 1 << i;
+                out.outmask |= param_bit(i);
                 out.numretvals += 1;
                 match defval {
                     Some(defval) => defval,
@@ -1144,17 +1149,28 @@ fn build_callargs(
             }
             direction => {
                 if direction == PARAMFLAG_FIN_FOUT {
-                    out.inoutmask |= 1 << i;
+                    out.inoutmask |= param_bit(i);
                     out.numretvals += 1;
                 }
                 get_arg(&mut index, name.as_deref(), defval, passed, kwargs)?
             }
         };
+        pyre_object::gc_roots::pin_root(value);
         out.args.push(value);
     }
+    for (i, arg) in out.args.iter_mut().enumerate() {
+        *arg = pyre_object::gc_roots::shadow_stack_get(base + i);
+    }
     Ok(out)
 }
 
+/// The `1 << i` bit `_build_callargs` sets in its two `int` masks.  A
+/// parameter past the width of that word has no bit of its own, which is the
+/// range [`build_result`] reads back.
+fn param_bit(i: usize) -> u32 {
+    1u32.checked_shl(i as u32).unwrap_or(0)
+}
+
 /// `_get_arg` — the next positional argument, else the keyword of that name,
 /// else the declared default.
 fn get_arg(
@@ -1457,9 +1473,17 @@ fn internal_pybytes_fromstringandsize(args: &[PyObjectRef]) -> Result<PyObjectRe
 /// `Windows Error 0x<code>`, which is what `test_windows_message` reads.
 #[cfg(windows)]
 fn internal_pyerr_setfromwindowserr(args: &[PyObjectRef]) -> Result<PyObjectRef, crate::PyError> {
+    // `PyErr_SetExcFromWindowsErrWithFilenameObjects` reads `GetLastError()`
+    // when the code it is handed is 0, so an explicit zero says the same
+    // thing as no argument at all rather than naming `ERROR_SUCCESS`.
     let code = match args.first() {
         Some(&arg) => crate::baseobjspace::int_w(arg)? as i32,
-        None => std::io::Error::last_os_error().raw_os_error().unwrap_or(0),
+        None => 0,
+    };
+    let code = if code == 0 {
+        std::io::Error::last_os_error().raw_os_error().unwrap_or(0)
+    } else {
+        code
     };
     Err(crate::PyError::os_error_win32_syscall2(
         code,

pyre/pyre-interpreter/src/module/_ctypes/metaclass.rs

@@ -1316,11 +1316,11 @@ fn cfield_set(args: &[PyObjectRef]) -> PyResult {
                 cdata::cdata_write(obj, offset, &bytes);
                 return Ok(pyre_object::w_none());
             }
-            cdata::release_bstr_slot(&tc, cdata::cdata_addr(obj).unwrap_or(0) + offset);
             let mut bytes = cdata::encode_instance_or_value(&tc, value, obj, &index.to_string())?;
             if field_needs_swap(obj, proto, size) {
                 bytes.reverse();
             }
+            cdata::release_bstr_slot(&tc, cdata::cdata_addr(obj).unwrap_or(0) + offset);
             cdata::cdata_write(obj, offset, &bytes);
             if cdata::is_cdata_instance(value) {
                 cdata::keep_ref(obj, &index.to_string(), value);
@@ -1953,8 +1953,8 @@ fn array_set_index(obj: PyObjectRef, meta: &ArrayMeta, idx: usize, value: PyObje
         "simple" => {
             let tc = cdata::type_code_of(meta.proto)
                 .ok_or_else(|| crate::PyError::type_error("element has no '_type_'"))?;
-            cdata::release_bstr_slot(&tc, cdata::cdata_addr(obj).unwrap_or(0) + offset);
             let bytes = cdata::encode_instance_or_value(&tc, value, obj, &idx.to_string())?;
+            cdata::release_bstr_slot(&tc, cdata::cdata_addr(obj).unwrap_or(0) + offset);
             cdata::cdata_write(obj, offset, &bytes);
             if cdata::is_cdata_instance(value) {
                 cdata::keep_ref(obj, &idx.to_string(), value);
@@ -2442,8 +2442,8 @@ fn pointer_setitem(args: &[PyObjectRef]) -> PyResult {
         "simple" => {
             let tc = cdata::type_code_of(proto)
                 .ok_or_else(|| crate::PyError::type_error("element has no '_type_'"))?;
-            cdata::release_bstr_slot(&tc, addr);
             let bytes = cdata::encode_instance_or_value(&tc, value, obj, &index.to_string())?;
+            cdata::release_bstr_slot(&tc, addr);
             unsafe { host_ctypes::copy_bytes_to_address(addr, &bytes, element_size) };
             if cdata::is_cdata_instance(value) {
                 cdata::keep_ref(obj, &index.to_string(), value);

pyre/pyre-interpreter/src/module/_multiprocessing/mod.rs

@@ -218,7 +218,14 @@ fn semlock_acquire(
         (false, _) => Some(0),
         (true, None) => None,
         (true, Some(seconds)) => {
-            Some((seconds * 1000.0).ceil().clamp(0.0, f64::from(u32::MAX)) as u32)
+            // `interp_semaphore.py:268-275` — a negative timeout is a poll,
+            // and one at half of `INFINITE` (about 25 days) is refused rather
+            // than saturated, so no wait silently becomes a different one.
+            let msecs = (seconds * 1000.0).max(0.0);
+            if msecs >= 0.5 * f64::from(u32::MAX) {
+                return Err(crate::PyError::overflow_error("timeout is too large"));
+            }
+            Some((msecs + 0.5) as u32)
         }
     };
     loop {
@@ -227,8 +234,11 @@ fn semlock_acquire(
             let _blocked = crate::module::thread::before_external_block();
             host_mp::wait_for_single_object(handle, slice)
         };
+        // `interp_semaphore.py:311-315` — the wait has taken the count, so it
+        // is reported before anything that can raise.  A signal pending at
+        // this moment is delivered at the next checkpoint like any other;
+        // raising here would consume the semaphore without handing it over.
         if status == host_mp::wait_object_0() {
-            crate::module::signal::interp_signal::checksignals_now()?;
             return Ok(true);
         }
         if status != host_mp::wait_timeout() {

pyre/pyre-interpreter/src/module/_socket/rsocket_rffi.rs

@@ -100,6 +100,10 @@ pub const SO_ERROR: libc::c_int = ws::SO_ERROR;
 /// The code a call about a descriptor that is not a socket comes back with.
 #[cfg(windows)]
 pub const WSAENOTSOCK: i32 = ws::WSAENOTSOCK;
+/// The code a send that would have blocked comes back with — a buffer with no
+/// room left, which is the one failure a caller may choose to drop.
+#[cfg(windows)]
+pub const WSAEWOULDBLOCK: i32 = ws::WSAEWOULDBLOCK;
 /// The code an expired wait reports, so a timeout this module times itself
 /// reads back the same as one the host produced.
 #[cfg(windows)]

pyre/pyre-interpreter/src/module/signal/interp_signal.rs

@@ -217,11 +217,14 @@ fn check_signum_in_range(signum: i64) -> Result<(), crate::PyError> {
     }
 }
 
+/// The runtime's own signal, absent from the libc crate.
+#[cfg(windows)]
+const SIGBREAK: i32 = 21;
+
 /// Whether the runtime has a signal under this number at all.  `SIGBREAK` is
 /// its own, so the set is spelled out rather than taken from `libc`.
 #[cfg(windows)]
 fn windows_handles_signal(signum: i32) -> bool {
-    const SIGBREAK: i32 = 21;
     matches!(
         signum,
         libc::SIGINT
@@ -642,6 +645,13 @@ pub fn register_module(ns: pyre_object::PyObjectRef) {
                     "set_wakeup_fd() requires an argument",
                 ));
             };
+            // `PYPYSIG_USE_SEND` — set by the Windows probe below, which is
+            // the only place a descriptor is asked whether it is a socket.
+            #[cfg_attr(
+                not(all(windows, not(feature = "sandbox"))),
+                expect(unused_mut)
+            )]
+            let mut use_send = false;
             // interp_signal.py:343-360 — a real fd is validated with
             // `os.fstat` then `get_status_flags`: a bad fd is a ValueError
             // and the fd must already be in non-blocking mode.
@@ -691,6 +701,7 @@ pub fn register_module(ns: pyre_object::PyObjectRef) {
                             &raw mut len,
                         )
                     };
+                    use_send = queried == 0;
                     if queried != 0 {
                         let code = rffi::last_error_code();
                         // `WSAENOTSOCK` is the descriptor answering that it
@@ -724,7 +735,7 @@ pub fn register_module(ns: pyre_object::PyObjectRef) {
             // interp_signal.py:376 — `pypysig_set_wakeup_fd`.  The OS
             // handler writes the signal-number byte to this fd so a
             // select/poll loop blocked elsewhere wakes up.
-            let prev = signalstate::set_wakeup_fd(fd, warn_on_full_buffer);
+            let prev = signalstate::set_wakeup_fd(fd, warn_on_full_buffer, use_send);
             Ok(pyre_object::w_int_new(prev as i64))
         }),
     );
@@ -1439,14 +1450,12 @@ pub fn register_module(ns: pyre_object::PyObjectRef) {
     // `Signals(2)` answer from here.
     #[cfg(windows)]
     {
-        // `SIGBREAK` is the runtime's own, absent from the libc crate.
-        const SIGBREAK: i64 = 21;
         crate::module_ns_store(ns, "SIGINT", pyre_object::w_int_new(libc::SIGINT as i64));
         crate::module_ns_store(ns, "SIGILL", pyre_object::w_int_new(libc::SIGILL as i64));
         crate::module_ns_store(ns, "SIGFPE", pyre_object::w_int_new(libc::SIGFPE as i64));
         crate::module_ns_store(ns, "SIGSEGV", pyre_object::w_int_new(libc::SIGSEGV as i64));
         crate::module_ns_store(ns, "SIGTERM", pyre_object::w_int_new(libc::SIGTERM as i64));
-        crate::module_ns_store(ns, "SIGBREAK", pyre_object::w_int_new(SIGBREAK));
+        crate::module_ns_store(ns, "SIGBREAK", pyre_object::w_int_new(SIGBREAK.into()));
         crate::module_ns_store(ns, "SIGABRT", pyre_object::w_int_new(libc::SIGABRT as i64));
         crate::module_ns_store(ns, "CTRL_C_EVENT", pyre_object::w_int_new(0));
         crate::module_ns_store(ns, "CTRL_BREAK_EVENT", pyre_object::w_int_new(1));