Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@crates/host_env/src/ctypes.rs`:
- Around line 1883-1904: The function copy_com_pointer currently uses
debug_assert! to validate the IUnknown vtable which is removed in release
builds, causing UB when src_ptr is non-zero but vtable is null or bogus; change
the runtime checks so that after computing iunknown and vtable you explicitly
test if vtable.is_null() (and also check the function pointer at vtable.add(1)
is non-null) and return HRESULT_E_POINTER if invalid, and only then perform the
transmute and call addref_fn; ensure you do not dereference *vtable until those
runtime checks pass and likewise avoid transmuting a null function pointer.
- Around line 375-402: The code in wstring_from_bytes and
wchar_array_field_value treats each WCHAR as an independent Unicode scalar via
char::from_u32, which drops surrogate pairs; instead collect the sequence of u16
code units produced by wchar_from_bytes (using WCHAR_SIZE to read each unit),
stop at a terminating 0, then decode the whole Vec<u16> as UTF-16 using either
String::from_utf16 or std::char::decode_utf16 to produce the String (use
decode_utf16 to gracefully handle invalid/ lone surrogates if you want
replacement chars); update both wstring_from_bytes and wchar_array_field_value
to build a Vec<u16> and run the proper UTF-16 decoder rather than mapping each
code unit to char individually.
- Around line 1981-1991: The function call_result_bytes currently hard-codes the
reported size for CallResult::Value as core::mem::size_of::<i64>(), which can be
wrong on some targets; instead use the actual serialized byte length produced by
to_ne_bytes(). Update call_result_bytes (both CallResult::Pointer and
CallResult::Value branches) to compute the reported width from the length of the
bytes returned by ptr.to_ne_bytes()/val.to_ne_bytes() (e.g., use bytes.len() or
equivalent) and return that length instead of a hard-coded type size.
- Around line 208-210: simple_type_align currently returns simple_type_size
which is wrong for ABIs where alignment != size; replace the forwarding with a
real alignment mapping inside simple_type_align: match on the type strings used
elsewhere (e.g. the same tokens handled by simple_type_size) and return the C
alignment for each token (e.g. i8/u8 => 1, i16/u16 => 2, i32/u32/ f32 => 4,
i64/u64/f64 => 8, ptr types => pointer alignment based on target pointer width,
etc.), returning Some(alignment) for recognized types and None as the fallback
(or use simple_type_size only when appropriate); reference the function name
simple_type_align and reuse simple_type_size only as a fallback, not as the
primary alignment source.
- Around line 1278-1298: The match arms in write_callback_result() (and
similarly in read_value_at_address() at the other noted location) currently
treat "l"/"L" as fixed 64-bit/32-bit types causing truncation/overrun on
ILP32/LP64 targets; change the memory access to use libc::c_long and
libc::c_ulong for the "l" and "L" branches (use *(result as *mut libc::c_long)
and *(result as *mut libc::c_ulong) respectively) and only cast between
CallbackResultValue's integer variants and libc::c_long/c_ulong at the API
boundary so the in-memory read/write respects the platform c_long/c_ulong width.
- Around line 102-115: with_swapped_errno currently writes the ctypes-local
errno into the OS slot, calls f, stores the post-call errno into
CTYPES_LOCAL_ERRNO, but fails to restore the original OS errno so the embedding
thread sees the foreign function's errno; fix by capturing the original OS errno
before setting it (save_old_os = crate::os::get_errno()), set the OS errno to
the ctypes-local value as now, call f, capture new_error =
crate::os::get_errno(), restore the original OS errno
(crate::os::set_errno(save_old_os)), then set CTYPES_LOCAL_ERRNO to new_error
and return result; apply the exact same pattern to the other helper in the
153-165 region (the corresponding swapped LastError helper) using
crate::os::get_last_error()/set_last_error() and the CTYPES_LOCAL_LAST_ERROR
slot.
- Around line 707-775: In read_array_element, guard against slicing
buffer[offset..] before verifying offset and available bytes: for type "u"
return DecodedValue::String(String::new()) if offset >= buffer.len(); for "f"
return DecodedValue::Float(0.0) if offset + 4 > buffer.len(); for "d"/"g" return
DecodedValue::Float(0.0) if offset + 8 > buffer.len(); and in the
integer/default branch use buffer.get(offset..) (or check offset + element_size
<= buffer.len()) before calling get(..element_size) and int_from_bytes so you
return the existing Signed(0) fallback instead of panicking; update the checks
around usages of wchar_from_bytes, first_chunk::<4>(), first_chunk::<8>(), and
int_from_bytes accordingly.
- Around line 172-184: LONG_DOUBLE_SIZE and the "g" format handlers are
inconsistent: LONG_DOUBLE_SIZE should represent the platform long double size
(16 bytes on x86_64/aarch64 Unix) and the "g" encoder/decoder and FFI mapping
must use the proper long double type instead of marshaling as an 8-byte f64.
Update the LONG_DOUBLE_SIZE constant to reflect c_longdouble size where
appropriate, change the "g" encoding path (the block that currently encodes as
8-byte float then pads) to encode a c_longdouble value of the correct size and
preserve its bytes (no zero-padding/truncation), change the "g" decoding path
(the block that currently reads 8 bytes into c_double) to read the full
c_longdouble size and reconstruct a c_longdouble, and replace any uses of
Type::f64() for the "g"/long double FFI mapping with the proper long double
mapping (Type::longdouble() / libffi ffi_type_longdouble) so libffi receives the
correct ffi_type_longdouble for FFI calls.
- Around line 2409-2416: The get_pointer() implementation in SharedLibrary must
not transmute the safe libloading::Library; instead persist a stable raw handle
or synthetic ID when the library is created and return that here. Modify the
SharedLibrary struct to either (A) capture the raw handle up front by converting
the platform-specific libloading::os::unix::Library to a raw handle via
into_raw()/from_raw() and store that usize (update creation/loading code to call
into_raw and reconstruct the safe wrapper with from_raw), or (B) add a dedicated
stable identifier field (AtomicUsize counter or UUID) assigned when the library
is loaded and return that from get_pointer(). Replace the transmute_copy usage
in get_pointer() to read and return the stored raw_handle or stable_id and
ensure destruction/reconstruction uses from_raw if you chose the raw-handle
approach.
In `@crates/host_env/src/faulthandler.rs`:
- Around line 196-209: The loop in enable_fatal_handlers iterates
FATAL_SIGNAL_HANDLERS and may return false after a failed install_sigaction,
leaving earlier entries enabled; update enable_fatal_handlers to roll back any
installs when a subsequent install fails by iterating the already-enabled
entries and restoring their previous handlers (using entry.previous and the same
mechanism as install_sigaction or its counterpart) and mark them disabled before
returning false; apply the same rollback logic to the analogous code region
(lines ~383-397) so partial installs are undone on failure.
- Around line 269-307: Validate the incoming signum before casting/indexing in
register_user_signal: check that the original signum (libc::c_int) is
non-negative and that (signum as usize) < USER_SIGNAL_CAPACITY; if not, return
an appropriate io::Error (e.g., from_raw_os_error(libc::EINVAL)) instead of
indexing USER_SIGNALS or USER_SIGNAL_CAPACITY. Use the validated usize only
after this check and keep references to USER_SIGNALS, USER_SIGNAL_CAPACITY, and
register_user_signal consistent.
- Around line 347-356: The code currently captures saved_errno after
restore_sigaction and then restores errno using errno_after_raise, which loses
the caller's original errno; move the capture of the caller's errno to before
you call restore_sigaction (call crate::os::get_errno() into saved_errno before
restore_sigaction), continue to call raise_signal and re-install the handler
(install_sigaction), then after re-installation call
crate::os::set_errno(saved_errno) instead of restoring errno_after_raise so the
original errno is preserved; update/remove the temporary errno_after_raise usage
accordingly and keep restore_sigaction, raise_signal, and install_sigaction
calls in the same sequence.
- Line 9: The static mutable FATAL_SIGNAL_HANDLERS must be protected from
concurrent access; replace the current static mut with a synchronization
primitive (e.g., wrap the underlying table in a Mutex or RwLock) and update the
public functions enable_fatal_handlers() and disable_fatal_handlers() to acquire
the lock before reading/modifying the table (or alternatively make those
functions unsafe and document the synchronization contract); ensure you
reference and lock the same guarded static (FATAL_SIGNAL_HANDLERS) in all helper
routines that touch it so no aliased mutable references or data races can occur.
In `@crates/host_env/src/io.rs`:
- Around line 216-218: The is_would_block_error function currently only checks
raw_os_error() == Some(libc::EAGAIN); change it to detect would-block using
Rust's platform-agnostic io::ErrorKind by checking err.kind() ==
io::ErrorKind::WouldBlock (and optionally OR with the existing raw_os_error() ==
Some(libc::EAGAIN) if you want to preserve the explicit errno check). Update the
function signature/reference to use std::io::ErrorKind so is_would_block_error
reliably returns true across platforms.
In `@crates/host_env/src/posix.rs`:
- Around line 457-468: The utimes function's tv closure incorrectly uses
d.as_micros() which gives total microseconds instead of the fractional
microseconds for timeval.tv_usec; update the tv closure in utimes so tv_sec
remains d.as_secs() as _ and tv_usec is the subsecond component (use
d.subsec_micros() or d.subsec_nanos()/1_000) cast to the correct type, ensuring
timeval.tv_usec contains only the microsecond remainder; adjust only the tv
closure inside the utimes function (references: utimes, tv closure,
timeval.tv_sec and timeval.tv_usec).
In `@crates/host_env/src/select.rs`:
- Around line 267-277: The function wait clears events then calls
rustix::event::epoll::wait which writes into events' spare capacity but doesn't
update events.len(); update events to reflect the number of entries written
before returning. Specifically, in the Ok(n) arm of select::wait (the function
named wait, operating on events: &mut Vec<Event> and calling
rustix::event::epoll::wait), set the vector length to n (e.g., advance or
set_len) so callers see the populated entries, then return Ok(n).
- Around line 86-88: Check the FD_SETSIZE bound before modifying set.__nfds to
avoid temporarily leaving the struct inconsistent: when adding a file
descriptor, ensure you validate n = set.__nfds against FD_SETSIZE (e.g., if n >=
FD_SETSIZE return an error) before doing set.__nfds = n + 1 and set.__fds[n] =
fd; in other words, perform the FD_SETSIZE check first and only increment __nfds
and write to __fds after the bound check succeeds (referencing set.__nfds,
set.__fds, and FD_SETSIZE).
In `@crates/host_env/src/socket.rs`:
- Around line 530-531: Replace the incorrect alias of SO_EXCLUSIVEADDRUSE to
SO_REUSEADDR: update the constant SO_EXCLUSIVEADDRUSE so it uses the
Windows-specific numeric value -5 (0xFFFFFFFB) instead of referencing
SO_REUSEADDR; locate the declaration of pub const SO_EXCLUSIVEADDRUSE and change
its value to the correct integer literal for Windows.
In `@crates/host_env/src/time.rs`:
- Around line 138-149: The code calls libc::sysconf(libc::_SC_CLK_TCK) into
tick_for_second and then divides process time fields by it without checking for
non-positive/zero values; guard the syscall result by checking the raw return
(from libc::sysconf) for <= 0 (or error) before converting to f64, return an
Err(std::io::Error::new(...)) on invalid tick count, and only compute
ProcessTimes.user/system/children_user/children_system/elapsed after confirming
tick_for_second is positive; reference the tick_for_second variable, the
libc::sysconf(libc::_SC_CLK_TCK) call, and the ProcessTimes construction to
locate where to add the check and early return.
In `@crates/host_env/src/winapi.rs`:
- Around line 670-678: The branch that detects a SIGINT event has an off-by-one:
when sigint_event is pushed into thread_handles_raw it occupies index
thread_handles_raw.len() - 1, so change the comparison against result to use
WAIT_OBJECT_0 + (thread_handles_raw.len() - 1) instead of WAIT_OBJECT_0 +
thread_handles_raw.len(); update the condition that references sigint_event,
thread_handles_raw and result (and KEEP using WAIT_OBJECT_0 and
ERROR_CONTROL_C_EXIT) so it correctly matches the signaled index for the SIGINT
handle.
---
Outside diff comments:
In `@crates/host_env/src/msvcrt.rs`:
- Around line 42-54: The getwch and getwche functions currently call
char::from_u32(...).unwrap(), which can panic if the underlying unsafe calls
_getwch() or _getwche() return an invalid Unicode scalar (e.g., surrogate
values); change both to handle invalid code points by using
char::from_u32(value).unwrap_or(char::REPLACEMENT_CHARACTER) (or equivalent
fallback) before calling to_string(), and keep getche as-is (it returns raw u8)
or document its behavior; ensure you reference the unsafe calls _getwch and
_getwche and the wrapper functions getwch and getwche when making the change.
---
Minor comments:
In `@crates/host_env/src/nt.rs`:
- Around line 1546-1559: The getlogin function currently calls
OsString::from_wide(...).to_str().unwrap() which can panic if the username
contains non-UTF-8 sequences; change the conversion to a loss-tolerant form
(e.g., use OsString/OsStr -> to_string_lossy().into_owned()) so invalid UTF-8 is
replaced rather than panicking. Locate the getlogin function and replace the
to_str().unwrap().to_string() chain with a to_string_lossy().into_owned() call
on the OsString/OsStr produced from OsString::from_wide(&buffer[..(size - 1) as
usize]) to return a safe String.
In `@crates/host_env/src/overlapped.rs`:
- Line 536: The current code uses unwrap() on ACCEPT_EX.get() (and similar
CONNECT_EX.get(), etc.), which will panic if initialize_winsock_extensions() was
not called; change the API to return an io::Result (e.g., change the function
signature to io::Result<u32>) and replace unwrap() with a checked lookup that
returns Err(io::Error::new(...)) when the extension pointer is None, or
alternatively call initialize_winsock_extensions() lazily and return an error on
failure; reference ACCEPT_EX.get(), CONNECT_EX.get(), and
initialize_winsock_extensions() when making the change so the caller-visible
precondition is removed and errors are propagated instead of panicking.
In `@crates/host_env/src/signal.rs`:
- Around line 299-303: The write call is casting wakeup_fd to the C int
parameter which can truncate on 64-bit Windows; change the unsafe
libc::write(wakeup_fd as _, &sigbyte... ) usage to first attempt a safe
conversion like i32::try_from(wakeup_fd) and only call libc::write with the
converted fd, handling the Err case (e.g., skip the write and log or return) to
avoid truncation; reference the wakeup_fd, sigbyte, and the libc::write
invocation when making this change.
In `@crates/host_env/src/winapi.rs`:
- Around line 627-636: The CreateThread call currently passes a stack size of 1
(in the call that creates the thread for batch_wait_thread), which is
non-idiomatic and suspicious; change the third argument to 0 so the thread uses
the process/executable default stack size (i.e., replace the literal 1 with 0 in
the CreateThread invocation that passes batch_wait_thread and
Arc::as_ptr(data)).
In `@crates/host_env/src/windows.rs`:
- Around line 133-144: The slice used to build service_pack omits the last
non-null wchar because `last` is the index of that character; change the slice
passed to OsString::from_wide to include that character (use last + 1) when
calling from_wide on version.szCSDVersion so service_pack contains the full
string; update the block that computes service_pack (variables `last`, `sp`, and
the call to OsString::from_wide) to handle empty cases safely and use
`&version.szCSDVersion[..last + 1]` instead of `&version.szCSDVersion[..last]`.
In `@crates/host_env/src/wmi.rs`:
- Around line 660-661: The code calls CloseHandle(init_event) and
CloseHandle(connect_event) unconditionally; change it to only call CloseHandle
when the corresponding event handle was successfully created by CreateEventW
(i.e., check that init_event and connect_event are not null/INVALID_HANDLE_VALUE
before calling CloseHandle). Update the cleanup logic around the CreateEventW
calls and any early-return paths so each handle is closed only if it was
actually allocated, referencing the symbols CreateEventW, init_event,
connect_event, and CloseHandle to locate the relevant code.
---
Duplicate comments:
In `@crates/host_env/src/syslog.rs`:
- Around line 46-49: The TOCTOU bug occurs because closelog() calls is_open()
before acquiring the global_ident() write lock, allowing a concurrent openlog()
to race; modify closelog() to acquire the write lock on global_ident() first
(e.g., let mut locked_ident = global_ident().write()), then check whether the
saved identifier is Some via locked_ident.as_ref() or is_open() logic under that
lock, call unsafe { libc::closelog() } and set *locked_ident = None while still
holding the lock so close and state update happen atomically with respect to
openlog().
In `@crates/host_env/src/time.rs`:
- Around line 264-267: get_tz_info currently ignores the return code of
GetTimeZoneInformation and decodes a zeroed struct on failure; change
get_tz_info to check the return value against TIME_ZONE_ID_INVALID, call
GetLastError() when invalid, and return a Result<WindowsTimeZoneInfo,
std::io::Error> (or another appropriate error type) instead of unconditionally
returning WindowsTimeZoneInfo; update callers of get_tz_info to handle the
Result and propagate or map the error, and ensure the WindowsTimeZoneInfo
construction only happens on a successful GetTimeZoneInformation return.
- Around line 376-384: asctime_from_tm currently only guards against negative
weekday values but still indexes WDAY_NAME with tm.tm_wday (and similarly
MON_NAME with tm.tm_mon) which can panic for tm.tm_wday == 7 or tm.tm_mon > 11;
update asctime_from_tm to validate both tm.tm_wday is within 0..=6 and tm.tm_mon
is within 0..=11 before using WDAY_NAME[tm.tm_wday as usize] and
MON_NAME[tm.tm_mon as usize], and handle out-of-range values the same way the
function handles negative values (reject/return the existing fallback) to
prevent panics.
---
Nitpick comments:
In `@crates/host_env/src/cert_store.rs`:
- Around line 90-124: The enum_crls function only opens a single system store
(defaulting to CurrentUser) causing CRLs from LocalMachine to be missed; modify
enum_crls to mirror enum_certificates by iterating both store locations
(CurrentUser and LocalMachine) when opening the store (either call
CertOpenSystemStoreW twice with the two system store flags or use CertOpenStore
with CERT_SYSTEM_STORE_CURRENT_USER and CERT_SYSTEM_STORE_LOCAL_MACHINE),
enumerate CRLs with CertEnumCRLsInStore for each opened store, push CrlEntry
items (der and encoding_type(crl.dwCertEncodingType)) into the same result Vec,
and ensure each opened store is closed with CertCloseStore; keep using the same
types/functions (enum_crls, CertEnumCRLsInStore, CertCloseStore, CrlEntry,
encoding_type) to locate and update the code.
In `@crates/host_env/src/faulthandler.rs`:
- Around line 163-164: In current_thread_id(), replace the explicit cast "as
u64" on libc::pthread_self() with a generic cast "as _" to avoid tripping
clippy::trivial_numeric_casts while preserving portability (keep the unsafe {
libc::pthread_self() as _ } expression and function signature returning u64);
this mirrors the pattern used elsewhere (e.g., RustPython's _thread) and avoids
introducing a lint suppression.
In `@crates/host_env/src/io.rs`:
- Line 1: The import CStr and the function/variable fd_is_own are only used on
specific targets and should be cfg-gated to avoid cross-target unused-item
lints; wrap use core::ffi::CStr with #[cfg(any(unix, target_os = "wasi"))] (or
the exact cfg used elsewhere) and add #[cfg(not(windows))] or #[cfg(any(unix,
target_os = "wasi"))] to the fd_is_own definition (and similarly guard the code
at the 174-178 region) so the symbols are only compiled on targets that need
them, keeping function names like fd_is_own and usages of CStr unchanged.
In `@crates/host_env/src/mmap.rs`:
- Around line 91-94: The unsafe impls for Send and Sync on NamedMmap lack
documented safety invariants; add a clear comment above the unsafe impls for
NamedMmap that states why sharing across threads is safe (e.g., the internal
Windows HANDLE is thread-safe, the underlying mapping is process-wide, any
mutable access is synchronized or the mapping is immutable/only exposes
atomic-safe access, and Drop does not race with concurrent use), reference the
fields used (the raw HANDLE and mapping pointer/len) and mention any required
caller guarantees; ensure the comment explains that these invariants must hold
for Send/Sync to be sound and update tests or reviewers if there are additional
assumptions about lifetime, ownership, or synchronization.
- Around line 274-284: The two identical functions map_anon should be
consolidated: replace the separate #[cfg(unix)] and #[cfg(windows)] blocks with
a single #[cfg(any(unix, windows))] attribute on one map_anon implementation
that uses MmapOptions::new(), sets len(size), calls map_anon(), and maps to
MappedFile::Write; ensure the function signature pub fn map_anon(size: usize) ->
io::Result<MappedFile> and the internal calls to MmapOptions, map_anon, and
MappedFile::Write remain unchanged.
In `@crates/host_env/src/multiprocessing.rs`:
- Around line 420-434: Replace the conditional debug-only check with a hard
assert so passing a deadline on Apple fails loudly: change the
debug_assert!(deadline.is_none()) in the Apple-specific branch of the code
handling sem_wait/sem_timedwait to assert!(deadline.is_none()) so the invariant
is enforced in release builds; ensure you update the branch that calls unsafe {
libc::sem_wait(handle) } and returns WaitStatus::Acquired /
WaitStatus::Interrupted / WaitStatus::Error(SemError::from_errno(err))
accordingly.
- Around line 199-209: current_thread_id has platform-dependent return types
(unix: u64 via pthread_self, windows: u32 via GetCurrentThreadId) causing an
inconsistent API; change the Windows implementation of current_thread_id to
return u64 as well by casting GetCurrentThreadId() to u64 so both cfg(unix) and
cfg(windows) expose the same u64 signature, and update the function signature
for the Windows variant (and any callers if needed) to match current_thread_id
-> u64 while keeping the unix implementation using pthread_self() as before.
In `@crates/host_env/src/nt.rs`:
- Around line 1082-1127: The two functions path_exists_via_open and
test_file_exists_by_name duplicate CreateFileW/opening logic and the regular
reparse point retry; extract a small helper (e.g., open_check_exists_or_reopen)
that accepts the wide path, initial flags (FILE_FLAG_BACKUP_SEMANTICS | optional
FILE_FLAG_OPEN_REPARSE_POINT), desired access (FILE_READ_ATTRIBUTES), and a
boolean indicating whether to follow links, performs the CreateFileW call, runs
test_file_type_by_handle(handle, TestType::RegularReparsePoint, false), ensures
CloseHandle is always called, and returns true/false indicating existence
(including the retry open with FILE_FLAG_BACKUP_SEMANTICS if a regular reparse
point was observed). Replace the duplicated blocks in path_exists_via_open and
test_file_exists_by_name with calls to this helper.
In `@crates/host_env/src/os.rs`:
- Around line 118-126: The cfg attributes on the two cpu_count functions are
using a confusing double-negation; update the second attribute to the clearer
equivalent by replacing #[cfg(not(any(not(target_arch = "wasm32"), target_os =
"wasi")))] with #[cfg(all(target_arch = "wasm32", not(target_os = "wasi")))] so
cpu_count() implementations read as "non-wasm32 or wasi" (the existing first fn)
and "wasm32 and not wasi" (the second fn) respectively; leave the function names
and bodies unchanged.
In `@crates/host_env/src/overlapped.rs`:
- Around line 74-75: The unsafe impls for Sync and Send on Operation are missing
a SAFETY justification even though Operation contains mutable fields (pending,
completed, read_buffer, write_buffer) that are mutated without internal
synchronization; add a clear `// SAFETY:` comment above `unsafe impl Sync for
Operation {}` and `unsafe impl Send for Operation {}` that describes why sharing
across threads is sound (e.g., external synchronization guarantees,
atomic/lock-free usage, or that only immutable methods are called concurrently),
or alternatively document on the `Operation` type that callers must ensure
exclusive access when invoking mutating methods like those that touch `pending`,
`completed`, `read_buffer`, and `write_buffer`. Include references to the
`Operation` struct and the specific fields/methods so future reviewers can
verify the invariants.
- Around line 1084-1116: The format_message function in overlapped.rs duplicates
windows.rs::format_error_message; remove the duplicated implementation and
replace callers of overlapped::format_message with a call to
windows::format_error_message(error_code) (or move the shared logic into a new
common helper and have both modules call that), ensuring you update imports to
bring windows::format_error_message into scope and delete the
LocalFree/FormatMessageW-specific code in overlapped.rs so only the single
canonical implementation remains.
In `@crates/host_env/src/posix.rs`:
- Around line 401-403: The pipe2 function currently returns nix::Result but
other functions use std::io::Result; change pub fn pipe2(...) ->
nix::Result<...> to pub fn pipe2(...) -> std::io::Result<(std::os::fd::OwnedFd,
std::os::fd::OwnedFd)> and convert the nix error to std::io::Error (e.g., via
.map_err(|e| std::io::Error::new(std::io::ErrorKind::Other, e))) on the
nix::unistd::pipe2(...) call so callers get a consistent std::io::Result from
pipe2.
- Around line 1147-1154: Remove the unnecessary rewinddir call in the Drop
implementation for FdDirStream: in the impl Drop for FdDirStream (the drop
method that currently calls libc::rewinddir and libc::closedir), delete the
libc::rewinddir(self.0.as_ptr()) call and keep only the
libc::closedir(self.0.as_ptr()) inside the unsafe block so the directory stream
is closed without resetting its position first.
- Around line 1564-1569: The helper fn should_keep currently returns true when
an fd should be closed (fd > above && not found in keep), which is
misleading—rename the function to something like should_close or is_closable
(e.g., fn should_close(above: i32, keep: &[BorrowedFd<'_>], fd: i32) -> bool)
and update all call sites that reference should_keep to the new name (the places
where it filters/decides which FDs to close). Keep the implementation identical
but use the new, semantically-correct identifier to avoid confusion.
- Around line 766-768: Change the chdir function to return std::io::Result<()>
instead of nix::Result<()>, and map any nix::Error into an std::io::Error before
returning; specifically, update the signature of chdir and replace the direct
nix::unistd::chdir(cwd) return with something like
nix::unistd::chdir(cwd).map_err(|e|
std::io::Error::new(std::io::ErrorKind::Other, e)) so the module API remains
consistent while preserving the original error information (referencing the
chdir function and nix::unistd::chdir call).
- Around line 801-841: Change the listed functions to return std::io::Result<()>
instead of nix::Result<()>, and convert any nix/libc errors to std::io::Error
before returning; specifically update signatures for setsid_if_needed,
setpgid_if_needed, setgroups_if_needed, setregid_if_needed and
setreuid_if_needed to -> std::io::Result<()>, then map/convert errors from calls
like nix::unistd::setsid(), nix::unistd::setpgid(...),
nix::unistd::setgroups(...), and nix::Error::result(ret) into std::io::Error
(e.g. using io::Error::from_raw_os_error from the errno or io::Error::new with
the nix error message) so the public API is consistent with the rest of the
module.
In `@crates/host_env/src/pwd.rs`:
- Around line 38-40: getpwnam currently swallows errors via .ok().flatten(),
making it inconsistent with getpwuid; change getpwnam signature to return
io::Result<Option<Passwd>> and propagate errors from User::from_name instead of
dropping them. Replace the .ok().flatten() pattern with proper error handling
(e.g., map the Result from User::from_name to io::Result and then map/transpose
into Option<Passwd>) so callers receive Err on underlying failures while still
returning Ok(None) when the user is absent; reference the getpwnam function and
User::from_name and match the error behavior of getpwuid.
In `@crates/host_env/src/select.rs`:
- Around line 183-209: The helper functions search_poll_fd, insert_poll_fd,
get_poll_fd_mut, and remove_poll_fd rely on the vector being sorted by PollFd.fd
but that invariant is implicit; either add a short rustdoc on each function (or
on the module) stating "fds must be sorted by fd" and the consequences, or
better encapsulate the Vec<PollFd> in a small wrapper type (e.g. PollFdList)
that owns the Vec and exposes methods search, insert, get_mut, remove which
maintain/validate the sorted-by-fd invariant internally; update these functions
to be methods on that wrapper (or call its validation) so accidental misuse is
prevented.
In `@crates/host_env/src/signal.rs`:
- Line 336: Replace the magic number 21 used in the signal mapping with the
SIGBREAK constant to improve consistency: locate the mapping or match arm that
currently has the entry `21 => "Break"` and change it to use the previously
defined SIGBREAK symbol (defined on line ~216) so it reads `SIGBREAK =>
"Break"`, ensuring imports/visibility of SIGBREAK are respected.
- Around line 176-192: The syscall result in pidfd_send_signal unnecessarily
casts the libc::syscall return to libc::c_long; remove the redundant cast so the
unsafe block assigns the syscall(...) directly to ret (which is declared as
libc::c_long), updating the assignment in the pidfd_send_signal function to
return the syscall result without the "as libc::c_long" cast.
- Around line 71-72: The call to siginterrupt(signalnum, 1) currently discards
its Result with let _ = ...; add a brief inline comment next to that statement
explaining why errors are intentionally ignored (for example: non-critical
best-effort to set SA_RESTART behavior, older platforms where siginterrupt may
be unavailable, or errors are expected and safe to ignore) so future readers
know this is deliberate; reference the siginterrupt call and the signalnum
variable when adding the comment.
- Around line 204-227: VALID_SIGNALS currently embeds the literal 21 instead of
the named constant SIGBREAK because SIGBREAK is defined below; move the SIGBREAK
declaration above VALID_SIGNALS (or define SIGBREAK before VALID_SIGNALS) and
replace the literal 21 in VALID_SIGNALS with SIGBREAK so the signal list
consistently uses the named constant (refer to VALID_SIGNALS and SIGBREAK to
locate and update the definitions).
- Around line 230-235: The init_winsock function currently requests WinSock 1.1
(WSAStartup called with 0x0101); update the requested version to WinSock 2.2 by
passing 0x0202 (or using MAKEWORD(2,2)) to the WSAStartup call in the
init_winsock block (symbols: init_winsock, WSA_INIT, WSAStartup, wsa_data) so
the code initializes WinSock 2.2 instead of 1.1.
- Around line 35-45: Remove the platform-specific ffi module that declares
unsafe extern "C" getitimer/setitimer and instead use the libc::getitimer and
libc::setitimer symbols uniformly on all Unix targets; locate the mod ffi block
and replace its usage sites (calls referencing ffi::getitimer / ffi::setitimer)
to call libc::getitimer and libc::setitimer directly, ensuring any unsafe blocks
and argument types match the existing calls that already use libc on other Unix
branches (see usages around getitimer and setitimer in this file).
In `@crates/host_env/src/socket.rs`:
- Around line 150-160: The cfg attribute on the socket-related code currently
redundantly lists many Unix target_os values alongside `unix`; replace the long
predicate with a single, maintainable condition—either #[cfg(unix)] or, if you
intend to exclude redox, use #[cfg(all(unix, not(target_os = "redox")))]—by
updating the #[cfg(...)] on the block in this file (the attribute immediately
above the socket-related definitions) to the chosen simplified form.
- Around line 194-206: The functions gai_error_string and h_error_string may
call CStr::from_ptr on a null pointer if libc::gai_strerror or libc::hstrerror
return null; change each function to call the libc function into a local pointer
(e.g., let p = libc::gai_strerror(err)), check p for null (std::ptr::null())
inside the unsafe block, and if null return a safe fallback string like
format!("Unknown gai error: {}", err) (and similarly for h_error_string:
"Unknown herror: {}"); otherwise construct the CStr and return its
to_string_lossy().into_owned() as before.
In `@crates/host_env/src/thread.rs`:
- Around line 4-6: The current_thread_id function casts libc::pthread_self()
with as u64 which triggers Clippy's trivial_numeric_casts on Linux; update the
cast to use as _ so the compiler infers the return type from the function
signature (i.e., change the unsafe block that returns libc::pthread_self() as
u64 to returning libc::pthread_self() as _), keeping the function name
current_thread_id and the libc::pthread_self reference intact.
In `@crates/host_env/src/time.rs`:
- Around line 22-27: The two cfg-annotated definitions of TimeT are identical
and should be collapsed into a single unconditional type alias (remove
#[cfg(unix)]/#[cfg(windows)] around TimeT); do the same for the identical cfg
branches that implement current_time_t and the other duplicated block around
lines ~73-77 by extracting any platform-specific values only and invoking the
shared logic once (reference symbols: TimeT and current_time_t) so duplicate
byte-for-byte branches are eliminated.
In `@crates/host_env/src/winapi.rs`:
- Around line 578-579: Add a SAFETY comment above the unsafe impls for BatchData
explaining why implementing Send and Sync is sound: note that BatchData contains
raw handles and an UnsafeCell, and document the required invariants (e.g., all
mutable writes to BatchData occur-before the thread is resumed, and all reads
occur-after the thread completes/joins; no concurrent access occurs; ownership
of raw handles is not aliased across threads). Reference the symbols BatchData
and the unsafe impl Send/Sync to make clear which types and impls the comment
applies to.
In `@crates/host_env/src/winreg.rs`:
- Around line 89-96: The function bytes_as_wide_slice relies on a debug-only
assert and must be made explicit about alignment guarantees; change
bytes_as_wide_slice to an unsafe fn bytes_as_wide_slice(bytes: &[u8]) -> &[u16]
and add a doc comment stating callers must guarantee the input is u16-aligned
and valid UTF-16/registry data, keep or tighten the existing
debug_assert!(prefix.is_empty() && suffix.is_empty(), ...) for debug builds, and
update all call sites to call it within an unsafe block so alignment
responsibility is clearly documented and enforced at the call boundary.
- Around line 421-425: The code handling the initial query_value_ex result masks
real errors by forcing buf_size = 256 when buf_size == 0 even if the call
returned an error other than Foundation::ERROR_MORE_DATA; update the logic in
the query loop (the block that inspects res and buf_size) to preserve and check
the original res: only treat ERROR_MORE_DATA (or a zero-size success) as a
resize-and-retry case, but if res != 0 && res != Foundation::ERROR_MORE_DATA
then return Err(res) immediately (or return the preserved original error)
instead of overwriting buf_size; ensure the condition references the same res
from query_value_ex and that query_value_ex, buf_size, and
Foundation::ERROR_MORE_DATA are the symbols used to locate the change.
In `@crates/host_env/src/wmi.rs`:
- Around line 670-675: The code can underflow when offset is odd (e.g., offset
== 1) because (offset as usize) / 2 - 1 can wrap; update the calculation to be
defensive: compute let bytes = offset as usize, then guard bytes < 2 => return
Ok(String::new()), or use let char_count = bytes / 2 .saturating_sub(1) (or
adjust bytes = bytes.saturating_sub(bytes % 2)) before indexing buffer; apply
this fix where offset, buffer and char_count are computed so you never perform a
subtraction that can underflow.
In `@crates/stdlib/src/mmap.rs`:
- Around line 406-437: The AccessMode-to-host_mmap::AccessMode mapping is
duplicated in multiple places (e.g., the closure that calls host_mmap::map_file
in the mmap constructor and other locations), so add a single conversion method
or impl to centralize it; implement either an impl AccessMode { fn
to_host(&self) -> host_mmap::AccessMode } or impl From<AccessMode> for
host_mmap::AccessMode, then replace the repeated match blocks wherever
AccessMode is converted (references: AccessMode, host_mmap::AccessMode,
host_mmap::map_file) to call the new conversion method or From::from to avoid
repetition.
In `@crates/stdlib/src/openssl.rs`:
- Around line 4107-4137: The code in enum_certificates currently ignores
CertificateEntries.had_open_store returned by
rustpython_host_env::cert_store::enum_certificates and silently returns an empty
list; update enum_certificates to check the returned
CertificateEntries.had_open_store and, if false, raise an OSError via the VM
(e.g. return Err(vm.new_os_error("could not open certificate store")) or
similar) before mapping entries; reference the enum_certificates function, the
CertificateEntries.had_open_store field, and
rustpython_host_env::cert_store::enum_certificates when making the change.