Android crash helper aborts with "capacity overflow" while collecting a crashed child's annotations, so later child crashes go unreported
Categories
(Toolkit :: Crash Reporting, defect)
Tracking
()
| Tracking | Status | |
|---|---|---|
| firefox158 | --- | fixed |
People
(Reporter: RyanVM, Assigned: RyanVM)
References
(Blocks 3 open bugs)
Details
Attachments
(1 file)
On Android, the crash helper sometimes aborts with capacity overflow right after writing a child process's minidump. Because the crash helper is not restarted on Android (bug 2039654), every later child crash in that session goes unreported. In geckoview-junit this shows up as ContentCrashTest#crashContentJava and the crash tests after it timing out, which is most of what gets classified under bug 1993445 on opt builds since July.
https://treeherder.mozilla.org/logviewer?job_id=593278291&repo=autoland
finalize_breakpad_minidump calls mozannotation_server::retrieve_annotations, which reads the child's annotation table out of its memory. An entry can be garbage when it is read (the child's other threads may still be updating the table), and process_reader's copy_array sizes its allocation straight from the length in that entry with Vec::with_capacity, which panics once the length exceeds isize::MAX. Before bug 2050534 the same bad entries made retrieve_annotations segfault instead.
The proposed patch makes copy_array check the size and reserve fallibly on all platforms, returning an error instead of panicking, and has mozannotation_server reject annotation lengths above 1 MiB and stop after max_annotations entries, so a bad entry is dropped and the minidump is still delivered. Why the entries are inconsistent in the first place (the table is read after the dump while the child may be running, without taking its mutex) is left for a follow-up.
|
||
Comment 1•5 days ago
|
||
The racy reads may be what is causing the garbage entries in bug 2074575.
|
Assignee | |
Comment 2•5 days ago
|
||
After writing a child's minidump, the crash helper reads the child's annotation table out of its memory. The
child may have kept running other threads while it waited, so an entry can be read mid-update or from a table
that was just reallocated, and its length is then garbage. copy_array sized its Vec directly from that length
with Vec::with_capacity(), so any length above isize::MAX panicked with "capacity overflow" and took the crash
helper down. Android's crash helper is not restarted after it dies, so every later child crash in that session
went unreported. This shows up in geckoview-junit as ContentCrashTest#crashContentJava and the crash tests
after it timing out once the helper has died. Before bug 2050534 the same bad entries crashed the helper with
a segfault inside retrieve_annotations instead.
copy_array now checks the byte count for overflow and reserves fallibly on every platform, returning
ReadError::TooLarge instead of panicking. mozannotation_server also rejects byte-buffer and nsCString lengths
above 1 MiB, far above any real annotation, so a garbage length that happens to be allocatable doesn't turn
into a multi-gigabyte ptrace read, and it stops walking the table after max_annotations entries. A bad entry
is now dropped and the rest of the annotations and the minidump are still delivered.
|
||
Updated•5 days ago
|
|
|
Assignee | |
Updated•5 days ago
|
|
||
Comment 4•4 days ago
|
||
| bugherder | ||
|
||
Comment 5•4 days ago
|
||
Capping the size of the annotations we capture is something I wanted to do for a while but this was a little too fast because there's a few things that do not track here: the first one being that the child threads' are suspended when taking a minidump and when reading the annotations, save for a very brief moment between the two which we intend to make it go away. It's very unlikely that we'd always corrupt annotations right at that time, what is more likely is that the annotations are corrupt to begin with and I'd like to know why. I would have preferred a different approach where we still make reads non-fallible, but with a shorter cap and two additional tweaks:
- We truncate annotations instead of dropping them entirely, so that we get an idea of what's going on. This could be done intelligently, e.g. if we're reading strings we stop at the first null byte instead of reading the entire length if it's longer.
- We add information about the annotation that overflowed to the soft failure stream of the minidump writer. This would require more work to integrate it but it's important, otherwise this can turn into a silent error and add to the things we don't know.
In general before quieting an error I'd like to see a proper analysis of what caused the error first, and then we act on it.
Description
•