Closed Bug 1455662 Opened 6 years ago Closed 6 years ago

mprotect() failure in linker causing crashes on S8

Categories

(Firefox for Android Graveyard :: General, defect)

Firefox 59
defect
Not set
normal

Tracking

(firefox59+ wontfix, firefox60+ fixed, firefox61+ fixed)

RESOLVED FIXED
Firefox 61
Tracking Status
firefox59 + wontfix
firefox60 + fixed
firefox61 + fixed

People

(Reporter: snorp, Assigned: snorp)

References

Details

Attachments

(1 file)

+++ 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.
Assignee: nobody → snorp
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.
Tracking since this is a potential mitigation for the spike in Fennec crashes for the 8.0 update to popular Samsung devices.
Attachment #8969694 - Flags: review?(mh+mozilla) → review?(nchen)
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
Attachment #8969694 - Flags: review?(nchen) → review-
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 on attachment 8969694 [details]
Bug 1455662 - Guard against mprotect() failure when manipulating link map

https://reviewboard.mozilla.org/r/238492/#review245396
Attachment #8969694 - Flags: review?(nchen) → review+
Pushed by jwillcox@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/3c15a3b3c85a
Guard against mprotect() failure when manipulating link map r=jchen
https://hg.mozilla.org/mozilla-central/rev/3c15a3b3c85a
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 61
Too late for 59, but we still want this on 60 as soon as we have verification that it helps with the crashes.
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.
Flags: needinfo?(snorp)
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
Flags: needinfo?(snorp)
Attachment #8969694 - Flags: approval-mozilla-beta?
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
Attachment #8969694 - Flags: approval-mozilla-release+
Attachment #8969694 - Flags: approval-mozilla-beta?
Attachment #8969694 - Flags: approval-mozilla-beta+
Product: Firefox for Android → Firefox for Android Graveyard
You need to log in before you can comment on or make changes to this bug.