1132760 - Move PR_DuplicateEnvironment into upstream NSPR

Status: RESOLVED FIXED

(NSPR :: NSPR, enhancement, P1)

Description

[Jed Davis [:jld] ⟨⏰|UTC-8⟩ ⟦he/him⟧]

For bug 986397 I'm going to need to remove the remaining mallocs from the fork→exec path in the IPC code, which means extending PR_DuplicateEnvironment to other platforms, which means getting it properly integrated into NSPR.

Comment 1

[Jed Davis [:jld] ⟨⏰|UTC-8⟩ ⟦he/him⟧]

Attachment: Gecko patch to move PR_DuplicateEnvironment into nsprpub/

This patch is not landable as it stands, because as I understand it we can't just scribble on nsprpub like that.  I gather that I'd need to break out that part, get it into NSPR, then once that's imported back into mozilla-central (and the minimum version in configure.in is touched) apply the rest of the patch.  But I'm not sure what happens to the Android and B2G builds during the window when there are two versions of PR_DuplicateEnvironment.  Advice on juggling all of this would be welcome.

Also, I basically copied the implementation as-is for the purpose of this patch, but I wouldn't be opposed to doing some style cleanup for the final version.

Comment 2

[(not currently active) Ted Mielczarek]

I haven't looked seriously at the patch yet, but what you can do is get the NSPR part landed, get the Gecko patch reviewed, then land the NSPR update and the Gecko patch as a single commit to m-c.

Comment 3

[Jed Davis [:jld] ⟨⏰|UTC-8⟩ ⟦he/him⟧]

(In reply to Jed Davis [:jld] from comment #1)
> But I'm not sure what happens to the Android and B2G
> builds during the window when there are two versions of
> PR_DuplicateEnvironment.

One way to be more sure: try it.  It turns out to be harmless, because what process_util_linux.cc defines is ::base::PR_DuplicateEnvironment, which coexists peacefully with ::PR_DuplicateEnvironment.

Comment 4

[Jed Davis [:jld] ⟨⏰|UTC-8⟩ ⟦he/him⟧]

Attachment: NSPR patch to add PR_DuplicateEnvironment

A patch against NSPR tip, now with some code cleanup and a test case.  Tested on GNU/Linux (Ubuntu 14.04), OS X (10.10.2), and FreeBSD (10.1).

Comment 5

[Jed Davis [:jld] ⟨⏰|UTC-8⟩ ⟦he/him⟧]

This doesn't work on older versions of OS X (e.g., whichever one is currently used for Mac Firefox build automation); the environ(7) man page notes:

> Shared libraries and bundles don't have direct access to environ, which is only available to
> the loader ld(1) when a complete program is being linked.  The environment routines can still
> be used, but if direct access to environ is needed, the _NSGetEnviron() routine, defined in
> <crt_externs.h>, can be used to retrieve the address of environ at runtime.

This will probably need some help from autoconf.

Comment 6

[Jed Davis [:jld] ⟨⏰|UTC-8⟩ ⟦he/him⟧]

Attachment: NSPR patch to add PR_DuplicateEnvironment [v2]

Now with help from autoconf.  And more complete handling of malloc failure, which doesn't have test coverage because I don't know a good way to do that.

Comment 7

[(not currently active) Ted Mielczarek]

Comment on attachment 8566358 [details] [diff] [review]
NSPR patch to add PR_DuplicateEnvironment [v2]

Review of attachment 8566358 [details] [diff] [review]:
-----------------------------------------------------------------

::: configure.in
@@ +2590,5 @@
> +[],
> +[[#include <crt_externs.h>
> +]])
> +;;
> +esac

We already have an AC_CHECK_HEADER(crt_externs.h) which should be sufficient here. FYI the only place this will fail is targeting iOS (it would be great if you wouldn't break that more because I'm trying to make it work again).

https://dxr.mozilla.org/mozilla-central/source/nsprpub/configure.in#1415

@@ +2607,5 @@
> +     [nspr_cv_have_environ=yes],
> +     [nspr_cv_have_environ=no])])
> +
> +if test "$nspr_cv_have_environ" != "no"; then
> +    AC_DEFINE([HAVE_ENVIRON])

