Repository navigation
Fix HighNumberedFd_NotLeakedAfterLoweringRLimit on 32-bit musl - #135867
Conversation
The test declared struct rlimit as two nuint fields, which is 8 bytes on 32-bit platforms. musl's rlim_t is 64 bits on every architecture, so on Alpine arm32 getrlimit writes past the end of the struct, and setrlimit takes its hard limit from the 8 bytes after it, which can make the call fail. Use Interop.Sys.GetRLimit and Interop.Sys.SetRLimit instead. System.Native converts their two-ulong RLimit to and from the platform's struct rlimit. Report errno if either call fails.
|
Azure Pipelines: Successfully started running 3 pipeline(s). 13 pipeline(s) were filtered out due to trigger conditions. There may be pipelines that require an authorized user to comment /azp run to run. |
|
Tagging subscribers to this area: @dotnet/area-system-diagnostics-process |
There was a problem hiding this comment.
🟢 Approval recommended
The focused test-only fix uses established interop, preserves regression coverage, and has no unresolved findings.
0 open findings
What changed in this PR
Fixes the process handle regression test on 32-bit musl by replacing platform-dependent libc declarations with shared System.Native interop.
Changes:
- Uses portable resource-limit types and constants while preserving the handle-leak scenario.
- Reports resource-limit failures with errno-based exceptions.
Validation was reported in the PR; tests were not rerun here.
| File | Description |
|---|---|
| src/libraries/System.Diagnostics.Process/tests/System.Diagnostics.Process.Tests.csproj | Includes shared resource-limit interop. |
| src/libraries/System.Diagnostics.Process/tests/ProcessHandlesTests.Unix.cs | Replaces incompatible declarations and improves failure diagnostics. |
🧠 Review effort: Balanced
adamsitnik
left a comment
There was a problem hiding this comment.
LGTM, big thanks for identifying the issue and providing the fix @pcshrosbree !
|
Thank you for the review and merge! |
|
/backport to release/11.0 |
|
Started backporting to |
…bit musl (#137407) Backport of #135867 to release/11.0. The main fix was contributed by @pcshrosbree. /cc @krwq @pcshrosbree ## Customer Impact - [ ] Customer reported - [x] Found internally The ProcessHandles descriptor-leak test fails during setup on Alpine ARM32, adding an unrelated failure to validation of .NET 11 servicing changes. The test declares `rlim_t` fields as `nuint`, but musl uses 64-bit fields even on 32-bit platforms. This makes its libc `getrlimit`/`setrlimit` declarations incorrect. This backport addresses #136327 by using the existing `System.Native` resource-limit interop, which converts to and from the platform's native structure. It retains the descriptor-leak check rather than disabling ARM32 coverage. Only the test and its project file change; there is no shipped runtime behavior, public API, package or reference-assembly change. ## Regression - [x] Yes - [ ] No Test-only regression introduced by #135753, which added `HighNumberedFd_NotLeakedAfterLoweringRLimit`. The test's assumption that `rlim_t` is pointer-sized does not hold on musl ARM32. This is not a regression in shipped runtime behavior. ## Testing The existing regression test is corrected; no test is disabled or removed. Validation reported on the merged main PR #135867: - Linux x64: 44 ProcessHandles tests passed; the full Process test project reported 675 passed, 6 skipped and 0 failed. - Reintroducing the descriptor-leak bug caused the corrected test to fail, confirming that it still detects the intended product regression. - Alpine ARM32 emulation reproduced the native layout mismatch and verified the resource-limit calls through `System.Native`. This was a focused console repro, not the full Process test on Alpine ARM32 hardware. Backport validation: - [CI build 1677098](https://dev.azure.com/dnceng-public/public/_build/results?buildId=1677098): the `Build linux_musl-arm Debug AllSubsets_CoreCLR_ReleaseRuntimeLibs` job passed, but the overall runtime pipeline failed. Build Analysis is also red; these failures have not been classified in this description update. - [Linker-test build 1677100](https://dev.azure.com/dnceng-public/public/_build/results?buildId=1677100) passed. - A passing result for the corrected test on Alpine ARM32 has not yet been confirmed for this backport. The passing build job alone does not establish that the test passed. - No local build or tests were run for this backport in this session. ## Risk Low. The diff is limited to `ProcessHandlesTests.Unix.cs` and `System.Diagnostics.Process.Tests.csproj`. It replaces test-local libc resource-limit declarations with existing shared interop and preserves the test's leak assertion and the returned hard limit. No shipped product code or dependencies change. The main PR's validation provides evidence that the test still catches descriptor leaks, but the backport's failing CI and Alpine ARM32 test result still need resolution before treating validation as complete. **IMPORTANT**: If this backport is for a servicing release, please verify that: - For .NET 8 and .NET 9: The PR target branch is `release/X.0-staging`, not `release/X.0`. - For .NET 10+: The PR target branch is `release/X.0` (no `-staging` suffix). This PR targets `release/11.0`. ## Package authoring no longer needed in .NET 9 **IMPORTANT**: Starting with .NET 9, you no longer need to edit a NuGet package's csproj to enable building and bump the version. Keep in mind that we still need package authoring in .NET 8 and older versions. Fixes #136327 > [!NOTE] > This PR description was prepared with GitHub Copilot. Co-authored-by: Peter Shrosbree <[email protected]>
Problem:
HighNumberedFd_NotLeakedAfterLoweringRLimit, added in #135753, fails on Alpine.324.Arm32:setrlimitreturns -1 atProcessHandlesTests.Unix.csline 86, in the test's setup. The 68 hits listed in the known-issue report on #135795 when I checked (14:10 UTC on October 15) are all in the Alpine.324.Arm32 job, and all fail at line 86.Cause: the test declares its own
RLimitwith twonuintfields and P/Invokes libc'sgetrlimitandsetrlimitwith it. That struct is 8 bytes on arm32. musl definesrlim_tasunsigned long longon every architecture (include/sys/resource.h), so musl'sstruct rlimitis 16 bytes.getrlimitwrites past the end of the test's struct, andsetrlimittakes its hard limit from the 8 bytes afternewLimit, which can make it fail. On glibc, the plaingetrlimitandsetrlimitsymbols use a 32-bitrlim_ton arm32, so the test's struct matches there.Fix: use
Interop.Sys.GetRLimitandInterop.Sys.SetRLimitfromCommon/src/Interop/Unix/System.Native/Interop.ResourceLimits.cs, as the System.IO.FileSystem and System.IO.FileSystem.Watcher tests already do. System.Native converts their two-ulongRLimitto and from the platform'sstruct rlimit. The resource becomes the PAL'sRLIMIT_NOFILE(9), the skip check compares withInterop.Sys.RLIM_INFINITY, the lowered limit keeps the returnedMaximumLimit, and a failing call throwsWin32Exceptionwith the errno, as the test already does fordup2.Validation
I don't have Alpine arm32 hardware, so I didn't run the test itself there.
clr+libs -rc Release(Debug libraries),System.Diagnostics.Process.Testswithout OuterLoop: the test passes,ProcessHandlesTests44 passed, whole project 681 total, 675 passed, 6 skipped, 0 failed.pal_process.cchanged so the child marks only fds below the current softRLIMIT_NOFILEas close-on-exec (the bug the test was written for, applied to theclose_rangepath), the test fails withAssert.NotEqual() Failure: Strings are equal, as does the test frommain.SystemNative_*RLimitcalls, on the official .NET 10.0.13 runtime underqemu-arm10.0.0 user mode. This reproduces the layout mismatch, not the full CI environment.Emulation results
musl runs used
linux-musl-armin an Alpine 3.22 armv7 root filesystem; glibc runs usedlinux-armin a Debian 12 armhf root filesystem. The container'sRLIMIT_NOFILEwas 4096 (soft) and 524288 (hard), and its root user did not haveCAP_SYS_RESOURCE. Each row attempts to lower the soft limit by 15:nuintfields, libc, resource 7)EPERM), as root and asnobodySystemNative_*RLimit(twoulong, PAL resource 9)SystemNative_*RLimitThe glibc runs read a soft limit of 524288 on the app's first
getrlimitcall. In a buffer filled with0xAB, musl'sgetrlimitchanged 16 bytes and glibc's changed 8. musl'ssetrlimitwith the test's 8-byte layout followed by 8 more bytes failed withEPERMwhen those bytes were all0xAB, failed withEINVALwhen they were 0, and succeeded when they held the real hard limit.This changes only the test and its project file.
Resolves #135795
Note
This PR was prepared with AI assistance (Anthropic Claude and OpenAI Codex), under my direction. I reviewed it before posting.