8337517: Redacted Heap Dumps #20409

Henry-Lin-A · 2024-07-31T19:10:41Z

Adds a command line option -redact to jcmd and -XX:+HeapDumpRedacted enabling redacted heap dumps. When enabled, the output binary heap dump has zeroes written out in place of the original primitive values in the object fields. There is a new jtreg test heapDumpRedactedTest.java that tests that the fields are properly redacted.

Progress

Change must be properly reviewed (1 review required, with at least 1 Reviewer)
Change must not contain extraneous whitespace
Commit message must refer to an issue
Change requires CSR request JDK-8337518 to be approved

Issues

JDK-8337517: Redacted Heap Dumps (Enhancement - P4)
JDK-8337518: Introduce options to jcmd and a JVM runtime flag to generate redacted heap dumps. (CSR)

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.org/jdk.git pull/20409/head:pull/20409
$ git checkout pull/20409

Update a local copy of the PR:
$ git checkout pull/20409
$ git pull https://git.openjdk.org/jdk.git pull/20409/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 20409

View PR using the GUI difftool:
$ git pr show -t 20409

Using diff file

Download this PR as a diff file:
https://git.openjdk.org/jdk/pull/20409.diff

Webrev

Link to Webrev Comment

bridgekeeper · 2024-07-31T19:11:29Z

👋 Welcome back Henry-Lin-A! A progress list of the required criteria for merging this PR into master will be added to the body of your pull request. There are additional pull request commands available for use with this pull request.

openjdk · 2024-07-31T19:11:41Z

❗ This change is not yet ready to be integrated.
See the Progress checklist in the description for automated requirements.

openjdk · 2024-07-31T19:12:36Z

@Henry-Lin-A The following labels will be automatically applied to this pull request:

hotspot-runtime
serviceability

When this pull request is ready to be reviewed, an "RFR" email will be sent to the corresponding mailing lists. If you would like to change these labels, use the /label pull request command.

mlbridge · 2024-07-31T21:38:45Z

Webrevs

01: Full - Incremental (a06f1ff4)
00: Full (e7383044)

plummercj · 2024-07-31T23:14:55Z

src/jdk.attach/linux/classes/sun/tools/attach/VirtualMachineImpl.java