Existing code already assumes environ is available everywhere but Darwin (modulo iOS not having anything usable from C):
https://dxr.mozilla.org/mozilla-central/source/nsprpub/pr/src/md/unix/uxproces.c#157

so I wouldn't bother with any of these configure checks, just do what uxproces.c is doing.

Comment 8

[Jed Davis [:jld] ⟨⏰|UTC-8⟩ ⟦he/him⟧]

(In reply to Ted Mielczarek [:ted.mielczarek] from comment #7)
> We already have an AC_CHECK_HEADER(crt_externs.h) which should be sufficient
> here.

Indeed.  Not sure how I missed that.

> FYI the only place this will fail is targeting iOS (it would be great
> if you wouldn't break that more because I'm trying to make it work again).

iOS wouldn't be using process_util_linux.cc, I don't think, so this shouldn't break any actual use cases; I'll have the NSPR test I'm adding skip it.

> Existing code already assumes environ is available everywhere but Darwin
> (modulo iOS not having anything usable from C):
> https://dxr.mozilla.org/mozilla-central/source/nsprpub/pr/src/md/unix/
> uxproces.c#157

Everywhere that's considered "unix", at least.  I'd have to go back to making PR_DuplicateEnvironment #ifdef PR_UNIX, if I understand correctly.

Comment 9

[Jed Davis [:jld] ⟨⏰|UTC-8⟩ ⟦he/him⟧]

(In reply to Jed Davis [:jld] from comment #8)
> #ifdef PR_UNIX

By which I mean XP_UNIX.  Too many prefixes....

Comment 10

[Jed Davis [:jld] ⟨⏰|UTC-8⟩ ⟦he/him⟧]

(In reply to Ted Mielczarek [:ted.mielczarek] from comment #7)
> We already have an AC_CHECK_HEADER(crt_externs.h) which should be sufficient
> here.

Almost but not quite, it turns out.  It'd need to be something like this:

> AC_CHECK_HEADER(crt_externs.h, AC_DEFINE(HAVE_CRT_EXTERNS_H))

Corollary: whatever the code in uxproces.c is supposed to do on OS X, it's currently not.

Within the NSPR test suite, it seems to be used — by way of PR_ProcessAttrSetInheritableFD() — in "pipeping2" and "sockping"; as expected, both of them fail with "PR_CreateProcess failed" on OS X, but with a fix for the autoconf they fail differently ("expected 5 bytes but got 0 bytes"), and on Linux they fail in a third and also unexplained way ("PR_Write failed: (-5961, 32)"; 32 appears to mean EPIPE).

Comment 11

[Jed Davis [:jld] ⟨⏰|UTC-8⟩ ⟦he/him⟧]

(In reply to Jed Davis [:jld] from comment #10)
> but with
> a fix for the autoconf they fail differently ("expected 5 bytes but got 0
> bytes"), and on Linux they fail in a third and also unexplained way
> ("PR_Write failed: (-5961, 32)"; 32 appears to mean EPIPE).

This means that I didn't build the corresponding *pong executables.  If I do that, then the tests pass on Linux and on OS X with the patch — and still fail on OS X without the patch.

Comment 12

[Jed Davis [:jld] ⟨⏰|UTC-8⟩ ⟦he/him⟧]

Attachment: NSPR patch to add PR_DuplicateEnvironment [v3]

To be applied after the patch in bug 698527.

Comment 13

[(not currently active) Ted Mielczarek]

Comment on attachment 8571694 [details] [diff] [review]
NSPR patch to add PR_DuplicateEnvironment [v3]

Review of attachment 8571694 [details] [diff] [review]:
-----------------------------------------------------------------

Sorry for the delay, this looks good, thanks!

::: pr/src/misc/prenv.c
@@ +118,5 @@
> +    _PR_UNLOCK_ENV();
> +    return result;
> +}
> +#else
> +PR_IMPLEMENT(char **) PR_DuplicateEnvironment(void)

This could probably stand a 1-line comment to the effect of "this platform doesn't support getting the raw environ block".

Comment 14

[(not currently active) Ted Mielczarek]

https://hg.mozilla.org/projects/nspr/rev/a5e5f6d79e24