1455662 - mprotect() failure in linker causing crashes on S8

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[James Willcox (:snorp) ([e-mail]) (he/him)]

+++ This bug was initially created as a clone of Bug #1450793 +++

This crash is showing up in high volume on the Play Store crash records but not on crash-stats, for Android 8.0 and up, on several different Galaxy S8 devices:


signal 11 (SIGSEGV), code 2 (SEGV_ACCERR)
in libmozglue.so

*** *** *** *** *** *** *** *** *** *** *** *** *** *** *** *** pid: 0, tid: 0 >>> org.mozilla.firefox <<< backtrace:

#00 pc 0000000000043d66 /data/app/org.mozilla.firefox-cEa3LWSWhqBfUNxUk_l7Jw==/lib/arm/libmozglue.so

These are the affected devices, 

dreamqlteue
dream2qlteue
SCV36
dreamqltecan
dream2qltecan
And the Note8 (greatqlte) as well.

Comment 1

[James Willcox (:snorp) ([e-mail]) (he/him)]

Attachment: Bug 1455662 - Guard against mprotect() failure when manipulating link map

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

Comment 2

[James Willcox (:snorp) ([e-mail]) (he/him)]

The patch from 1450793 has apparently not fixed things, so we need some other mitigation here. This patch should avoid the crash and will print the cause of the error. I think crash dumps may not be very useful from phones affected by this, but hopefully we'll get the logcat with the mprotect() error. This may allow us to figure out why it's failing.

Comment 3

[Liz Henry (:lizzard Please n-i to RyanVM, jcristau, or pascal)]

Tracking since this is a potential mitigation for the spike in Fennec crashes for the 8.0 update to popular Samsung devices.

Comment 4

[(inactive) Jim Chen [:jchen] [:darchons]]

Comment on attachment 8969694 [details]
Bug 1455662 - Guard against mprotect() failure when manipulating link map

https://reviewboard.mozilla.org/r/238492/#review244706

::: mozglue/linker/ElfLoader.cpp:907
(Diff revision 1)
>  
>      prot = getProt(start, &end);
>      if (prot == -1 || (start + length) > end)
>        MOZ_CRASH();
>  
>      if (prot & PROT_WRITE)

`success` should be true in this case

::: mozglue/linker/ElfLoader.cpp:908
(Diff revision 1)
>      prot = getProt(start, &end);
>      if (prot == -1 || (start + length) > end)
>        MOZ_CRASH();
>  
>      if (prot & PROT_WRITE)
>        return;

`success` will be uninitialized here

::: mozglue/linker/ElfLoader.cpp:920
(Diff revision 1)
> +            page, length, prot | PROT_WRITE, ret,
> +            errno, strerror(errno));
> +    }
> +  }
> +
> +  bool IsWritable() {

const

::: mozglue/linker/ElfLoader.cpp:920
(Diff revision 1)
> +            page, length, prot | PROT_WRITE, ret,
> +            errno, strerror(errno));
> +    }
> +  }
> +
> +  bool IsWritable() {

const

::: mozglue/linker/ElfLoader.cpp:1000
(Diff revision 1)
>      firstAdded = map;
>      /* When adding a library for the first time, r_map points to data
>       * handled by the system linker, and that data may be read-only */
>      EnsureWritable w(&dbg->r_map->l_prev);
> +    if (!w.IsWritable()) {
> +      return;

We will have corrupted state here if we return early.

::: mozglue/linker/ElfLoader.cpp:1029
(Diff revision 1)
>      firstAdded = map->l_prev;
>      /* When removing the first added library, its l_next is going to be
>       * data handled by the system linker, and that data may be read-only */
>      EnsureWritable w(&map->l_next->l_prev);
> +    if (!w.IsWritable()) {
> +      return;

Same here

Comment 5

[James Willcox (:snorp) ([e-mail]) (he/him)]

Comment on attachment 8969694 [details]
Bug 1455662 - Guard against mprotect() failure when manipulating link map

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/238492/diff/1-2/

Comment 6

[Mike Hommey [:glandium]]

Comment on attachment 8969694 [details]
Bug 1455662 - Guard against mprotect() failure when manipulating link map

https://reviewboard.mozilla.org/r/238492/#review245178

::: mozglue/linker/ElfLoader.cpp:916
(Diff revision 2)
>  
>      page = firstPage;
> -    mprotect(page, length, prot | PROT_WRITE);
> +    int ret = mprotect(page, length, prot | PROT_WRITE);
> +    success = ret == 0;
> +    if (!success) {
> +      ERROR("mprotect(%p, %zu, %d) = %d %d %s",

nit: "mprotect(%p, %zu, %o) = %d (errno=%d; %s)"

::: mozglue/linker/ElfLoader.cpp:1006
(Diff revision 2)
> +  dbg->r_state = r_debug::RT_ADD;
> +  dbg->r_brk();

This actually needs to happen *before* touching dbg->r_map->l_prev.

::: mozglue/linker/ElfLoader.cpp:1036
(Diff revision 2)
> +  dbg->r_state = r_debug::RT_DELETE;
> +  dbg->r_brk();

Likewise, this needs to happen before touching map->l_next->l_prev.

Comment 7

[James Willcox (:snorp) ([e-mail]) (he/him)]

Comment on attachment 8969694 [details]
Bug 1455662 - Guard against mprotect() failure when manipulating link map

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/238492/diff/2-3/

Comment 8

[(inactive) Jim Chen [:jchen] [:darchons]]

Comment on attachment 8969694 [details]
Bug 1455662 - Guard against mprotect() failure when manipulating link map

https://reviewboard.mozilla.org/r/238492/#review245396

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/3c15a3b3c85a
Guard against mprotect() failure when manipulating link map r=jchen

Comment 10

[Andreea Pavel [:apavel]]

https://hg.mozilla.org/mozilla-central/rev/3c15a3b3c85a

Comment 11

[Ryan VanderMeulen [:RyanVM]]

Too late for 59, but we still want this on 60 as soon as we have verification that it helps with the crashes.

Comment 12

[Julien Cristau [:jcristau]]

Looking at the play console, volume of SIGSEGV/SEGV_ACCERR in mozglue on nightly seems to have gone down the last few days, so it looks like this helped.  Please request uplift to m-r so we can get this in for 60.0.

Comment 13

[James Willcox (:snorp) ([e-mail]) (he/him)]

Comment on attachment 8969694 [details]
Bug 1455662 - Guard against mprotect() failure when manipulating link map

Approval Request Comment
[Feature/Bug causing the regression]: Unknown
[User impact if declined]: Startup crashes in some circumstances
[Is this code covered by automated tests?]: Yes
[Has the fix been verified in Nightly?]: No, we have no STR
[Needs manual test from QE? If yes, steps to reproduce]:  No
[List of other uplifts needed for the feature/fix]: None
[Is the change risky?]: No
[Why is the change risky/not risky?]: Only improves error handling
[String changes made/needed]: None

Comment 14

[Julien Cristau [:jcristau]]

Comment on attachment 8969694 [details]
Bug 1455662 - Guard against mprotect() failure when manipulating link map

fennec startup crash fix, approved for 60rc and 60.0b17

Comment 15

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/releases/mozilla-release/rev/dede96ad1e4f
https://hg.mozilla.org/releases/mozilla-beta/rev/b28d01fdafa5e2c0d39dc389574bd7d6e4a0ffac (FIREFOX_60b_RELBRANCH)