Implement Environment.GetEnvironmentVariables for Apple platforms using official API on iOS/tvOS/MacCatalyst #58161

filipnavara · 2021-08-26T08:19:44Z

ghost · 2021-08-26T08:19:52Z

Tagging subscribers to this area:
See info in area-owners.md if you want to be subscribed.

Issue Details

Not for review yet, only trying CI

Author:	filipnavara
Assignees:	-
Labels:	`area-VM-meta-mono`
Milestone:	-

filipnavara · 2021-08-26T11:25:34Z

src/mono/System.Private.CoreLib/src/System/Environment.Unix.Mono.cs

@@ -97,22 +89,57 @@ private static void EnsureEnvironmentCached()
        {
            var results = new Dictionary<string, string>();

-            foreach (string name in GetEnvironmentVariableNames())
+            IntPtr block = Interop.Sys.GetEnviron();


This code is straight copy from NativeAOT.

then I think it would be better to have it in shared location

Generally I would agree.

I started with a minimal fix because I wanted to limit the impact for backporting to .NET 6 branch.

Then I moved it to System.Native based on chat with @akoeplinger (and I agree that it belongs there). Proper sharing of the code with NativeAOT will need few more changes and I wanted to validate the approach first before proceeding with that.

Yeah I think this is fine for now, we can unify with NativeAOT in main later on.

filipnavara · 2021-08-26T12:22:52Z

cc @jkotas This will inevitably clash with NativeAOT. Ideally we'd want to share parts of the code but I am waiting for some feedback about the approach first. It may need to be backported to .NET 6 branch.

…ms using official API

src/mono/System.Private.CoreLib/src/System/Environment.Unix.Mono.cs

jkotas · 2021-08-26T14:46:13Z

cc @jkotas This will inevitably clash with NativeAOT

No worries. It is easy to fixup conflicts like this one.

src/mono/mono/mini/CMakeLists.txt

src/mono/mono/metadata/icall.c

akoeplinger · 2021-08-26T14:43:03Z

src/mono/System.Private.CoreLib/src/System/Environment.Unix.Mono.cs

@@ -97,22 +89,57 @@ private static void EnsureEnvironmentCached()
        {
            var results = new Dictionary<string, string>();

-            foreach (string name in GetEnvironmentVariableNames())
+            IntPtr block = Interop.Sys.GetEnviron();


Yeah I think this is fine for now, we can unify with NativeAOT in main later on.

src/libraries/Native/Unix/System.Native/CMakeLists.txt

src/libraries/Native/Unix/System.Native/pal_environment.m

jkotas · 2021-08-26T16:06:16Z

remove HAVE_NSGETENVIRON

Why not use it when it is available and avoid the memory leak from the other solution?

src/libraries/Native/Unix/System.Native/pal_environment.m

filipnavara · 2021-08-26T16:17:09Z

Why not use it when it is available and avoid the memory leak from the other solution?

_NSGetEnviron is not a public API. It used to fail App Store submissions in past and it's not even in the public headers (anymore? maybe on ARM64 only?).

jkotas · 2021-08-26T16:22:33Z

_NSGetEnviron is not a public API

It is not a public API on Apple device OSes, but I believe that it is de-facto public API on Apple desktop OSes.

filipnavara · 2021-08-26T16:26:37Z

It is not a public API on Apple device OSes, but I believe that it is de-facto public API on Apple desktop OSes.

You are right, it seems to exist on the desktop headers. I incorrectly based the assumption on a 14-year old comment in the Mono code (https://github.com/dotnet/runtime/blame/3f61d42cdfc65cfa1822986a9d257f1558545c4d/src/mono/mono/metadata/icall.c#L6273-L6276).

src/mono/System.Private.CoreLib/src/System/Environment.Unix.Mono.cs

src/libraries/Native/Unix/System.Native/pal_environment.h

Co-authored-by: Alexander Köplinger <[email protected]>

src/libraries/Native/Unix/System.Native/pal_environment.h

vargaz · 2021-08-27T02:06:37Z

Is it useful for ios apps to read their native environment ? Apple generally doesn't like it for security reasons, thats why a lot of these apis were removed. Wouldn't be better to return just the env variables defined by the user/set during the build ?

Co-authored-by: Adeel Mujahid <[email protected]>

filipnavara · 2021-08-27T02:22:11Z

Is it useful for ios apps to read their native environment?

In some cases it is. There are some global variables like HOME that .NET uses (and so did mono/mono). It is also useful for diagnostics and for reading variables that were set through PList files inside the app, or through mlaunch or other external tools.

I would find it highly inconsistent that GetEnvironmentVariable returns something and GetEnvironmentVariables does not which is how legacy Mono behaved.

filipnavara · 2021-08-27T08:40:54Z

Apple generally doesn't like it for security reasons, thats why a lot of these apis were removed.

I am not sure that is the reason for the removal. _NSGetEnviron is inherently not thread safe and it's exported only to support C runtime. Perhaps Apple just decided not to expose the "broken" API for a different reason. Just like they are now trying to push people away from BSD socket API into their own Network.framework.

[[NSProcessInfo processInfo] environment] has been public API since the beginning (iOS 2) and it is not deprecated. That's in stark contrast to _NSGetEnviron which was never a public API on the device platforms to begin with. If Apple wanted to restrict access to environment variables they can easily return empty or filtered dictionaries in the public APIs (not so easy with _NSGetEnviron which exposes some internal structure details).

Wouldn't be better to return just the env variables defined by the user/set during the build ?

There's too many ways to set the environment variables (Info.plist files, simctl tool, mlaunch tool, ios-deploy tool) that trying to emulate it would be poor approximation. I don't think it makes sense if there is public API available. Setting these environment variables may not be important for release workflows but it's one of the few ways that are universally available for passing options for both on-device and simulator diagnostics. It also makes a lot of sense for developer scenarios like unit tests and benchmarks where the filtering options are passed through environment variables [and which are deployed using different tools based on a workflow and device setup]. Aside from the application variables there are few global variables that may be useful (HOME, LOCALE, LANGUAGE, etc.) and which are queried by existing code.

akoeplinger · 2021-08-27T11:00:16Z

/backport to release/6.0

github-actions · 2021-08-27T11:00:28Z

Started backporting to release/6.0: https://github.com/dotnet/runtime/actions/runs/1173975336

github-actions · 2021-08-27T11:02:20Z

@akoeplinger backporting to release/6.0 failed, the patch most likely resulted in conflicts:

$ git am --3way --ignore-whitespace --keep-non-patch changes.patch

Applying: Implement mono_icall_get_environment_variable_names for Apple platforms using official API
Using index info to reconstruct a base tree...
M	src/mono/mono/metadata/icall.c
Falling back to patching base and 3-way merge...
Auto-merging src/mono/mono/metadata/icall.c
CONFLICT (content): Merge conflict in src/mono/mono/metadata/icall.c
error: Failed to merge in the changes.
hint: Use 'git am --show-current-patch=diff' to see the failed patch
Patch failed at 0001 Implement mono_icall_get_environment_variable_names for Apple platforms using official API
When you have resolved this problem, run "git am --continue".
If you prefer to skip this patch, run "git am --skip" instead.
To restore the original branch and stop patching, run "git am --abort".
Error: The process '/usr/bin/git' failed with exit code 128

Please backport manually!

…ng official API (dotnet#58161) Move environment handling from Mono runtime to System.Native Fixes dotnet#58156 Co-authored-by: Alexander Köplinger <[email protected]> Co-authored-by: Adeel Mujahid <[email protected]>

…ng official API (#58161) (#58254) Move environment handling from Mono runtime to System.Native Fixes #58156 Co-authored-by: Filip Navara <[email protected]> Co-authored-by: Adeel Mujahid <[email protected]>

dotnet-issue-labeler bot added the area-VM-meta-mono label Aug 26, 2021

ghost added the community-contribution Indicates that the PR has been added by a community member label Aug 26, 2021

filipnavara commented Aug 26, 2021

View reviewed changes

filipnavara changed the title ~~Implement mono_icall_get_environment_variable_names for Apple platforms using official API~~ Implement Environement.GetEnvironmentVariables for Apple platforms using official API Aug 26, 2021

filipnavara marked this pull request as ready for review August 26, 2021 12:20

filipnavara requested review from imhameed, lambdageek, marek-safar, SamMonoRT, thaystg and vargaz as code owners August 26, 2021 12:20

filipnavara added 2 commits August 26, 2021 15:06

Implement mono_icall_get_environment_variable_names for Apple platfor…

7d697e7

…ms using official API

Move environment handling from Mono runtime to System.Native

33e5b5d

filipnavara force-pushed the env-macos-ios branch from 22ee5be to 33e5b5d Compare August 26, 2021 13:06

marek-safar added area-PAL-libraries and removed area-VM-meta-mono labels Aug 26, 2021

danmoseley reviewed Aug 26, 2021

View reviewed changes

src/mono/System.Private.CoreLib/src/System/Environment.Unix.Mono.cs Show resolved Hide resolved

filipnavara changed the title ~~Implement Environement.GetEnvironmentVariables for Apple platforms using official API~~ Implement Environment.GetEnvironmentVariables for Apple platforms using official API Aug 26, 2021

akoeplinger requested changes Aug 26, 2021

View reviewed changes

filipnavara added 2 commits August 26, 2021 17:04

Revert unnecessary changes

5eb8dc2

Split pal_environment, remove HAVE_NSGETENVIRON

3346036

filipnavara force-pushed the env-macos-ios branch from 9ba262c to 3346036 Compare August 26, 2021 15:55

jkotas reviewed Aug 26, 2021

View reviewed changes

src/libraries/Native/Unix/System.Native/pal_environment.m Outdated Show resolved Hide resolved

filipnavara added 3 commits August 26, 2021 18:29

Bring back _NSGetEnviron

0e520ab

Remove the memory leak

d9f527b

Fix error with unused variable

d55d4ad

jkotas reviewed Aug 26, 2021

View reviewed changes

src/mono/System.Private.CoreLib/src/System/Environment.Unix.Mono.cs Outdated Show resolved Hide resolved

jkotas reviewed Aug 26, 2021

View reviewed changes

src/mono/System.Private.CoreLib/src/System/Environment.Unix.Mono.cs Outdated Show resolved Hide resolved

filipnavara added 2 commits August 26, 2021 20:03

Add try-finally block

30b72b4

Match Windows behavior for duplicate keys

8759796

jkotas approved these changes Aug 26, 2021

View reviewed changes

akoeplinger approved these changes Aug 26, 2021

View reviewed changes

src/libraries/Native/Unix/System.Native/pal_environment.h Outdated Show resolved Hide resolved

Update src/libraries/Native/Unix/System.Native/pal_environment.h

64744d6

Co-authored-by: Alexander Köplinger <[email protected]>

am11 reviewed Aug 26, 2021

View reviewed changes

src/libraries/Native/Unix/System.Native/pal_environment.h Outdated Show resolved Hide resolved

Update src/libraries/Native/Unix/System.Native/pal_environment.h

316b2f1

Co-authored-by: Adeel Mujahid <[email protected]>

akoeplinger approved these changes Aug 27, 2021

View reviewed changes

akoeplinger merged commit 3e8e0b8 into dotnet:main Aug 27, 2021

akoeplinger changed the title ~~Implement Environment.GetEnvironmentVariables for Apple platforms using official API~~ Implement Environment.GetEnvironmentVariables for Apple platforms using official API on iOS/tvOS/MacCatalyst Aug 27, 2021

akoeplinger mentioned this pull request Aug 27, 2021

[release/6.0] Implement Environment.GetEnvironmentVariables for Apple platforms using official API for iOS/tvOS/MacCatalyst #58254

Merged

ghost locked as resolved and limited conversation to collaborators Sep 26, 2021

jeffhandley added area-System.Runtime.InteropServices and removed area-PAL-libraries labels Nov 11, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement Environment.GetEnvironmentVariables for Apple platforms using official API on iOS/tvOS/MacCatalyst #58161

Implement Environment.GetEnvironmentVariables for Apple platforms using official API on iOS/tvOS/MacCatalyst #58161

filipnavara commented Aug 26, 2021 •

edited

Loading

ghost commented Aug 26, 2021

filipnavara Aug 26, 2021

marek-safar Aug 26, 2021

filipnavara Aug 26, 2021

akoeplinger Aug 26, 2021

filipnavara commented Aug 26, 2021

jkotas commented Aug 26, 2021

akoeplinger Aug 26, 2021

jkotas commented Aug 26, 2021

filipnavara commented Aug 26, 2021

jkotas commented Aug 26, 2021

filipnavara commented Aug 26, 2021

vargaz commented Aug 27, 2021

filipnavara commented Aug 27, 2021 •

edited

Loading

filipnavara commented Aug 27, 2021 •

edited

Loading

akoeplinger commented Aug 27, 2021

github-actions bot commented Aug 27, 2021

github-actions bot commented Aug 27, 2021

Implement Environment.GetEnvironmentVariables for Apple platforms using official API on iOS/tvOS/MacCatalyst #58161

Implement Environment.GetEnvironmentVariables for Apple platforms using official API on iOS/tvOS/MacCatalyst #58161

Conversation

filipnavara commented Aug 26, 2021 • edited Loading

ghost commented Aug 26, 2021

filipnavara Aug 26, 2021

Choose a reason for hiding this comment

marek-safar Aug 26, 2021

Choose a reason for hiding this comment

filipnavara Aug 26, 2021

Choose a reason for hiding this comment

akoeplinger Aug 26, 2021

Choose a reason for hiding this comment

filipnavara commented Aug 26, 2021

jkotas commented Aug 26, 2021

akoeplinger Aug 26, 2021

Choose a reason for hiding this comment

jkotas commented Aug 26, 2021

filipnavara commented Aug 26, 2021

jkotas commented Aug 26, 2021

filipnavara commented Aug 26, 2021

vargaz commented Aug 27, 2021

filipnavara commented Aug 27, 2021 • edited Loading

filipnavara commented Aug 27, 2021 • edited Loading

akoeplinger commented Aug 27, 2021

github-actions bot commented Aug 27, 2021

github-actions bot commented Aug 27, 2021

filipnavara commented Aug 26, 2021 •

edited

Loading

filipnavara commented Aug 27, 2021 •

edited

Loading

filipnavara commented Aug 27, 2021 •

edited

Loading