Use native Windows API for Unix.{getenv,putenv,environment}, Sys.getenv by nojb · Pull Request #1479 · ocaml/ocaml

nojb · 2017-11-14T12:45:56Z

See MPR#4499. Under Windows the C runtime functions getenv, putenv, etc. work on a copy of the "native" environment made at program launch. This means they can get out of sync if the environment is modified using the native Windows API (e.g. when building mixed OCaml/C applications).

This PR works around this issue by using the native Windows calls GetEnvironmentVariable, SetEnvironmentVariable and GetEnvironmentStrings to implement the corresponding OCaml functions ~~(also in the runtime)~~.

One issue is that GetEnvironmentVariable returns a string that needs to be explicitly deallocated, as opposed to getenv. In order to use the new functions in shared Unix/Windows code, all calls needed to be switched to the explicit deallocation style.

~~Concretely, this meant adapting caml_secure_getenv so that it returns a copy of the string and adding calls to caml_stat_free as needed.~~

@alainfrisch @dra27

alainfrisch · 2017-11-15T08:24:13Z

caml_secure_getenv

I'm wondering if we shouldn't rename the function so as to make sure that (i) this PR adapts all use sites and (ii) third-party code is forced to adapt (i.e. release the result).

nojb · 2017-11-15T08:30:29Z

Is caml_secure_getenv (supposed to be) used by third-party code?

dra27 · 2017-11-15T08:31:21Z

Has anyone looked at how other language runtimes solve this one?

nojb · 2017-11-15T10:41:02Z

I took a quick glance just now.

Ruby makes a copy of the whole environment block returned by GetEnvironmentStringsW in each invocation to getenv and puts that in a global variable, so that they do not need to deallocate the returned pointer. See https://github.com/ruby/ruby/blob/trunk/win32/win32.c#L5169-L5200.
Python uses plain getenv in their runtime system. For their library functions they make a copy of the environment at program launch and expose that to the user. This means that even calls to the libc putenv will not be reflected in the user-facing environment. See https://stackoverflow.com/questions/10952507/when-would-os-environfoo-not-match-os-getenvfoo.

Actually in our case, we could just adapt the library functions and leave the runtime system unmodified. It would fix the bug and make the patch much less invasive. And we never had any problem with the runtime system anyway, so there isn't much of a reason to change it.

I will go ahead and make this change as it greatly simplifies the patch.

alainfrisch · 2017-11-15T10:57:23Z

And we never had any problem with the runtime system anyway, so there isn't much of a reason to change it.

Does this mean that when the runtime system itself needs to access environment variables (such as OCAMLRUNPARAM), it would not see changes applied to these variables through Win32 functions from the host program? I agree it's not super important to fix this scenario, but I can imagine cases of an OCaml code base hosted in e.g. a .NET application, and the host application wants to set OCAMLRUNPARAM params before to control the OCaml runtime before its launch.

dra27 · 2017-11-15T11:03:18Z

@nojb - which adaptation do you mean?

nojb · 2017-11-15T11:12:13Z

@alainfrisch Yes, that's it. But in any case the two fixes ("runtime" and "library") are essentially independent, so I think it would simplify to consider them separately.

@dra27 to only modify the library functions (Sys.getenv, Unix.putenv, etc.) and not the runtime (caml_secure_getenv).

dra27 · 2017-11-15T11:13:54Z

Thanks for looking up those two other runtimes, @nojb! The fact that two much larger runtimes are willing to do that is making me wonder if we should just be using the Windows API for reading the environment (no reason not to use _putenv for writing) and provide something like caml_sync_wenviron which exposes the "hack" you've previously described from LexiFi's codebase. We can then recommend that C stubs should be using caml_secure_getenv or caml_getenv because it provides more consistent Windows behaviour, and document caml_sync_wenviron as being necessary for an odd corner case where you're using/wrapping a third party C library which expects _wenviron to be up-to-date?

nojb · 2017-11-15T11:44:21Z

