Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Use native Windows API for Unix.{getenv,putenv,environment}, Sys.getenv #1479

Merged
merged 5 commits into from Dec 15, 2017

Conversation

Projects
None yet
4 participants
@nojb
Copy link
Contributor

nojb commented Nov 14, 2017

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

@nojb nojb force-pushed the nojb:use_native_windows_getenv branch 2 times, most recently from d654829 to cb558fd Nov 14, 2017

@alainfrisch

This comment has been minimized.

Copy link
Contributor

alainfrisch commented Nov 15, 2017

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

This comment has been minimized.

Copy link
Contributor Author

nojb commented Nov 15, 2017

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

@dra27

This comment has been minimized.

Copy link
Contributor

dra27 commented Nov 15, 2017

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

@nojb

This comment has been minimized.

Copy link
Contributor Author

nojb commented Nov 15, 2017

I took a quick glance just now.

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

This comment has been minimized.

Copy link
Contributor

alainfrisch commented Nov 15, 2017

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

This comment has been minimized.

Copy link
Contributor

dra27 commented Nov 15, 2017

@nojb - which adaptation do you mean?

@nojb

This comment has been minimized.

Copy link
Contributor Author

nojb commented Nov 15, 2017

@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

This comment has been minimized.

Copy link
Contributor

dra27 commented Nov 15, 2017

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

This comment has been minimized.

Copy link
Contributor Author

nojb commented Nov 15, 2017

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

@alainfrisch

This comment has been minimized.

Copy link
Contributor

alainfrisch commented Nov 15, 2017

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

This comment has been minimized.

Copy link
Contributor

dra27 commented Nov 15, 2017

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).

@@ -731,7 +731,43 @@ int caml_snprintf(char * buf, size_t size, const char * format, ...)
wchar_t *caml_secure_getenv (wchar_t const *var)
{
/* Win32 doesn't have a notion of setuid bit, so getenv is safe. */
return CAML_SYS_GETENV (var);
return _wgetenv(var);

This comment has been minimized.

@xavierleroy

xavierleroy Nov 20, 2017

Contributor

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.

This comment has been minimized.

@nojb

nojb Nov 20, 2017

Author Contributor

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.

This comment has been minimized.

@xavierleroy

xavierleroy Nov 20, 2017

Contributor

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.

This comment has been minimized.

@nojb

nojb Nov 20, 2017

Author Contributor

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

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

This comment has been minimized.

@xavierleroy

xavierleroy Nov 20, 2017

Contributor

Add a line with the name of the new author.

This comment has been minimized.

@nojb

nojb Nov 20, 2017

Author Contributor

Thanks, fixed.

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

This comment has been minimized.

@xavierleroy

xavierleroy Nov 20, 2017

Contributor

Update years: 1998, 2017

@nojb nojb force-pushed the nojb:use_native_windows_getenv branch from b80bc5d to c2fd60e Nov 20, 2017

@xavierleroy
Copy link
Contributor

xavierleroy left a comment

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

This comment has been minimized.

Copy link
Contributor

alainfrisch commented Nov 24, 2017

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
Copy link
Contributor

dra27 left a comment

_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!

@@ -106,7 +106,7 @@ void caml_parse_ocamlrunparam(void)
case _T('W'): scanmult (opt, &caml_runtime_warnings); break;
}
while (*opt != _T('\0')){
if (*opt++ == ',') break;
if (*opt++ == _T(',')) break;

This comment has been minimized.

@dra27

dra27 Nov 26, 2017

Contributor

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

This comment has been minimized.

@dra27

dra27 Nov 30, 2017

Contributor

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

This comment has been minimized.

@nojb

nojb Nov 30, 2017

Author Contributor

Will do it shortly, thanks.

Changes Outdated
@@ -70,6 +70,10 @@ Working version
- MPR#7668: -principal is broken with polymorphic variants
(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.

This comment has been minimized.

@dra27

dra27 Nov 26, 2017

Contributor

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

@nojb nojb force-pushed the nojb:use_native_windows_getenv branch from 434b42a to 3f94f49 Nov 27, 2017

@nojb

This comment has been minimized.

Copy link
Contributor Author

nojb commented Nov 27, 2017

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.

@@ -18,3 +18,11 @@ CAMLprim value caml_SetEnvironmentVariable(value s1, value s2)
caml_stat_free(w2);
return Val_unit;
}

CAMLprim value stub_putenv(value s)

This comment has been minimized.

@alainfrisch

alainfrisch Nov 29, 2017

Contributor

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

This comment has been minimized.

@nojb

nojb Nov 29, 2017

Author Contributor

No, there isn't, good catch!

@alainfrisch alainfrisch added this to the 4.07 milestone Nov 29, 2017

@alainfrisch

This comment has been minimized.

Copy link
Contributor

alainfrisch commented Nov 29, 2017

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

@nojb

This comment has been minimized.

Copy link
Contributor Author

nojb commented Nov 29, 2017

Yes (I checked).

@dra27

This comment has been minimized.

Copy link
Contributor

dra27 commented Nov 30, 2017

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)?

@dra27

dra27 approved these changes Nov 30, 2017

@nojb

This comment has been minimized.

Copy link
Contributor Author

nojb commented Nov 30, 2017

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

nojb added some commits Oct 11, 2017

@nojb nojb force-pushed the nojb:use_native_windows_getenv branch from cb7e3a7 to 26ec0bf Nov 30, 2017

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

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

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

windows: fix tests for OCaml 4.07
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
workarounds for
 - ocaml/ocaml#1535 (no dll dependency)
 - ocaml/ocaml#1528 (string_of_float format)
 - ocaml/ocaml#1479 (chdir changes environment)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.