fix(ext/node): pass uid/gid to spawn and implement process.getgroups#32772
fix(ext/node): pass uid/gid to spawn and implement process.getgroups#32772bartlomieju merged 6 commits intodenoland:mainfrom
Conversation
Two fixes for child_process uid/gid support: 1. `uid` and `gid` options were destructured but not passed to `Deno.Command` in async `spawn()` (they were already passed in `spawnSync`). Also converts `PermissionDenied` errors to `EPERM` and throws synchronously, matching Node.js behavior. 2. Implements `process.getgroups()` using libc `getgroups()` syscall, which was missing entirely.
1440016 to
2518e47
Compare
kajukitli
left a comment
There was a problem hiding this comment.
The PR adds os.getgroups() functionality for Node.js compatibility. The implementation has a critical type safety issue where a Vec<u32> buffer is cast to *mut libc::gid_t, but gid_t may not be 32 bits on all Unix platforms, potentially causing memory corruption. There's also a race condition between the two getgroups calls and silent error handling that could hide system errors.
[HIGH] ext/node/ops/os/mod.rs:269: Type mismatch when casting u32 buffer to libc::gid_t pointer. On some Unix platforms, libc::gid_t may be a different size than u32 (e.g., u16 or u64). The code allocates Vec but casts to *mut libc::gid_t, which could cause memory corruption or incorrect values.
Suggestion: Use Veclibc::gid_t for the buffer, then convert to u32 when returning:
let mut groups: Vec<libc::gid_t> = vec![0; ngroups as usize];followed byOk(groups.iter().map(|&g| g as u32).collect())
[MEDIUM] ext/node/ops/os/mod.rs:265: Race condition: The number of groups can change between the first getgroups(0, null) call and the second call with the buffer. If groups increase, the second call may fail or truncate results.
Suggestion: Consider using a retry loop if the second call indicates more groups than expected, or allocate a larger buffer upfront.
[LOW] ext/node/ops/os/mod.rs:263: Silently returning empty vector on getgroups error hides legitimate system errors from the caller.
Suggestion: Consider returning an error when ngroups < 0 from the first call instead of silently returning an empty vector.
- Use Vec<libc::gid_t> instead of Vec<u32> for the buffer to avoid type mismatch on platforms where gid_t is not u32 - Return errors instead of silently returning empty vec when getgroups syscall fails - Change return type from PermissionCheckError to OsError to support reporting IO errors Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
kajukitli
left a comment
There was a problem hiding this comment.
Two verified findings: a critical bug in the PermissionDenied error handler that breaks the error handling flow by using throw instead of reassigning e, and a medium-severity inconsistency where process.getgroups is set to undefined on Windows instead of being deleted like other process identity methods.
[HIGH] ext/node/polyfills/internal/child_process.ts:583: PermissionDenied handler uses throw instead of reassigning e, breaking the error handling flow. When Deno.errors.NotFound is caught, the code reassigns e = _createSpawnError(...) and falls through to set up stdio streams (stdin/stdout/stderr pipes) before emitting the error asynchronously via queueMicrotask. The new PermissionDenied handler instead does throw _createSpawnError(...), which skips all stdio pipe setup and the deferred error emission. This means the ChildProcess will have null stdin/stdout/stderr when uid/gid permission errors occur, and the error won't be emitted via the normal 'error' event pathway — it will throw synchronously instead.
Suggestion: Change
throw _createSpawnError("EPERM", command, args.slice(1));toe = _createSpawnError("EPERM", command, args.slice(1));to match the pattern used for the ENOENT case, allowing the catch block to fall through and properly set up stdio streams and emit the error asynchronously.
[MEDIUM] ext/node/polyfills/process.ts:859: process.getgroups is set to undefined on Windows via a ternary, meaning 'getgroups' in process returns true and process.getgroups is undefined. This is inconsistent with both Node.js (where the property doesn't exist on Windows) and the other process identity methods in this file (getgid, getuid, getegid, geteuid) which are assigned unconditionally then deleted on Windows around line 1044-1048.
Suggestion: Assign
process.getgroupsunconditionally as() => op_getgroups(), then adddelete process.getgroups;in theif (isWindows)block alongside the other deletions (around line 1044-1048).
- Change `throw` to `e =` in PermissionDenied catch handler so the error flows through existing stdio setup and #_handleError path - Assign process.getgroups unconditionally and delete on Windows, matching getgid/getuid/getegid/geteuid pattern - Use EPERM instead of ENOTSUP for uid/gid on Windows Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Node.js expects ENOTSUP on Windows per test-child-process-uid-gid.js. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Node.js throws EPERM synchronously from spawn() for uid/gid permission errors (unlike ENOENT which is emitted as an error event). The throw is needed for assert.throws() in test-child-process-uid-gid.js to work. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Two fixes for child_process uid/gid support:
uidandgidoptions were destructured but not passed toDeno.Commandin asyncspawn()(they were already passed inspawnSync). Also convertsPermissionDeniederrors toEPERMand throws synchronously, matching Node.js behavior.process.getgroups()using libcgetgroups()syscall, which was missing entirely.