fix: setgroups(size=0) does not commit cleared groups back to the PCB

[Deepak8858]

Fixes #1838

Calling setgroups(0, NULL) was modifying a local copy of credentials but not writing them back to the PCB. This fix ensures the updated credentials are persisted.

[fslongjin]

@claude review Look at this from an overall perspective to see if it's reasonable.

[dragonos-community-ai-pr-reviewer]

Claude finished @fslongjin's task in 4m 36s —— View job

---

Claude Code Review

I'll review the PR #1852 fix for setgroups(idempotency) from an overall perspective.

Current progress:

Tasks:

• Analyze the PR change and understand the bug
• Review the fix for correctness and potential side effects
• Check for similar patterns in other credential-related syscalls
• Provide overall assessment and recommendations

---

1. Bug Analysis

The bug was in kernel/src/process/syscall/sys_groups.rs in the SysSetGroups::handle() method. When setgroups(0, NULL) is called to clear all supplementary groups, the code path for size == 0 would:

1. Clone the current credentials (let mut cred = (**pcb.cred.lock()).clone())
2. Clear the groups (cred.setgroups(Vec::new()))
3. Missing step: Fail to write the modified credentials back to the PCB

The diff shows the fix adds the missing assignment:

*pcb.cred.lock() = Cred::new_arc(cred);

This ensures the cleared groups are persisted. The bug only affected the size == 0 path; the size > 0 path already had the assignment.

Why this matters: Without the fix, setgroups(0, NULL) would appear to succeed but wouldn't actually clear the supplementary groups, violating POSIX semantics and potentially causing security issues.

2. Fix Correctness Assessment

✅ The fix is correct: It adds the missing credential persistence step, matching the pattern used in the size > 0 path and other credential-modifying syscalls.

Locking considerations:

• The current implementation uses two separate locks: one to clone credentials (pcb.cred.lock()) and another to assign them back.
• This creates a small race window where another thread could modify credentials between the two locks.
• However, this pattern is necessary for the size > 0 path because reading from userspace (UserBufferReader) might sleep and shouldn't be done while holding a spinlock.
• For consistency, the size == 0 path follows the same pattern (clone→drop lock→modify→lock→assign).

Alternative approach: For the size == 0 path only, we could hold the lock across the entire operation since there's no userspace access. However, keeping the same pattern as size > 0 maintains consistency and is acceptable.

Credential handling: The use of Cred::new_arc(cred) correctly creates a new Arc<Cred> with proper self_ref initialization, matching the pattern used throughout the codebase.

Edge cases covered:

• Permission checks (CAP_SETGID) happen before cloning
• Size validation (size > NGROUPS_MAX) returns EINVAL
• User buffer validation failures return appropriate errors
• The fix doesn't introduce any new error paths

Conclusion: The fix correctly addresses the bug without introducing new issues.

3. Similar Patterns in Other Syscalls

I examined all credential-modifying syscalls that use pcb.cred.lock():

• sys_setgid, sys_setuid, sys_setreuid, sys_setresuid, sys_setresgid, sys_setregid, sys_setfsuid, sys_setfsgid

Findings:

• All these syscalls use a single-guard pattern: they hold the lock guard across the entire operation (clone→modify→assign).
• All properly assign the modified credentials back via *guard = Cred::new_arc(new_cred).
• No similar missing-assignment bugs were found.

The sys_groups.rs file is unique in using a two-lock pattern due to the need to read from userspace, which could sleep. This is a reasonable design choice.

4. Overall Assessment and Recommendations

Verdict: ✅ The PR is correct and ready to merge.

The fix addresses a real bug where setgroups(0, NULL) would not persist cleared groups. The implementation is minimal and follows existing patterns in the codebase.

Strengths:

• Minimal change (one line) with low risk
• Correctly matches the pattern used in the size > 0 path
• No side effects or regression risks identified
• Properly closes issue fix(cred): setgroups(size=0) does not commit cleared groups back to the PCB #1838

Potential Improvements (optional):

1. Consider holding the lock for size == 0 path: Since there's no userspace access, the lock could be held across the entire operation to eliminate the race window. This would require restructuring the function slightly but would improve atomicity.
2. Add a comment about the two-lock pattern: A brief comment explaining why the lock is dropped before userspace access could help future maintainers.

Testing considerations: While not required for this PR, adding a unit test for setgroups(0, NULL) would help catch regressions. However, given the simplicity of the fix and that it matches existing patterns, this is not critical.

Final recommendation: Merge this PR as it stands. The fix is correct, minimal, and addresses the reported issue.

---