942407 - Breakpad signal handler is incorrectly detecting signals sent with kill()

Status: RESOLVED FIXED

(Toolkit :: Crash Reporting, defect)

Description

[Jed Davis [:jld] ⟨⏰|UTC-7⟩ ⟦he/him⟧]

This:

  if (info->si_pid) {
    // This signal was triggered by somebody sending us the signal with kill().

is incorrect; the si_pid field is valid only when the signal was actually sent by a process (either with a syscall like kill() or, for SIGCHLD, by exiting).  In particular, on Linux it overlaps with the si_addr field, so an actual crash will typically have nonzero si_pid.

What I think we actually want here is (info->si_code == SI_USER || info->si_code == SI_QUEUE).  (No, I don't know why anyone would sigqueue() a crash signal, but it seems to be valid.)  The difference isn't obvious normally, but if (for example) tgkill is forbidden by a sandbox that raises SIGSYS instead and causes a confusing recursive crash dump, then that's bad.

And this should go upstream — if nothing else, it can also be relevant if the next signal handler is expecting the original crash's siginfo.

Comment 1

[Jed Davis [:jld] ⟨⏰|UTC-7⟩ ⟦he/him⟧]

Attachment: bug942407-breakpad-usersig-hg0.diff

It's a little more complicated than that — Linux also has SI_TKILL, and could potentially add other si_code values of interest in the future.  Fortunately, it's also simpler than that: positive values are reserved for kernel-generated signals, so fake crashes will always be <= 0.

I've tested this on B2G with both real and fake segfaults, and it appears to do the right thing.

I notice that upstream has a special case for SIGABRT, which is documented as being for a Linux feature (or maybe only in the ChromeOS fork?) that sends it in response to a SysRq key, and with si_pid = 0; but it's not mentioned what the si_code value is for that, and I couldn't easily find the responsible kernel source by inspection.

Comment 2

[(not currently active) Ted Mielczarek]

Comment on attachment 8339130 [details] [diff] [review]
bug942407-breakpad-usersig-hg0.diff

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

Okay, after reading man pages and headers, I understand this. You're right that upstream has some special handling, I'm pretty sure that's a ChromeOS-specific thing, so we'll need to preserve that.

This will need to land upstream, can you generate a patch against the Breakpad SVN source and upload it to the Rietveld instance at breakpad.appspot.com? You can land this on mozilla-central if you stick the patch file here:
http://mxr.mozilla.org/mozilla-central/source/toolkit/crashreporter/breakpad-patches/

Comment 3

[Jed Davis [:jld] ⟨⏰|UTC-7⟩ ⟦he/him⟧]

(In reply to Ted Mielczarek [:ted.mielczarek] from comment #2)
> This will need to land upstream, can you generate a patch against the
> Breakpad SVN source and upload it to the Rietveld instance at
> breakpad.appspot.com? You can land this on mozilla-central if you stick the
> patch file here:
> http://mxr.mozilla.org/mozilla-central/source/toolkit/crashreporter/breakpad-
> patches/

The patch against our current version, or the patch against upstream?

Comment 4

[(not currently active) Ted Mielczarek]

I guess the patch against our current version. Sucks that it's already bitrotted. :-(

Comment 5

[Jed Davis [:jld] ⟨⏰|UTC-7⟩ ⟦he/him⟧]

Attachment: bug942407-breakpad-usersig-hg1.diff

Carrying over r+(ted).

Comment 6

[Jed Davis [:jld] ⟨⏰|UTC-7⟩ ⟦he/him⟧]

The upstream issue: https://breakpad.appspot.com/984002/

Comment 7

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/b196fffee844

Comment 8

[Ryan VanderMeulen [:RyanVM]]

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

Comment 9

[(not currently active) Ted Mielczarek]

(In reply to Jed Davis [:jld] from comment #6)
> The upstream issue: https://breakpad.appspot.com/984002/

This got fixed in a different patch upstream:
https://chromium.googlesource.com/breakpad/breakpad/+/5f0fc7acef3e80a292074e80cb373af9ceb1afb3

At least it got fixed, I guess?

Comment 10

[Jed Davis [:jld] ⟨⏰|UTC-7⟩ ⟦he/him⟧]

(In reply to Ted Mielczarek [:ted.mielczarek] from comment #9)
> At least it got fixed, I guess?

Yeah.  The only difference between the patches is in comments.