773414 - Add a PR_DuplicateEnvironment function

Status: RESOLVED FIXED

(NSPR :: NSPR, defect)

Description

[Dave Hylands [:dhylands]]

We need to be able to create a copy of the environment, in a thread-safe manner. The copy will then be manipulated and passed to functions like execve.

Comment 1

[Dave Hylands [:dhylands]]

Attachment: Bug 773414 - Add PR_DuplicateEnvironment function - v1

This patch just contains the function protoype. I figured we'd get
it reviewed before worring about the implementation.

Comment 2

[Dave Hylands [:dhylands]]

Attachment: Bug 773414 - Add PR_DuplicateEnvironment function.

Feedback: [e-mail]

Comment 3

[Dave Hylands [:dhylands]]

Comment on attachment 641602 [details] [diff] [review]
Bug 773414 - Add PR_DuplicateEnvironment function.

Added clearer description of format of data returned, and which function to use to release the memory.

Comment 4

[Dave Hylands [:dhylands]]

This is a particularly nasty problem for us since we can't do fork/setenv/exec or we occasionally experience deadlock.

Comment 5

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Dave, let's go ahead with this impl.  If we need to temporarily fork our NSPR clone, I think that's probably OK.

Comment 6

[Dave Hylands [:dhylands]]

Attachment: Version which works for Linux

This version works on all platforms which support environ.
Windows XP and newer uses GetEnvironmentStrings
MacOSX uses __NSGetEnviron

Comment 7

[Dave Hylands [:dhylands]]

Attachment: Version which works for linux and returns NULL for non-linux

Comment 8

[Dave Hylands [:dhylands]]

Comment on attachment 642752 [details] [diff] [review]
Version which works for linux and returns NULL for non-linux

This is patch will get us by for now. I think it needs Mac/Windows implementations to be complete.

Comment 9

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Comment on attachment 642752 [details] [diff] [review]
Version which works for linux and returns NULL for non-linux

Ted, please let us know how we should go about temporarily landing this.

Comment 10

[Dave Hylands [:dhylands]]

Attachment: Version of PR_DuplicateEnvironment which doesn't need any changes in NSPR

Comment 11

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Comment on attachment 643142 [details] [diff] [review]
Version of PR_DuplicateEnvironment which doesn't need any changes in NSPR

>diff --git a/mozglue/build/BionicGlue.cpp b/mozglue/build/BionicGlue.cpp

> #include <unistd.h>
> #include <pthread.h>
> #include <vector>
> 
>+#include <string.h>
>+

Nit: headers should look like this

 #include <pthread.h>
 #include <string.h>
 #include <unistd.h>

 #include <vector>
 
>+struct PRLock;
>+typedef struct PRLock PRLock;

We don't need these.

>+typedef PRIntn PRBool;

We don't need this.

>+#define PR_Malloc(n)        malloc(n)

This won't work on win32, but that's OK.

>+extern "C" NS_EXPORT char*

Introduce PR_IMPLEMENT(...)

>+__wrap_PR_GetEnv(const char *var)

>+extern "C" NS_EXPORT char **
>+PR_DuplicateEnvironment(void)
>+{

>+        *d = strdup(*s);

Introduce PL_strdup --- in for a penny, in for a pound ;).

r=me with nits picked.

Comment 12

[Dave Hylands [:dhylands]]

