Skip to content

Implement GetCommandLineArgs fallback on Unix - #131431

Open
am11 wants to merge 6 commits into
dotnet:mainfrom
am11:feature/GetCommandLineArgs-Unix
Open

Implement GetCommandLineArgs fallback on Unix#131431
am11 wants to merge 6 commits into
dotnet:mainfrom
am11:feature/GetCommandLineArgs-Unix

Conversation

@am11

@am11 am11 commented Jul 27, 2026

Copy link
Copy Markdown
Member

@azure-pipelines

Copy link
Copy Markdown
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.

@dotnet-policy-service

Copy link
Copy Markdown
Contributor

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

Comment thread src/native/libs/System.Native/pal_process.c Outdated
Comment thread src/native/minipal/getcmdline.h Outdated
Comment thread src/native/minipal/getcmdline.h Outdated
@am11
am11 force-pushed the feature/GetCommandLineArgs-Unix branch 2 times, most recently from d375f27 to fd686e7 Compare July 28, 2026 16:27
@am11
am11 force-pushed the feature/GetCommandLineArgs-Unix branch from fd686e7 to 675263d Compare July 28, 2026 16:33
am11 added 2 commits July 29, 2026 14:12
.
Co-authored-by: Adeel Mujahid <3840695+am11@users.noreply.github.com>
Comment thread src/native/minipal/getcmdline.h
return resultArgv;

#elif defined(__linux__) || defined(__sun)
int fd = open("/proc/self/cmdline", O_RDONLY);

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't see a good reason why this, or really much of anything else in the file, needs to be written in C. I think we could use standard platform pinvokes (or even just opening the proc fs in this case) from C#.

@am11 am11 Aug 3, 2026

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There are number of varied syscalls for each platform; it'd require exposing them all to managed side just to move the implementation there as opposed to having it here next to getexepath.c (which also uses approximately same syscalls) with a single P/Invoke.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We have most of the required syscalls exposed in managed code already. I do not think it would be too bad to expose the rest.

However, I agree with your point that moving the whole implementation to C# would just be more complicated. We had faced similar tradeoff in the native shims. https://github.com/dotnet/runtime/blob/main/docs/coding-guidelines/interop-guidelines.md touches on it: "At first, it seemed that we'd want to use 1:1 names throughout, but it turns out there are many cases where being strictly 1:1 isn't practical."

having it here next to getexepath.c (which also uses approximately same syscalls)

It is similar code, but it is not a good general-purpose functionality that the minipal should be about. I do not think we would expose it at all if we were starting from scratch. The reason for fixing the linked issue is combination of:

  • it works on Windows for historic reasons
  • parity between OSes is good
  • introducing breaking change on Windows to achieve the parity is not worth it

I think it would be better for this to live in the System.Native shim directly as I have mentioned in the other comment.

@am11
am11 requested a review from agocke August 9, 2026 12:59
@jkotas
jkotas requested a lite review from Copilot August 13, 2026 00:42

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Implements a Unix/WASI/browser fallback path for Environment.GetCommandLineArgs() when Environment.s_commandLineArgs isn’t initialized (hosted-library scenario), by adding a System.Native entrypoint that retrieves argv via a new minipal_getcmdline helper and wiring it into CoreLib’s GetCommandLineArgsNative().

Changes:

  • Add minipal_getcmdline and expose it via SystemNative_GetCommandLine / SystemNative_FreeCommandLine.
  • Update CoreLib Unix/Browser implementation to call the new System.Native interop and remove the prior stubbed-out implementation.
  • Broaden the fallback test coverage beyond “Windows CoreCLR only” (still excluding Mono).

Reviewed changes

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

Show a summary per file
File Description
src/native/minipal/getcmdline.h New minipal helper that attempts to obtain argv across multiple Unix-like targets (plus WASI/browser).
src/native/libs/System.Native/pal_process.h Declares new SystemNative_GetCommandLine / SystemNative_FreeCommandLine exports.
src/native/libs/System.Native/pal_process.c Implements the new exports by delegating to minipal_getcmdline.
src/native/libs/System.Native/pal_process_wasi.c Implements the new exports for WASI builds.
src/native/libs/System.Native/entrypoints.c Registers the new exports for System.Native DllImport dispatch.
src/libraries/Common/src/Interop/Unix/System.Native/Interop.GetCommandLine.cs Adds LibraryImport declarations for SystemNative_GetCommandLine / SystemNative_FreeCommandLine.
src/libraries/System.Private.CoreLib/src/System/Environment.UnixOrBrowser.cs Adds a Unix/Browser GetCommandLineArgsNative() implementation that calls the new interop.
src/libraries/System.Private.CoreLib/src/System/Environment.Unix.cs Removes the old stub GetCommandLineArgsNative() implementation.
src/libraries/System.Private.CoreLib/src/System/Environment.Browser.cs Removes the old Browser stub for GetCommandLineArgsNative().
src/libraries/System.Private.CoreLib/src/System.Private.CoreLib.Shared.projitems Includes the new interop source file in CoreLib build items.
src/libraries/System.Runtime/tests/System.Runtime.Extensions.Tests/System/Environment.GetCommandLineArgs.cs Updates conditions so fallback tests run on non-Mono (and parser tests run on Windows non-Mono).
Suppressed comments (1)

src/native/minipal/getcmdline.h:83

  • The /proc/self/cmdline reader doesn't check whether the initial malloc succeeded, and it doesn't handle read() returning -1 (including EINTR). As written, a read error after partial data can silently return a truncated/invalid argv buffer, and a NULL buf would be dereferenced.
    size_t bufSize = 1024;
    char* buf = (char*)malloc(bufSize);
    size_t totalBytes = 0;
    ssize_t bytesRead;

Comment thread src/native/minipal/getcmdline.h
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants