Strip heap dumps that Android wrote, arrays too big for an Int, and gzipped ones - #2959
Merged
Conversation
Stripping replaces primitive arrays and wrapped primitives, and copies everything else over unchanged. That includes the string records holding the class, field and method names the rest of the heap dump refers to, which is easy to read as "no strings survive stripping" — a heap dump from Android has nothing but names in those records, but HotSpot writes its whole symbol table out, so every string constant of every loaded class is in there too. Someone stripping a JVM heap dump before sharing it should know that the constants written in the code go with it. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The stripper learned where a primitive wrapper keeps its value from the CLASS_DUMP record of the wrapper class, and could therefore only zero the instances that came after it. HotSpot writes every class dump ahead of any instance, so that held for heap dumps from a JVM — which is all the test covering this used — but ART walks the heap in memory order, and that puts the class dump of java.lang.Integer after most of the Integer instances. 91% of the wrapper instances in leak_asynctask_o.hprof and 98% of those in leak_asynctask_m.hprof kept their value. Nothing about a wrapper needs learning from the class dump: it declares exactly one instance field, and a class declares its own fields ahead of the ones it inherits, so the value is at the start of the instance field values. The class id now comes from LOAD_CLASS, which both writers put before the heap dump, and the class dump is read to check that layout instead of to derive it — writing a heap dump that looks stripped and isn't is the failure worth being loud about. That also drops the search for the "value" field name, which was the one place stripping depended on a name rather than on the format, and a latent bug with it: the running field offset was assigned rather than accumulated, so it was only ever right for the first two fields. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The bytes written over the content of a primitive array were built as a byte array of that content's size, one per array. That is an allocation proportional to the largest array in the heap dump — 374 MB of it to strip a 294 MB heap dump pulled from a real app, and a 216 MB heap to strip a 110 MB one holding a byte[104857600] — and it was sized in Int arithmetic, so a long[268435456] asked for an array of -2147483648 bytes and stripping died with a NegativeArraySizeException. That is the half of #2777 that the parser fix left behind. Three patterns are now built per heap dump and repeated over the content of each array instead, which is a constant 24 KB whatever the heap dump holds: 34 MB allocated for that 294 MB heap dump, an 8 MB heap for the one with the 100 MB array, and a 2.15 GB heap dump holding a long[268435456] strips in 3 seconds where it used to crash. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The heap id in a heap dump info record is an Int, which is how HprofWriter writes it and how readHeapDumpInfoRecord reads it, but the skip in HprofRecordReader and the copy in HprofPrimitiveArrayStripper both counted it as an object id. With 4 byte identifiers those are the same size, and Android — the only heap dumper that emits these records — always uses 4 bytes, so nothing ever noticed. With 8 byte identifiers both moved 4 bytes too far and read the middle of the next sub-record as a tag: "Unknown tag 0x00". Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
A heap dump that's been shared has almost always been gzipped on the way — the Android heap dumps in our test resources compress 3.9x to 4.4x, and 5.6x to 5.8x once stripped — so stripping one meant gunzipping it, stripping it, and gzipping the result again by hand. The file overload now reads gzipped content gzipped, deciding from the first two bytes rather than from the name, and writes gzipped output when the output name ends with ".gz", which is what the default output name of a ".hprof.gz" input already produced. The two halves are separate extensions on the source and sink providers, so the streaming overload can take either one on its own. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Five things found while comparing
HprofPrimitiveArrayStripperagainsthprof-redact, one commit each.
Wrapped primitives were left alone on Android
Stripping is meant to zero the value every primitive wrapper wraps, on top of zeroing primitive
arrays. It learned where that value sits from the
CLASS_DUMPrecord of the wrapper class, so itcould only zero the instances dumped after it. HotSpot writes every class dump ahead of any instance,
which is why the test covering this — built from a JVM heap dump — passed. ART walks the heap in
memory order, so most
Integerinstances come before the class dump ofjava.lang.Integer:leak_asynctask_o.hprofleak_asynctask_m.hprofNothing about a wrapper needs learning from the class dump: it declares exactly one instance field,
and a class declares its own fields ahead of the ones it inherits, so the value is at the start of the
instance field values. The class id now comes from
LOAD_CLASS, which both ART and HotSpot writebefore the heap dump starts, and the class dump is read to check that layout instead of to derive
it. Two new tests: one against
leak_asynctask_o.hprofasserting every wrapper wraps zero, onewriting the wrapper's class dump after its instances, both verified to fail without the fix.
Writing over an array allocated a copy of it
The replacement bytes were built as a byte array the size of the array's content, one per array.
That's the remaining half of #2777: sized in
Intarithmetic, so along[268435456]asked for anarray of -2147483648 bytes and stripping died with a
NegativeArraySizeException. Three patterns arenow built per heap dump and repeated over each array's content instead — a constant 24 KB, whatever
the heap dump holds.
long[268435456]NegativeArraySizeExceptionbyte[104857600]Output is byte for byte identical on the heap dumps in our test resources and on that 294 MB one.
CopyingSource.overwriteRepeatingis the new public method;overwriteis untouched.Heap dump info records were skipped by the wrong size
HEAP_DUMP_INFOholds anIntheap id and then a string id.HprofRecordReader.readHeapDumpInfoRecordreads it that way and
HprofWriterwrites it that way, but the skip inHprofRecordReaderand thecopy in the stripper both counted the heap id as an object id. With the 4 byte identifiers Android
always uses those are the same size, which is why nothing noticed; with 8 byte identifiers both move
4 bytes too far and read the middle of the next sub-record as a tag, failing with "Unknown tag
0x00".
Transparent gzip
A heap dump that's been shared has almost always been gzipped on the way, so stripping one meant
gunzipping it and gzipping the result by hand.
strip-hprofand the file overload now read gzippedcontent gzipped — decided from the first two bytes, not from the name — and write gzipped output when
the output name ends with
.gz, which is what the default output name of a.hprof.gzinput alreadygave you.
StreamingSourceProvider.gunzipIfGzipped()andStreamingSinkProvider.gzip()are what doit and compose onto anything.
The Android heap dumps in our test resources compress 3.9x to 4.4x, and 5.6x to 5.8x once stripped.
Saying what stripping doesn't cover
Stripping replaces primitive arrays and wrapped primitives and copies everything else over unchanged,
which includes the string records holding class, field and method names. An Android heap dump has
nothing but names in there. HotSpot writes its whole symbol table out, so a JVM heap dump also carries
every string constant of every loaded class — no user data, but the constants written in the code. The
KDoc and the CLI help now say so.
Left for later
A primitive array of more than 2^31 elements still can't be stripped: the length is read as a signed
Int, in the stripper and inHprofRecordReaderalike, so it wraps negative. Reaching that needs a16 GB
longarray, and fixing it is a change to how both read the length, not to how they size theskip.
ABI
shark-hprofgainsCopyingSource.overwriteRepeating,StreamingSourceProvider.gunzipIfGzipped()and
StreamingSinkProvider.gzip(). Nothing removed, nothing changed.🤖 Generated with Claude Code