1233275 - Fix IPC process_util_bsd LaunchApp race on environ

Status: RESOLVED FIXED

(Core :: IPC, defect)

Description

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

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.

Comment 1

[Jan Beich]

Attachment: MozReview Request: Bug 1233275 - Copy environment for IPC using NSPR. r?jld

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

Comment 2

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

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 3

[Jan Beich]

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/

Comment 4

[Jan Beich]

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 5

[Jan Beich]

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 6

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

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 7

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/84f912382f78

Comment 8

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/84f912382f78

Comment 9

[Jan Beich]

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 10

[Sylvestre Ledru [:Sylvestre]]

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