(In reply to Chris Jones [:cjones] [:warhammer] from comment #11)
> Comment on attachment 643142 [details] [diff] [review]
> Version of PR_DuplicateEnvironment which doesn't need any changes in NSPR
> 
...
> >+#define PR_Malloc(n)        malloc(n)
> 
> This won't work on win32, but that's OK.

I don't think anything in this file will work on win32 (since gcc is the only compiler that does the __wrap_ stuff).
I don't understand why malloc wouldn't work on win32.
http://msdn.microsoft.com/en-us/library/6ewkz86d.aspx

> >+extern "C" NS_EXPORT char*
> 
> Introduce PR_IMPLEMENT(...)

When in Rome... I was just doing what the other __wrap_ functions do. PR_IMPLEMENT does exactly the same thing as NS_EXPORT, but its defined in a header I can't #include (mozglue is logically below nspr). To #include any of the PR headers requires prcpucfg.h which is a header generated by nspr, which means mozglue would need to be built after nspr.

> >+__wrap_PR_GetEnv(const char *var)
> 
> >+extern "C" NS_EXPORT char **
> >+PR_DuplicateEnvironment(void)
> >+{
> 
> >+        *d = strdup(*s);
> 
> Introduce PL_strdup --- in for a penny, in for a pound ;).

Even when I was building as part of NSPR, I wasn't able to use PL_strdup (the header is buried in some weird location). All other uses of strdup inside nsprpub/pr use the bare strdup rather than PL_strdup

Comment 13

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

(In reply to Dave Hylands [:dhylands] from comment #12)
> (In reply to Chris Jones [:cjones] [:warhammer] from comment #11)
> > Comment on attachment 643142 [details] [diff] [review]
> > Version of PR_DuplicateEnvironment which doesn't need any changes in NSPR
> > 
> ...
> > >+#define PR_Malloc(n)        malloc(n)
> > 
> > This won't work on win32, but that's OK.
> 
> I don't think anything in this file will work on win32 (since gcc is the
> only compiler that does the __wrap_ stuff).
> I don't understand why malloc wouldn't work on win32.
> http://msdn.microsoft.com/en-us/library/6ewkz86d.aspx
> 

Yep, that's why it's OK.

> > >+extern "C" NS_EXPORT char*
> > 
> > Introduce PR_IMPLEMENT(...)
> 
> When in Rome... I was just doing what the other __wrap_ functions do.

I thought the idea here was to have the verbatim implementation that we'd land in NSPR.  If not, then get rid of the rest of the macro garbage too ;).

Comment 14

[Dave Hylands [:dhylands]]

Attachment: Removes all of the PR macro stuff and goes bare metal

Cleaned up #includes
Decided to remove all of the PR macro ugliness since this isn't going to be exactly what would land in NSPR anyways

Comment 15

[Mike Hommey [:glandium]]

(In reply to Dave Hylands [:dhylands] from comment #12)
> which means mozglue would need to be built after nspr.

which is not possible because nspr is/needs to be linked against mozglue.

Comment 16

[Ed Morley [:emorley]]

Push backed out for XUL Android bustage:
https://tbpl.mozilla.org/?tree=Mozilla-Inbound&rev=41f4bdd4b6bd

https://hg.mozilla.org/integration/mozilla-inbound/rev/7dd75469589b

[And yeah I wish we could just ditch those builds as much as you... :-) ]

Comment 17

[Dave Hylands [:dhylands]]

Attachment: Adds PR_GetEnvLock and wraps PR_GetEnv and PR_SetEnv

Needed to move PR_DuplicateEnvironment up into process_util_linux.cc because android wraps the heap functions. The heap functions in mozglue don't get wrapped so we would up with mismatched alloc/frees.

Comment 18

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

dhylands, is this ready to re-land?

Comment 19

[Dave Hylands [:dhylands]]

It is. I put it back on m-i yesterday.
https://tbpl.mozilla.org/?tree=Mozilla-Inbound&pusher=[e-mail]

Comment 20

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/497967ad20bf

Comment 21

[Mike Hommey [:glandium]]

> This is tempoarary until we can get PR_DuplicateEnvironment landed
> into NSPR.

Two years later, no sign of PR_DuplicateEnvironment in NSPR.

Comment 22

[Dave Hylands [:dhylands]]

glandium - do you know the procedure to get stuff landed in NSPR? I tried contacting [e-mail] at the time and never got a response, so it slid off my radar.

Comment 23

[Mike Hommey [:glandium]]

(In reply to Dave Hylands [:dhylands] from comment #22)
> glandium - do you know the procedure to get stuff landed in NSPR? I tried
> contacting [e-mail] at the time and never got a response, so it slid
> off my radar.

Like for anything else: File a bug, attach a patch, request review to the relevant party. If wtc doesn't review, try ted.

Comment 24

[Julien Pierre]

The fix for this caused a build regression on Solaris.
See bug 1209397 .

I believe this is because nspr.def was not updated .

Comment 25

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

(In reply to Julien Pierre from comment #24)
> The fix for this caused a build regression on Solaris.
> See bug 1209397 .

For historical reference, bug 1132760 (my attempt to upstream this from Gecko to NSPR) is the one that caused the regression seen in bug 1209397.