Fix Unix file descriptor assertion

[Copilot]

The Unix descriptor assertion treated the mutable soft open-file limit as a validity bound.

Changes

• Retain only the non-negative descriptor assertion.
• Add an isolated Unix dup2 regression test that lowers RLIMIT_NOFILE after duplicating a handle, then writes through the resulting SafeFileHandle with RandomAccess.Write.

[azure-pipelines]

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.

[Copilot]

Pull request overview

Fixes an incorrect Unix file-descriptor debug assertion that treated the mutable soft RLIMIT_NOFILE as a validity bound, and adds a Unix regression test covering dup2 + lowering RLIMIT_NOFILE + RandomAccess.Write via SafeFileHandle.

Changes:

• Removes the _SC_OPEN_MAX-based upper-bound assertion from ToFileDescriptor.
• Adds a Unix-only regression test that duplicates a file descriptor, lowers RLIMIT_NOFILE, then writes through the duplicated SafeFileHandle.
• Wires the new test and required Unix interop into the System.Runtime filesystem test project.

Reviewed changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.

File	Description
src/native/libs/Common/pal_utilities.h	Adjusts Unix file-descriptor assertion to avoid using a mutable soft-limit as a validity bound.
src/libraries/System.Runtime/tests/System.IO.FileSystem.Tests/System.IO.FileSystem.Tests.csproj	Adds the new Unix dup2 regression test and required RLIMIT interop file to the test compilation list.
src/libraries/System.Runtime/tests/System.IO.FileSystem.Tests/RandomAccess/Dup2.Unix.cs	Introduces a new Unix regression test exercising dup2 + lowered RLIMIT_NOFILE + RandomAccess.Write.

[adamsitnik]

@copilot please address my feedback

[dotnet-policy-service]

Tagging subscribers to this area: @dotnet/area-system-io
See info in area-owners.md if you want to be subscribed.

[Copilot]

@copilot please address my feedback

Addressed in 0cb66306.

Note

This response was generated by Copilot.

[Copilot]

Pull request overview

Copilot reviewed 3 out of 3 changed files in this pull request and generated no new comments.

Suppressed comments (1)

src/native/libs/Common/pal_utilities.h:83

• ToFileDescriptorUnchecked truncates intptr_t to int. After removing the sysconf(_SC_OPEN_MAX) bound, ToFileDescriptor no longer asserts that the value is representable as an int, so a large fd could silently wrap/truncate in debug builds. Consider keeping an upper-bound assertion that doesn’t depend on the mutable soft limit, e.g. fd <= INT_MAX.

inline static int ToFileDescriptor(intptr_t fd)
{
#ifndef TARGET_WASI // the valid range of file descriptors is probably INT32_MIN <= fd && fd <= INT32_MAX, the negative handles are valid for console.
    assert(0 <= fd);
#endif

[adamsitnik]

@copilot address the feedback

[Copilot]

@copilot address the feedback

Addressed in 3ee4b61d.

Note

This response was generated by Copilot.

[Copilot]

Pull request overview

Copilot reviewed 3 out of 3 changed files in this pull request and generated no new comments.

Suppressed comments (2)

src/native/libs/Common/pal_utilities.h:83

• ToFileDescriptor casts intptr_t to int. The new upper-bound assert uses fd < INT_MAX, but INT_MAX itself is representable by int and the comment above says fd <= INT32_MAX. Using < can trip debug builds for a theoretically valid descriptor value; the check should match the cast range (<= INT_MAX).

#ifndef TARGET_WASI // the valid range of file descriptors is probably INT32_MIN <= fd && fd <= INT32_MAX, the negative handles are valid for console.
    assert(0 <= fd);
    assert(fd < INT_MAX);

src/libraries/System.Runtime/tests/System.IO.FileSystem.Tests/SafeFileHandle/GetFileType.Unix.cs:113

• The test potentially targets low file descriptors when the current soft limit is already small (e.g., 3 => maxAllowedFileDescriptor becomes 2, overwriting stderr via dup2). This can break the RemoteExecutor process (pipes/std* fds) and make the test flaky. Consider raising the soft limit up to the hard limit (capped), and skip when the resulting limit is too small for a safe high descriptor.

                Assert.Equal(0, Interop.Sys.GetRLimit(Interop.Sys.RlimitResources.RLIMIT_NOFILE, out Interop.Sys.RLimit limits));

                limits.CurrentLimit = Math.Min(limits.CurrentLimit, MaximumFileDescriptorLimit);
                Assert.InRange(limits.CurrentLimit, 2UL, (ulong)int.MaxValue);
                Assert.Equal(0, Interop.Sys.SetRLimit(Interop.Sys.RlimitResources.RLIMIT_NOFILE, ref limits));

[adamsitnik]

@copilot address my feedback

[Copilot]

Pull request overview

Copilot reviewed 3 out of 3 changed files in this pull request and generated no new comments.

Suppressed comments (2)

src/native/libs/Common/pal_utilities.h:85

• ToFileDescriptor still allows values < INT_MIN (when TARGET_WASI is defined) to pass the assertions, even though the subsequent cast to int would truncate/produce an invalid descriptor. It would be safer for the debug-time contract to assert the full int range before narrowing.

#ifndef TARGET_WASI // the valid range of file descriptors is probably INT32_MIN <= fd && fd <= INT32_MAX, the negative handles are valid for console.
    assert(0 <= fd);
#endif
    assert(fd <= INT_MAX);

src/libraries/System.Runtime/tests/System.IO.FileSystem.Tests/SafeFileHandle/GetFileType.Unix.cs:113

• This test can end up dup2'ing onto fd 0/1/2 when RLIMIT_NOFILE is very small (e.g., 2 or 3), which can break the remote test process’ stdin/stdout/stderr and make the failure mode noisy/flaky. Guard against too-small limits and skip in that case.

                limits.CurrentLimit = Math.Min(limits.CurrentLimit, MaximumFileDescriptorLimit);
                Assert.InRange(limits.CurrentLimit, 2UL, (ulong)int.MaxValue);
                Assert.Equal(0, Interop.Sys.SetRLimit(Interop.Sys.RlimitResources.RLIMIT_NOFILE, ref limits));

[azure-pipelines]

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.

[adamsitnik]

LGTM

[adamsitnik]

/ba-g Helix Job Monitor timeout is unrelated