1528304 - -GS buffer overflow in arm64 breakpad

Status: RESOLVED FIXED

(Toolkit :: Crash Reporting, defect)

Description

[(Away)]

Bug 1518947 made it so that we collect arm64 registers for exception memory regions: https://searchfox.org/mozilla-central/rev/8e8ccec700159dc4efe061cfec2af10b21a8e62a/toolkit/crashreporter/breakpad-client/windows/common/minidump_callback.cc#215-222

However there are 32 registers on arm64 and the buffer is still sized at 16.

If Firefox crashes and there is no JIT on the stack, the -GS failure kills the process with a fastfail and no crash report is generated. (If there is JIT on the stack, other issues stand in the way. More in bug 1526276)

kExceptionAppMemoryRegions needs to be updated to at least 32: https://searchfox.org/mozilla-central/rev/8e8ccec700159dc4efe061cfec2af10b21a8e62a/toolkit/crashreporter/breakpad-client/windows/common/minidump_callback.h#48

Comment 1

[(Away)]

Attachment: Update max memory recording regions.

Comment 2

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/autoland/rev/a1dd47617412

Comment 3

[Julien Cristau [:jcristau]]

https://hg.mozilla.org/mozilla-central/rev/a1dd47617412

Comment 4

[Liz Henry (:lizzard) (relman/hg->git project)]

David, want to request uplift to beta?

Comment 5

[(Away)]

Comment on attachment 9044228 [details]
Update max memory recording regions.

Beta/Release Uplift Approval Request

Feature/Bug causing the regression

Bug 1518947

User impact if declined

Buffer overflow + missing crash reports

Is this code covered by automated tests?

Unknown

Has the fix been verified in Nightly?

No

Needs manual test from QE?

No

If yes, steps to reproduce

List of other uplifts needed

None

Risk to taking this patch

Low

Why is the change risky/not risky? (and alternatives if risky)

Buffer size fix

(Also, since I can't write a free-form answer to the automated tests question above, I'll write it here: this code would be covered by tests except that arm64 isn't running in CI yet.)

String changes made/needed

No

Comment 6

[(Away)]

I checked crash-stats and we still haven't seen any crash reports after this landing, most likely because the other half of the puzzle (bug 1526276 comment 17) still stands in the way.

Comment 7

[Liz Henry (:lizzard) (relman/hg->git project)]

Comment on attachment 9044228 [details]
Update max memory recording regions.

First part of a potential fix for missing crash reports.
OK for uplift for beta 10.

Comment 8

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/1deedd79bc67

Comment 9

[Al Billings [:abillings - ex-MoCo]]

Minusing for advisories since it is ARM64 only, which we aren't shipping yet.