The BSD version of LaunchApp uses posix_spawn, which means it needs to construct the child's environment beforehand (instead of doing fork/setenv/exec, which is POSIX-incompliant due to setenv being async-signal-unsafe but happens to work on some platforms). It uses the global variable environ to iterate over the current environment, which is thread-unsafe if another thread can mutate the environment. This race might be unlikely in practice, but we were careful to avoid it on Linux (bug 773414), so it's probably worth doing the same on BSD. NSPR has thread-safe environment wrappers, which as of bug 1132760 include PR_DuplicateEnvironment (which creates a dynamically allocated deep copy of the environment), and Gecko already depends on a new enough version of NSPR (because BSD is not Solaris and therefore bug 1209397 doesn't apply). Also, the process_util_bsd implementation of environment merging looks smaller and simpler than the process_util_linux one, so I'd suggest moving it into process_util_posix and using it in both.

Review commit: https://reviewboard.mozilla.org/r/39621/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/39621/

Comment on attachment 8729959 [details] MozReview Request: Bug 1233275 - Copy environment for IPC using NSPR. r?jld https://reviewboard.mozilla.org/r/39621/#review36417 ::: ipc/chromium/src/base/process_util_bsd.cc:78 (Diff revision 1) > if (combined_env_vars.find(varName) == combined_env_vars.end()) { > combined_env_vars[varName] = varValue; > } > pos++; > } > + PR_Free(environ); This frees the array but leaks the individual strings; they also need to be PR_Free()d after the std::string ctor copies them.

Comment on attachment 8729959 [details] MozReview Request: Bug 1233275 - Copy environment for IPC using NSPR. r?jld Review request updated; see interdiff: https://reviewboard.mozilla.org/r/39621/diff/1-2/

PR_DuplicateEnvironment() doesn't fix -Wl,-z,defs issue. Linux builds fine because glibc provides environ but as a weak symbol. I keep forgetting the difference. uxproces.o: In function `_MD_CreateUnixProcess': nsprpub/pr/src/md/unix/uxproces.c:(.text+0x79): undefined reference to `environ' prenv.o: In function `PR_DuplicateEnvironment': nsprpub/pr/src/misc/prenv.c:(.text+0x1ad): undefined reference to `environ'

Comment on attachment 8729959 [details] MozReview Request: Bug 1233275 - Copy environment for IPC using NSPR. r?jld Review request updated; see interdiff: https://reviewboard.mozilla.org/r/39621/diff/2-3/

Comment on attachment 8729959 [details] MozReview Request: Bug 1233275 - Copy environment for IPC using NSPR. r?jld https://reviewboard.mozilla.org/r/39621/#review39011

Comment on attachment 8729959 [details] MozReview Request: Bug 1233275 - Copy environment for IPC using NSPR. r?jld [Approval Request Comment] ESR consideration: Minor security enhancement User impact if declined: Possible race when copying environment Fix Landed on Version: Firefox 48 Risk to taking this patch (and alternatives if risky): NPOTB String or UUID changes made by this patch: None

Comment on attachment 8729959 [details] MozReview Request: Bug 1233275 - Copy environment for IPC using NSPR. r?jld Doesn't match the esr criteria. Should be a sec-high or sec-critical