1823692 - Storing the memory around the instruction IP is broken in 32-bit Linux builds

Status: RESOLVED FIXED

(Toolkit :: Crash Reporting, defect)

Description

[Gabriele Svelto [:gsvelto]]

As per title, toolkit/crashreporter/test/unit/test_crashreporter_crash.js fails on Linux 32-bit builds on this check. Disabling the oxidized writer and re-enabling the Breakpad writer solves the problem so chances are that the regressing bug was 1620993 but I need to test it.

Comment 1

[Gabriele Svelto [:gsvelto]]

Turns out this used to work, it broke with bug 1793784.

Comment 2

[BugBot [:suhaib / :marco/ :calixte]]

Set release status flags based on info from the regressing bug 1793784

Comment 3

[Dianna Smith [:diannaS]]

:gsvelto can you set the severity on this ticket? even though it was regressed in 110, trying to assess tracking for 112.

Comment 4

[Gabriele Svelto [:gsvelto]]

I'm giving it S3 because it affects a subset of our crash reports and even for those affected we don't rely on this information just yet (but we want to).

Comment 5

[Alex Franchuk [:afranchuk]]

This is due to the process context information having the incorrect type (i64) for general registers. As a result, all registers and the signal mask in 32bit crash reports have been incorrect.

I've submitted a PR to fix the issue here.

Comment 6

[Alex Franchuk [:afranchuk]]

Attachment: Bug 1823692 - Storing the memory around the instruction IP is broken in 32-bit Linux builds r=gsvelto,#firefox-build-system-reviewers

Using the newer version of the crash-context crate corrects the 32-bit Linux context structure,
which was the cause of the IP memory not being stored (among possibly other 32-bit Linux issues).

Comment 7

[Gabriele Svelto [:gsvelto]]

Jake Shadle just cut a new release of minidump-writer. Let's pull it, audit it and this bug will be fixed as it'll pull in the new version of crash-context with your fix. Additionally we'll get a couple more small fixes on Linux which doesn't hurt.

Comment 8

[Alex Franchuk [:afranchuk]]

Sounds great, will do so now.

Comment 9

[Alex Franchuk [:afranchuk]]

This is slightly complicated by bitflags 2 being newly used in minidump-writer, where bitflags 1 is used in the rest of m-c. I doubt minidump-writer actually needs version 2, but that's what's in the latest release. And version 1 is used by a lot of crates in the graph.

Comment 10

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/c4f82df5d5ac
Storing the memory around the instruction IP is broken in 32-bit Linux builds r=gsvelto,glandium,supply-chain-reviewers

Comment 11

[Sandor Molnar[:smolnar]]

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

Comment 12

[BugBot [:suhaib / :marco/ :calixte]]

The patch landed in nightly and beta is affected.
:afranchuk, is this bug important enough to require an uplift?

• If yes, please nominate the patch for beta approval.
• If no, please set status-firefox112 to wontfix.

For more information, please visit auto_nag documentation.

Comment 13

[Alex Franchuk [:afranchuk]]

Attachment: Bug 1823692 - Storing the memory around the instruction IP is broken in 32-bit Linux builds r=gsvelto,#firefox-build-system-reviewers

Using the newer version of the crash-context crate corrects the 32-bit Linux context structure,
which was the cause of the IP memory not being stored (among possibly other 32-bit Linux issues).

Original Revision: https://phabricator.services.mozilla.com/D174314

Comment 14

[Phabricator Automation]

Uplift Approval Request

• Fix verified in Nightly: yes
• Explanation of risk level: The change fixes a bug on x86 Linux and does not affect other platforms.
• Steps to reproduce for manual QE testing: None
• Code covered by automated testing: yes
• String changes made/needed: No
• Needs manual QE test: no
• User impact if declined: Minimal. Users on x86 Linux may, however unlikely, encounter a crash in the minidump writer due to invalid memory access.
• Risk associated with taking this patch: Low
• Is Android affected?: no

Comment 15

[Dianna Smith [:diannaS]]

Comment on attachment 9327520 [details]
Bug 1823692 - Storing the memory around the instruction IP is broken in 32-bit Linux builds r=gsvelto,#firefox-build-system-reviewers

Changing nomination to release

Comment 16

[Dianna Smith [:diannaS]]

Comment on attachment 9327520 [details]
Bug 1823692 - Storing the memory around the instruction IP is broken in 32-bit Linux builds r=gsvelto,#firefox-build-system-reviewers

This is an S3 110 regression and doesn't graft cleanly to release. I think it might be best for this to ride the 113 trains and go out on May 9th. If you feel strongly otherwise, please feel free to NI me.