Closed Bug 773414 Opened 8 years ago Closed 8 years ago

Add a PR_DuplicateEnvironment function

Categories

(NSPR :: NSPR, defect)

x86_64
Linux
defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: dhylands, Assigned: dhylands)

References

Details

Attachments

(1 file, 6 obsolete files)

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.
Blocks: 772734
Assignee: wtc → dhylands
Attachment #641578 - Flags: feedback?(wtc)
Feedback: wtc@google.com
Attachment #641578 - Attachment is obsolete: true
Attachment #641578 - Flags: feedback?(wtc)
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.
Attachment #641602 - Flags: feedback?(wtc)
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.
Attached patch Version which works for Linux (obsolete) — Splinter Review
This version works on all platforms which support environ.
Windows XP and newer uses GetEnvironmentStrings
MacOSX uses __NSGetEnviron
Attachment #641602 - Attachment is obsolete: true
Attachment #641602 - Flags: feedback?(wtc)
Attachment #642740 - Attachment is obsolete: true
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.
Attachment #642752 - Flags: superreview?(ted.mielczarek)
Attachment #642752 - Flags: review?(jones.chris.g)
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.
Attachment #642752 - Flags: review?(jones.chris.g) → review+
Attachment #642752 - Attachment is obsolete: true
Attachment #642752 - Flags: superreview?(ted.mielczarek)
Attachment #643142 - Flags: review?(jones.chris.g)
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.
Attachment #643142 - Flags: review?(jones.chris.g) → review+
(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
Attachment #643254 - Flags: review?(jones.chris.g)
Attachment #643142 - Attachment is obsolete: true
Attachment #643254 - Flags: review?(jones.chris.g) → review+
(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.
Attachment #643254 - Attachment is obsolete: true
Attachment #643483 - Flags: review+
dhylands, is this ready to re-land?
https://hg.mozilla.org/mozilla-central/rev/497967ad20bf
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
> 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.
You need to log in before you can comment on or make changes to this bug.