821192 - nsUpdateDriver needs to waitpid() all subprocesses before applying update

Status: RESOLVED FIXED

(Toolkit :: Startup and Profile System, defect, P1)

Description

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Right now we simply exec() the updater binary over the b2g process to finish applying the background update.  This is subject to the following race-condition-y problem

 1. b2g process forks new subprocess
 2. Immediately afterwards, b2g process exec("updater")s

At this point, CLOEXEC will take out the b2g process's socketpair with the new subprocess.  However, the following two steps can race

 3a. updater moves old /system/b2g directory out of the way
 3b. updater moves new /system/b2g directory into place

 4a. plugin-container exec()s
 4b. plugin-container finishes loading dynamic libraries

If this happens, extremely scary things can happen in the plugin-container.  Most of those things should be harmless crashes, but since Camera and Movie apps will have high privileges we need to defend against that.

The solution is to enumerate the live subprocesses just before we exec() in nsUpdateProcessor and join with all of them.

bb? to triage assignee.

Comment 1

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

We have a helper to enumerate child processes here

http://mxr.mozilla.org/mozilla-central/source/dom/ipc/ContentParent.h#82

I think we'll need a new interface ContentParent::KillAndWait() that uses the existing process_util.h helper.  We also need to double check that that helper is bulletproof.

Comment 2

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Stealing.

Comment 3

[Dave Hylands [:dhylands]]

NP. I hadn't started any coding, just looking at where it would be coded. I'm happy to review...

Comment 4

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Attachment: Patch

Includes fixes for a couple of other unrelated bugs.  Need to split.

Comment 5

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

(In reply to Dave Hylands [:dhylands] from comment #3)
> NP. I hadn't started any coding, just looking at where it would be coded.
> I'm happy to review...

No worries.  I was afraid this one would get messy, and it did :/.

Comment 6

[Dave Hylands [:dhylands]]

Comment on attachment 695587 [details] [diff] [review]
Patch

Review of attachment 695587 [details] [diff] [review]:
-----------------------------------------------------------------

At a conceptual level, this is more or less what was thinking would need to happen.

::: dom/ipc/ContentParent.cpp
@@ +315,5 @@
> +    {
> +        MonitorAutoLock lock(monitor);
> +        while (!done) {
> +            lock.Wait();
> +        }

The only issue I can think of is that this is going to block the MainThread. If the IOThread happened to do something similar (Post a runnable to the main thread) at around the same time, then we could deadlock.

Comment 7

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

The IO thread isn't allowed to block on the main thread, because the main thread needs to block on the IO thread when sending sync IPC.

Comment 8

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Attachment: part 1: Fix the watchdog timeout code

When we're (asynchronously) shutting content processes, their exits result in SIGCHLD being delivered to the parent, which interrupts the sleep.  So we have so far basically always been committing SIGKILL seppuku right after attempting restart.  That's very bad because it means our profile sync may not be completing, data loss issue.

Comment 9

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Attachment: part 2: Add an interface to join all live content processes

Comment 10

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Attachment: part 3: Join all subprocesses before restarting the main process, when we're e.g. about to apply an update

Fixes the "real bug" here.  We only need to do this when restarting, because reboots and poweroffs will use the hardware itself to kill any running processes.

Comment 11

[Dave Hylands [:dhylands]]

Comment on attachment 695841 [details] [diff] [review]
part 1: Fix the watchdog timeout code

Review of attachment 695841 [details] [diff] [review]:
-----------------------------------------------------------------

::: hal/linux/LinuxPower.cpp
@@ +79,5 @@
> +      if (sleepSeconds <= 0) {
> +        break;
> +      }
> +      sleep(sleepSeconds);
> +    }

This should use TimeTicks::Now which is a monotonically increasing time, whereas TimeStamp::Now is wall time, which could change (due to timezone changes or the user setting the time).

Comment 12

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

(In reply to Dave Hylands [:dhylands] from comment #11)
> Comment on attachment 695841 [details] [diff] [review]
> part 1: Fix the watchdog timeout code
> 
> Review of attachment 695841 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: hal/linux/LinuxPower.cpp
> @@ +79,5 @@
> > +      if (sleepSeconds <= 0) {
> > +        break;
> > +      }
> > +      sleep(sleepSeconds);
> > +    }
> 
> This should use TimeTicks::Now which is a monotonically increasing time,
> whereas TimeStamp::Now is wall time, which could change (due to timezone
> changes or the user setting the time).

TimeStamp is monotonic [1], rather painfully so on some platforms :/.  (I wrote the POSIX clock_gettime(CLOCK_MONOTONIC) implementation, which was delightfully non-painful :).)

[1] http://mxr.mozilla.org/mozilla-central/source/xpcom/ds/TimeStamp.h#158

Comment 13

[Dave Hylands [:dhylands]]

(In reply to Chris Jones [:cjones] [:warhammer] from comment #12)
> > This should use TimeTicks::Now which is a monotonically increasing time,
> > whereas TimeStamp::Now is wall time, which could change (due to timezone
> > changes or the user setting the time).
> 
> TimeStamp is monotonic [1], rather painfully so on some platforms :/.  (I
> wrote the POSIX clock_gettime(CLOCK_MONOTONIC) implementation, which was
> delightfully non-painful :).)
> 
> [1] http://mxr.mozilla.org/mozilla-central/source/xpcom/ds/TimeStamp.h#158

Right you are - I was confused and was looking at Time::Now and not TimeStamp::Now.

Comment 14

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Comment on attachment 695842 [details] [diff] [review]
part 2: Add an interface to join all live content processes

Review of attachment 695842 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/ipc/ContentParent.cpp
@@ +290,5 @@
> +    // Don't touch any arguments to this function from now on.
> +}
> +
> +/*static*/ void
> +ContentParent::JoinAllSubprocesses()

Nit: I'd assert main thread here.

@@ +299,5 @@
> +        printf_stderr("There are no live subprocesses.");
> +        return;
> +    }
> +
> +    printf_stderr("Subprocesses are still alive.  Doing emergency join.");

Nit: Need a newline.

Comment 15

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Done and done.

https://tbpl.mozilla.org/?tree=Try&rev=40abbd7ff692

Comment 16

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

https://hg.mozilla.org/integration/mozilla-inbound/rev/661960a9cf89

Comment 17

[Graeme McCutcheon [:graememcc]]

https://hg.mozilla.org/mozilla-central/rev/661960a9cf89

Comment 18

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/0a687d4da87a
https://hg.mozilla.org/releases/mozilla-b2g18/rev/d15afe54a3de