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.

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

Feedback: wtc@google.com

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.

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

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

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

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

(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

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

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

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

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

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.

dhylands, is this ready to re-land?

It is. I put it back on m-i yesterday. https://tbpl.mozilla.org/?tree=Mozilla-Inbound&pusher=dhylands@mozilla.com

> This is tempoarary until we can get PR_DuplicateEnvironment landed > into NSPR. Two years later, no sign of PR_DuplicateEnvironment in NSPR.

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

(In reply to Dave Hylands [:dhylands] from comment #22) > glandium - do you know the procedure to get stuff landed in NSPR? I tried > contacting wtc@google.com 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.

The fix for this caused a build regression on Solaris. See bug 1209397 . I believe this is because nspr.def was not updated .

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