@@ -139,7 +139,7 @@ public void detach() throws IOException {
     * Execute the given command in the target VM.
     */
    InputStream execute(String cmd, Object ... args) throws AgentLoadException, IOException {
-        assert args.length <= 3;                // includes null
+        assert args.length <= 4;                // includes null


I'm pretty sure we've been down this path before, and allowing 4 args will result in newer versions of the JVM not working with older versions of the tool. See JDK-8219721

dholmes-ora · 2024-08-01T03:37:26Z

I must be missing something in the approach. The vast majority of confidential data will be in strings yet you focus on primitives that would rarely (if ever for boolean float/double) contain anything that could be recognised as such.

turbanoff · 2024-08-01T08:38:26Z

test/hotspot/jtreg/serviceability/dcmd/gc/HeapDumpRedactedTest.java

+        testPrimitiveArray("boolArray", targetObject, "false");
+
+        //static field
+        JavaThing staticInt =  testClass.getStaticField("staticInt");


Suggested change

JavaThing staticInt = testClass.getStaticField("staticInt");

JavaThing staticInt = testClass.getStaticField("staticInt");

AlanBateman · 2024-08-01T09:19:35Z

The alternative is of course to not burden the VM and heap dumpers with this but instead provide tooling to process a HPROF heap dump to zero the contents of Strings and other fields.

shipilev

Looks interesting, some suggestions:

shipilev · 2024-08-01T14:00:51Z

src/hotspot/share/services/heapDumper.cpp

-      } else {
-        writer->write_raw(array->int_at_addr(0), length_in_bytes);
+  if (redact) {
+    writer->write_raw(nullptr, length_in_bytes); //nullptr to write zeros


Would be easier to just:

if (redact) { writer->write_raw(nullptr, length_in_bytes); //nullptr to write zeros writer->end_sub_record(); return; }

...and leave the rest of the code alone.

shipilev · 2024-08-01T14:02:43Z

src/hotspot/share/services/heapDumper.cpp

@@ -988,7 +996,7 @@ void DumperSupport::dump_double(AbstractDumpWriter* writer, jdouble d) {
 }

 // dumps the raw value of the given field
-void DumperSupport::dump_field_value(AbstractDumpWriter* writer, char type, oop obj, int offset) {
+void DumperSupport::dump_field_value(AbstractDumpWriter* writer, char type, oop obj, int offset, bool redact) {


I think it would be cleaner to specialize this method into two: dump_field_value and dump_redacted_field_value, and dispatch to it from outside. Likely saves branches.

shipilev · 2024-08-01T14:16:35Z

I must be missing something in the approach. The vast majority of confidential data will be in strings yet you focus on primitives that would rarely (if ever for boolean float/double) contain anything that could be recognised as such.

(moved the reply to bug, so that it is not buried in PR comments: https://bugs.openjdk.org/browse/JDK-8337517?focusedId=14695155#comment-14695155)

plummercj · 2024-08-01T15:20:41Z

The alternative is of course to not burden the VM and heap dumpers with this but instead provide tooling to process a HPROF heap dump to zero the contents of Strings and other fields.

That approach also has the added benefit of addressing this issue with heap dumps produced by SA, which is not being covered by this PR.

earthling-amzn · 2024-08-01T17:27:25Z

@plummercj - I'm sorry, can you expand this acronym for me?

heap dumps produced SA

This reverts commit e87b74e.

plummercj · 2024-08-01T18:05:34Z

@plummercj - I'm sorry, can you expand this acronym for me?

heap dumps produced SA

Serviceability Agent. It's a set of tools used to debug jvm and application issues. It can be run against a live process or a core file. It does things like produce stack traces and heap dumps, but also is good at inspecting the JVM and heap state.

https://docs.oracle.com/en/java/javase/17/docs/specs/man/jhsdb.html

See jhsdb jmap -binaryheap.

The SA implementation for heap dumping is mostly in: src/jdk.hotspot.agent/share/classes/sun/jvm/hotspot/utilities/HeapHprofBinWriter.java

plummercj · 2024-08-01T18:12:09Z

heap dumps produced SA

Sorry also about the typo. That should have been produced by SA. I fixed it in the original comment.

kevinjwalls · 2024-08-02T08:28:00Z

Notes on built-in redaction vs. external tool:

As a system owner, I would want both kinds of dump?
The pure dump for myself, and the redacted dump to share with a 3rd party for analysis.

They may come back with information about relationships (a particular HttpRequest object #ID is involved, or observations on threads and locks etc...), and I can confirm in more detail in the original dump with the actual Strings.

That suggests a tool outside the JVM, a post-processor for HPROF dumps that performs redaction.

If I can use jcmd to switch redaction on and off, to get both kinds of dump: do I really want to perform the dump twice (if some people don't like suspending their app once), and getting slightly different (or maybe significantly different) state in the two dumps.

plummercj · 2024-08-02T15:38:16Z

If I can use jcmd to switch redaction on and off, to get both kinds of dump: do I really want to perform the dump twice

Yep, and there are two issues with getting two dumps. The first is that sometimes the dump is triggered automatically, like when out of memory, so there may not even be an option for a 2nd dump. The other issue is that if you request two dumps with jcmd, the two dumps won't be identical.

earthling-amzn · 2024-08-20T20:02:06Z

This review has gone quiet. I sense that there are no major technical objections to the change, but that the consensus prefers to leave this functionality to external tools. If there are no further comments by the end of the week, we will withdraw this pull request.

bridgekeeper · 2024-09-17T21:24:30Z

@Henry-Lin-A This pull request has been inactive for more than 4 weeks and will be automatically closed if another 4 weeks passes without any activity. To avoid this, simply add a new comment to the pull request. Feel free to ask for assistance if you need help with progressing this pull request towards integration!

bridgekeeper · 2024-10-16T01:15:30Z

@Henry-Lin-A This pull request has been inactive for more than 8 weeks and will now be automatically closed. If you would like to continue working on this pull request in the future, feel free to reopen it! This can be done using the /open pull request command.

Henry-Lin-A added 6 commits July 2, 2024 21:29

redacted heap dump and test

96c08c3

formatting

345e9c2

add redact option to jcmd line

44d6e6a

changed heapDumper.cpp dump function to take in redact

2e8e392

add jmap redact

e87b74e

fix array redact and format

7503aa1

openjdk bot added the csr Pull request needs approved CSR before integration label Jul 31, 2024

openjdk bot added serviceability serviceability-dev@openjdk.org hotspot-runtime hotspot-runtime-dev@openjdk.org labels Jul 31, 2024

remove whitespace

e738304

Henry-Lin-A marked this pull request as ready for review July 31, 2024 21:33

openjdk bot added the rfr Pull request is ready for review label Jul 31, 2024

plummercj reviewed Jul 31, 2024

View reviewed changes

turbanoff reviewed Aug 1, 2024

View reviewed changes

shipilev reviewed Aug 1, 2024

View reviewed changes

Revert "add jmap redact"

c94bacb

This reverts commit e87b74e.

Henry-Lin-A added 2 commits August 1, 2024 18:30

format HeapDumpRedactedTest.java

a57f64b

refactored redact conditional

a06f1ff

bridgekeeper bot closed this Oct 16, 2024

	JavaThing staticInt = testClass.getStaticField("staticInt");
	JavaThing staticInt = testClass.getStaticField("staticInt");

8337517: Redacted Heap Dumps #20409

8337517: Redacted Heap Dumps #20409

Uh oh!

Conversation

Henry-Lin-A commented Jul 31, 2024 • edited by openjdk bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Progress

Issues

Reviewing

Webrev

Uh oh!

bridgekeeper bot commented Jul 31, 2024

Uh oh!

openjdk bot commented Jul 31, 2024

Uh oh!

openjdk bot commented Jul 31, 2024

Uh oh!

mlbridge bot commented Jul 31, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Webrevs

Uh oh!

plummercj Jul 31, 2024

Choose a reason for hiding this comment

Uh oh!

dholmes-ora commented Aug 1, 2024

Uh oh!

turbanoff Aug 1, 2024

Choose a reason for hiding this comment

Uh oh!

AlanBateman commented Aug 1, 2024

Uh oh!

shipilev left a comment

Choose a reason for hiding this comment

Uh oh!

shipilev Aug 1, 2024

Choose a reason for hiding this comment

Uh oh!

shipilev Aug 1, 2024

Choose a reason for hiding this comment

Uh oh!

shipilev commented Aug 1, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

plummercj commented Aug 1, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

earthling-amzn commented Aug 1, 2024

Uh oh!

plummercj commented Aug 1, 2024

Uh oh!

plummercj commented Aug 1, 2024

Uh oh!

kevinjwalls commented Aug 2, 2024

Uh oh!

plummercj commented Aug 2, 2024

Uh oh!

earthling-amzn commented Aug 20, 2024

Uh oh!

bridgekeeper bot commented Sep 17, 2024

Uh oh!

bridgekeeper bot commented Oct 16, 2024

Uh oh!

Uh oh!

Henry-Lin-A commented Jul 31, 2024 •

edited by openjdk bot

Loading

mlbridge bot commented Jul 31, 2024 •

edited

Loading

shipilev commented Aug 1, 2024 •

edited

Loading

plummercj commented Aug 1, 2024 •

edited

Loading