Fix IPC process_util_bsd LaunchApp race on environ

RESOLVED FIXED in Firefox 48

Status

()

RESOLVED FIXED
3 years ago
2 years ago

People

(Reporter: jld, Assigned: jbeich)

Tracking

Trunk
mozilla48
Unspecified
FreeBSD
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox46 wontfix, firefox48 fixed, firefox-esr45 affected)

Details

(Whiteboard: [npotb])

Attachments

(1 attachment)

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

Comment 1

3 years ago
Created attachment 8729959 [details]
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/
(Assignee)

Updated

3 years ago
Attachment #8729959 - Flags: review?(jld)
Attachment #8729959 - Flags: review?(jld)
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.
(Assignee)

Updated

3 years ago
Attachment #8729959 - Attachment description: MozReview Request: Bug 1233275 - Copy environment for IPC using NSPR. → MozReview Request: Bug 1233275 - Copy environment for IPC using NSPR. r?jld r?glandium
Attachment #8729959 - Flags: review?(mh+mozilla)
Attachment #8729959 - Flags: review?(jld)
(Assignee)

Comment 3

3 years ago
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/
(Assignee)

Comment 4

3 years ago
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'
(Assignee)

Comment 5

3 years ago
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/
Attachment #8729959 - Attachment description: MozReview Request: Bug 1233275 - Copy environment for IPC using NSPR. r?jld r?glandium → MozReview Request: Bug 1233275 - Copy environment for IPC using NSPR. r?jld
Attachment #8729959 - Flags: review?(mh+mozilla)
Comment on attachment 8729959 [details]
MozReview Request: Bug 1233275 - Copy environment for IPC using NSPR. r?jld

https://reviewboard.mozilla.org/r/39621/#review39011
Attachment #8729959 - Flags: review?(jld) → review+
(Assignee)

Updated

3 years ago
Keywords: checkin-needed
Whiteboard: [npotb]
(Assignee)

Updated

3 years ago
Blocks: 1259852

Comment 8

3 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/84f912382f78
Status: NEW → RESOLVED
Last Resolved: 3 years ago
status-firefox48: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla48
(Assignee)

Comment 9

2 years ago
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
Attachment #8729959 - Flags: approval-mozilla-esr45?
Assignee: nobody → jbeich
status-firefox46: affected → wontfix
status-firefox-esr45: --- → affected
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
Attachment #8729959 - Flags: approval-mozilla-esr45? → approval-mozilla-esr45-
(Assignee)

Updated

2 years ago
See Also: → bug 1321317
You need to log in before you can comment on or make changes to this bug.