By the way, the Haskell runtime uses plain getenv (see e.g. https://github.com/ghc/ghc/blob/master/rts/RtsFlags.c#L637).

alainfrisch · 2017-11-15T11:46:13Z

Only fixing the stdlib functions (not direct access from the runtime system) seems easy enough. What would be the argument against this approach? Are you concerned by the extra allocation/copy?

dra27 · 2017-11-15T11:53:22Z

I have no problem with just fixing the stdlib/unix functions - my lingering concern is the confusion which may arise from "incorrect" use of environment variables from the C side (this approach "fixes" interop with .NET and anything else which just uses the API, but it doesn't address C libraries which might expect getenv to behave Unix-ly/sensibly).

xavierleroy · 2017-11-20T08:54:17Z

byterun/win32.c

 {
  /* Win32 doesn't have a notion of setuid bit, so getenv is safe. */
-  return CAML_SYS_GETENV (var);
+  return _wgetenv(var);


Why not calling caml_win32_getenv here? This introduces an inconsistency between the secure and the plain versions of getenv, while I thought the consensus is that under Windows they are identical.

The point is that the string returned by caml_win32_getenv needs to be explicitly deallocated, as opposed to the one returned by _wgetenv. If we want to use caml_win32_getenv for caml_secure_getenv we need to change all the call sites of caml_secure_getenv to require explicit deallocation of the string. This is what the first version of this PR did, but it was a more invasive change. Rather, in the current version, we only use caml_win32_getenv for the library functions {Sys,Unix}.getenv and the runtime, which uses caml_secure_getenv, is left unchanged.

As far as I can see there is no inconsistency between secure and plain versions. Rather the inconsistency is between the stdlib versions (caml_sys_getenv, caml_sys_unsafe_getenv, both using caml_win32_getenv) and the one used by the runtime (caml_secure_getenv, uses _wgetenv and there is no separate plain version). We can remove this inconsistency if we shift to an API requiring explicit deallocation of the returned string everywhere.

OK, thanks for the explanation. To help other readers of this code, could you add a comment summarizing this discussion? E.g. :

caml_win32_getenv is used to implement Sys.getenv and Unix.getenv in such a way that they get direct access to the Win32 environment rather than to the copy that is cached by the C runtime system. The result of caml_win32_getenv is dynamically allocated and must be explicitly deallocated.

In contrast, the OCaml runtime system still calls _wgetenv from the C runtime system, directly or through caml_secure_getenv. The result is statically allocated and needs no deallocation.

Good idea. Added your suggested comment with a minor tweak.

xavierleroy · 2017-11-20T08:56:38Z

otherlibs/win32unix/putenv.c

+/*                                 OCaml                                  */
+/*                                                                        */
+/*             Xavier Leroy, projet Cristal, INRIA Rocquencourt           */
+/*                                                                        */


Add a line with the name of the new author.

Thanks, fixed.

xavierleroy · 2017-11-20T08:56:56Z

otherlibs/win32unix/putenv.c

+/*             Xavier Leroy, projet Cristal, INRIA Rocquencourt           */
+/*                                                                        */
+/*   Copyright 1998 Institut National de Recherche en Informatique et     */
+/*     en Automatique.                                                    */


Update years: 1998, 2017

xavierleroy

Thanks for adding the comment. I have no further suggestions to make. As far as I can tell (not being a Windows expert and all), this is good to go.

alainfrisch · 2017-11-24T10:59:40Z

this approach "fixes" interop with .NET and anything else which just uses the API, but it doesn't address C libraries which might expect getenv to behave Unix-ly/sensibly

To be sure to understand your concern: currently, if we bind to a C library that uses getenv, the library will see changes made from OCaml's putenv; with this PR, this would not work any more. Is that right?
So one could argue this is actually a regression in some scenarios.

Would it work to read with GetEnvironmentVariable but set with wputenv? IIRC, wputenv also sends the change to the Win32 environment (with SetEnvironmentVariable) -- if this is not true, one could to both wputenv and SetEnvironmentVariable.

dra27

_wputenv has "always" called SetEnvironmentVariableW (I checked) - I'm really not convinced of the need to alter Unix.putenv at all... it seems better to allow it to keep the C runtime in sync on those calls, surely?

We should document the fact that the C environment and the "OCaml" (i.e. real) environment can become out-of-sync, possibly as a Windows quirk in the "Interfacing with C" chapter (but that doesn't have to go in this GPR). And, as I think I said already, we could include a helper C function which resyncs the C side (but that also doesn't have to be part of this GPR). For C runtimes prior to VS2015 (i.e. pre-UCRT), you just have to free a couple of internal CRT pointers and call _setenvp and _wenviron will be reinitialised. I haven't deciphered the C++ of the UCRT to work out the equivalent call there, but there clearly is one. Or you can do it the way you described at LexiFi and just test what's in _wenviron and what's in the result of GetEnvironmentStringsW and use _wputenv to "write" the changes.

Regarding caml_secure_getenv, I did some digging in our codebase. With two exceptions, caml_secure_getenv is "finished with" prior to the actual OCaml program starting. These two exceptions are:

In instrumented GC mode (which can't at present be enabled on Windows), CAML_INSTR_AT_EXIT reads OCAML_INSTR_FILE. I think this is arguably a bug, and this environment variable should really be read in CAML_INSTR_INIT and the value stored in a static global for CAML_INSTR_ATEXIT to use as otherwise it permits the OCaml program being instrumented to cheekily, if not dangerously, alter the file written to.
In caml_setup_afl. This is a primitive, and I think it should be upgraded to use caml_win32_getenv.

So the only way using caml_secure_getenv can cause inconsistencies/problems at the moment is if _wenviron has already been got out of sync by code called before OCaml starts (e.g. if the runtime is launched from a C program). However, it would be bad (I can't convince myself how bad, but probably not that bad) if future changes used caml_secure_getenv (i.e. _wgetenv) when really they should be switching between caml_secure_getenv on Unix and caml_win32_getenv on Windows at that point. Before @xavierleroy quite reasonably explodes, let's remember that we're dealing with two problems here: nasty Unix-specific suid behaviour on one hand and nasty Windows-specific environment-block inconsistencies on the other, so I don't think we can ever hope to solve both of these in nice #define-looking ways, sadly - I think that occasionally #ifdef soup is inevitable.

I think therefore that caml_secure_getenv on Windows should stop working after caml_sys_init has been called. The only problem with this is caml_main where CAMLSIGPIPE is read just after caml_sys_init. It's not totally clear to me what that variable's all about, but at a glance it does look as though that _beginthread call could go before the call to caml_sys_init above it.

If we've done that, then we could have a belt-and-braces approach to the Windows implementation of caml_secure_getenv and have it do the equivalent of Unix.environment on its first call, which would even allow proper operation in the weird case of an out-of-sync environment before caml_main is called.

Weird as that setup may sound, I think it better to have this window between whichever function has been used to launch the runtime and the call to caml_sys_init where caml_secure_getenv does work on Windows and does accurately reflect the OS environment than to patch the calls to caml_secure_getenv everywhere with Windows-specific #ifdefs both because, as @nojb pointed out, the initial diff is quite invasive but also because we'd have to remember to do that at every point in future, where this way it would "just work" when someone adds a new and exciting startup feature which reads an environment variable for settings!

dra27 · 2017-11-26T16:22:55Z

byterun/startup_aux.c

      }
      while (*opt != _T('\0')){
-        if (*opt++ == ',') break;
+        if (*opt++ == _T(',')) break;


I think this is an unrelated bug in #1200? It should really go in a separate PR.

Just a reminder that this does need to go in a PR, though! Have you pushed a branch for it?

Will do it shortly, thanks.

dra27 · 2017-11-26T16:23:51Z

Changes

  (Jacques Garrigue, report by Jun Furuse)

+- PR#4499, GPR#1479: Use native Windows API to implement Sys.getenv,
+  Unix.getenv, Unix.putenv, and Unix.environment under Windows.


Potentially, Unix.putenv should be removed from this list.

nojb · 2017-11-27T14:15:38Z

Thanks for the comments @dra27! I think I addressed all the changes due for this PR, the others can be done in another one.

Also rebased on trunk and cleaned up the history.

alainfrisch · 2017-11-29T17:44:10Z

testsuite/tests/lib-unix/win-env/stubs.c

  return Val_unit;
 }
+
+CAMLprim value stub_putenv(value s)


Is there a reason to define this function, now that Unix.putenv relies on _wputenv again?

No, there isn't, good catch!

alainfrisch · 2017-11-29T17:47:04Z

Are changes made through _wputenv visible from the non-wide getenv?

nojb · 2017-11-29T20:17:29Z

Yes (I checked).

dra27 · 2017-11-30T10:25:20Z

Yes, I agree that this all looks good, though it would be good to be able to address the other things before 4.07 ... will you be able to (I can take some/all of it on, otherwise)?

nojb · 2017-11-30T10:44:14Z

I have a full plate at the moment, so if you can take the other things on, I would appreciate it 😄. Thanks!

workarounds for - ocaml/ocaml#1535 (no dll dependency) - ocaml/ocaml#1528 (string_of_float format) - ocaml/ocaml#1479 (chdir changes environment)

nojb force-pushed the use_native_windows_getenv branch 2 times, most recently from d654829 to cb558fd Compare November 14, 2017 13:25

xavierleroy reviewed Nov 20, 2017

View reviewed changes

nojb force-pushed the use_native_windows_getenv branch from b80bc5d to c2fd60e Compare November 20, 2017 16:43

xavierleroy approved these changes Nov 21, 2017

View reviewed changes

dra27 requested changes Nov 26, 2017

View reviewed changes

nojb force-pushed the use_native_windows_getenv branch from 434b42a to 3f94f49 Compare November 27, 2017 13:26

alainfrisch reviewed Nov 29, 2017

View reviewed changes

alainfrisch added this to the 4.07 milestone Nov 29, 2017

dra27 approved these changes Nov 30, 2017

View reviewed changes

nojb added 3 commits November 30, 2017 13:37

{Sys,Unix}.getenv: use GetEnvironmentVariable on Windows

5783424

Unix.environment: Use GetEnvironmentStrings on Windows

1855ae5

Testsuite

60c2660

nojb added 2 commits November 30, 2017 13:38

Changes

12c46c9

Comment

26ec0bf

nojb force-pushed the use_native_windows_getenv branch from cb7e3a7 to 26ec0bf Compare November 30, 2017 12:38

xavierleroy merged commit d8b5449 into ocaml:trunk Dec 15, 2017

fdopen added a commit to fdopen/ocaml-ctypes that referenced this pull request Jun 24, 2018

windows: fix tests for OCaml 4.07

7fc260a

workarounds for - ocaml/ocaml#1535 (no dll dependency) - ocaml/ocaml#1528 (string_of_float format) - ocaml/ocaml#1479 (chdir changes environment)

fdopen added a commit to fdopen/ocaml-ctypes that referenced this pull request Dec 6, 2018

windows: fix issues introduced by OCaml 4.07

4750aef

workarounds for - ocaml/ocaml#1535 (no dll dependency) - ocaml/ocaml#1528 (string_of_float format) - ocaml/ocaml#1479 (chdir changes environment)

vicuna mentioned this pull request Mar 14, 2019

Changes to the environment are invisible to Sys.getenv #4499

Closed

pitrou mentioned this pull request Sep 26, 2020

Runtime environment variable changes ignored on Windows aws/aws-sdk-cpp#1476

Closed

2 tasks

Conversation

nojb commented Nov 14, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

alainfrisch commented Nov 15, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

nojb commented Nov 15, 2017

Uh oh!

dra27 commented Nov 15, 2017

Uh oh!

nojb commented Nov 15, 2017

Uh oh!

alainfrisch commented Nov 15, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

dra27 commented Nov 15, 2017

Uh oh!

nojb commented Nov 15, 2017

Uh oh!

dra27 commented Nov 15, 2017

Uh oh!

nojb commented Nov 15, 2017

Uh oh!

alainfrisch commented Nov 15, 2017

Uh oh!

dra27 commented Nov 15, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

xavierleroy left a comment

Choose a reason for hiding this comment

Uh oh!

alainfrisch commented Nov 24, 2017

Uh oh!

dra27 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nojb commented Nov 27, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

alainfrisch commented Nov 29, 2017

Uh oh!

nojb commented Nov 29, 2017

Uh oh!

dra27 commented Nov 30, 2017

Uh oh!

nojb commented Nov 30, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

nojb commented Nov 14, 2017 •

edited

Loading

alainfrisch commented Nov 15, 2017 •

edited

Loading

alainfrisch commented Nov 15, 2017 •

edited

Loading