Merge pull request #44 from paypal/issue-43-sanitize-primitive-fields

when --sanitize-byte-char-arrays-only=false is set, retain refs to objects

Author: maxxedev

README.md

@@ -153,15 +153,12 @@ Sanitize a heap dump by replacing byte and char array contents
   -e, --exclude-string-fields=<excludeStringFields>
                      String fields to exclude from sanitization. Value in com.example.MyClass#fieldName format
                        Default: java.lang.Thread#name,java.lang.ThreadGroup#name
-  -f, --force-string-coder-match=<forceMatchStringCoder>
+  -f, --force-string-coder-match=<true|false>
                      Force strings coder values to match sanitizationText.coder value
                        Default: true
-  -s, --sanitize-byte-char-arrays-only
+  -s, --sanitize-byte-char-arrays-only=<true|false>
                      Sanitize byte/char arrays only
                        Default: true
-  -S, --sanitize-arrays-only
-                     Sanitize arrays only
-                       Default: false
   -t, --text=<sanitizationText>
                      Sanitization text to replace with
                        Default: \0
@@ -172,29 +169,27 @@ Sanitize a heap dump by replacing byte and char array contents
 ### Explanation of options
 
 * `-a, --tar-input    Treat input as tar archive`
-  * Meant for use with `-` or `stdin` as inputFile when piping heap dump from k8s `kubectl cp` command which produces tar archive.
+  * Meant for use with `-` or `stdin` as inputFile when piping heap dump from k8s `kubectl cp` command which produces tar
+    archive.
 
 * `-b, --buffer-size=<bufferSize>`
-  * Higher buffer size should improve performance when reading and writing large heap dump files at the cost of higher memory usage.
+  * Higher buffer size should improve performance when reading and writing large heap dump files at the cost of higher
+    memory usage.
 
 * `-d, --docker-registry=<dockerRegistry>`
   * Meant for use with private docker-registry setups.
 
 * `-e, --exclude-string-fields=<excludeStringFields>`
   * CSV list of string fields to exclude from sanitization.
 
-* `-f, --force-string-coder-match=<forceMatchStringCoder>`
-  * Newer Java versions (Java 9+) may encode string instances differently. This setting forces sanitized value in heap dump
-    to match the coder of the sanitization text provided via `-t` flag. If unset, some sanitized string fields may not be
-    displayed correctly in analysis tools due to coder mismatch.
-
-* `-s, --sanitize-byte-char-arrays-only`
-  * When set to true, only byte[] and char[] arrays are sanitized. When false, all fields are sanitized (primitive, 
-  non-primitive, array, non-array). Be warned that some tools like VisualVM may not be able to open such
-  sanitized heap dumps; Eclipse Memory Analyzer (MAT) is known to work.
+* `-f, --force-string-coder-match=<true|false>`
+  * In Java 9+, string instances may be encoded differently based on content. This setting forces encoding of sanitized
+    strings in heap dump to match the encoding of the sanitization text provided via `-t` flag. If unset, some sanitized
+    string fields may not be displayed correctly in analysis tools due to coder mismatch.
 
-* `-S, --sanitize-arrays-only`
-  * Deprecated. Use `--sanitize-byte-char-arrays-only` instead.
+* `-s, --sanitize-byte-char-arrays-only=<true|false>`
+  * When set to true, only byte and char arrays are sanitized. When false, all primitive array fields and all primitive
+    non-array fields are sanitized.
 
 * `-t, --text=<sanitizationText>`
   * Sanitization text to replace with. Default is null character `\0`.

src/main/java/com/paypal/heapdumptool/sanitizer/HeapDumpSanitizer.java

@@ -246,7 +246,7 @@ private void copyHeapDumpClassDump(final Pipe pipe, final long classObjectId) th
 
         final int numStaticFields = pipe.pipeU2();
         for (int i = 0; i < numStaticFields; i++) {
-            pipe.pipeId();
+            pipe.pipeId(); // field name id
             final int entryType = pipe.pipeU1();
             pipeStaticField(pipe, entryType);
         }
@@ -270,7 +270,7 @@ private void copyHeapDumpClassDump(final Pipe pipe, final long classObjectId) th
     }
 
     private boolean shouldTrackClassMetadata() {
-        return !sanitizeCommand.getExcludeStringFields().isEmpty();
+        return !sanitizeCommand.getExcludeStringFields().isEmpty() || !sanitizeCommand.isSanitizeByteCharArraysOnly();
     }
 
     private boolean isAssignableClassWithExcludeStringField(final long classObjectId) {
@@ -281,7 +281,7 @@ private boolean isAssignableClassWithExcludeStringField(final long classObjectId
 
     private void pipeStaticField(final Pipe pipe, final int entryType) throws IOException {
         final int valueSize = BasicType.findValueSize(entryType, pipe.getIdSize());
-        if (isSanitizeAll()) {
+        if (shouldSanitizeField(entryType)) {
             applySanitization(pipe, valueSize);
         } else {
             pipe.pipe(valueSize);
@@ -313,12 +313,13 @@ private void copyHeapDumpInstanceDump(final Pipe pipe, final long objectId) thro
             copyStringsInstanceFields(pipe, objectId, numBytes);
 
         } else if (isAssignableClassWithExcludeStringField(classObjectId)) {
-            copyInstanceWithExcludeStringField(pipe, className, numBytes);
+            copyInstanceAndSanitizeSomeFields(pipe, className, numBytes);
 
         } else {
-            if (isSanitizeAll()) {
-                applySanitization(pipe, numBytes);
+            if (!sanitizeCommand.isSanitizeByteCharArraysOnly()) {
+                copyInstanceAndSanitizeSomeFields(pipe, className, numBytes);
             } else {
+                // no need to sanitize instance dump. sanitize (byte/char) arrays only, in array dump section
                 pipe.pipe(numBytes);
             }
         }
@@ -378,17 +379,21 @@ private Collection<String> getExcludeStringFieldsInClassHierarchy(final String c
                 .collect(Collectors.toList());
     }
 
-    private void copyInstanceWithExcludeStringField(final Pipe pipe, final String className, final long numBytes) throws IOException {
+    private void copyInstanceAndSanitizeSomeFields(final Pipe pipe, final String className, final long numBytes) throws IOException {
         final Collection<String> excludeStringFields = getExcludeStringFieldsInClassHierarchy(className);
         final ClassObject classObject = classNameToClassObjectsMap.get(className);
-        final MutableLong numBytesMutable = new MutableLong(numBytes);
         Objects.requireNonNull(classObject);
+        final MutableLong numBytesMutable = new MutableLong(numBytes);
         getAllFieldsInClassHierarchy(className).forEach(field -> {
             final int fieldSize = field.type.getValueSize(pipe.getIdSize());
 
             if (excludeStringFields.contains(field.name)) {
                 final long id = Failable.call(pipe::pipeId);
                 excludeStringObjectIds.add(id);
+
+            } else if (shouldSanitizeField(field.type.getU1Code())) {
+                Failable.run(() -> applySanitization(pipe, fieldSize));
+
             } else {
                 Failable.run(() -> pipe.pipe(fieldSize));
             }
@@ -403,10 +408,13 @@ private String getClassName(final long classObjectId) {
         return stringIdToStringMap.getOrDefault(stringId, "");
     }
 
-    private boolean isSanitizeAll() {
-        return ENABLE_SANITIZATION &&
-                !sanitizeCommand.isSanitizeByteCharArraysOnly() &&
-                !sanitizeCommand.isSanitizeArraysOnly();
+    private boolean shouldSanitizeField(final int fieldType) {
+        if (!ENABLE_SANITIZATION) {
+            return false;
+        }
+
+        final BasicType basicType = BasicType.findByU1Code(fieldType).orElse(BasicType.OBJECT);
+        return !sanitizeCommand.isSanitizeByteCharArraysOnly() && basicType != BasicType.OBJECT;
     }
 
     private void copyHeapDumpObjectArrayDump(final Pipe pipe) throws IOException {

src/main/java/com/paypal/heapdumptool/sanitizer/SanitizeCommandProcessor.java

@@ -39,9 +39,6 @@ public SanitizeCommandProcessor(final SanitizeCommand command, final SanitizeStr
 
     @Override
     public void process() throws Exception {
-        if (command.isSanitizeArraysOnly() && command.isSanitizeByteCharArraysOnly()) {
-            throw new IllegalArgumentException("sanitizeArraysOnly and sanitizeByteCharArraysOnly cannot be both set to true simultaneously");
-        }
         if (streamFactory.isStdinInput() && !command.getExcludeStringFields().isEmpty()) {
             throw new IllegalArgumentException("stdin input and excludeStringFields cannot be both set to true simultaneously");
         }

src/main/java/com/paypal/heapdumptool/sanitizer/SanitizeOrCaptureCommandBase.java

@@ -54,13 +54,6 @@ public abstract class SanitizeOrCaptureCommandBase implements CliCommand {
             showDefaultValue = ALWAYS)
     private boolean sanitizeByteCharArraysOnly = true;
 
-    @Option(names = {"-S", "--sanitize-arrays-only"},
-            description = "Sanitize arrays only",
-            arity = "1",
-            defaultValue = "false",
-            showDefaultValue = ALWAYS)
-    private boolean sanitizeArraysOnly;
-
     @Option(names = {"-t", "--text"}, description = "Sanitization text to replace with", defaultValue = "\\0", showDefaultValue = ALWAYS)
     private String sanitizationText = "\\0";
 
@@ -81,7 +74,6 @@ public void copyFrom(final SanitizeOrCaptureCommandBase other) {
         this.forceMatchStringCoder = other.forceMatchStringCoder;
         this.excludeStringFields = other.excludeStringFields;
         this.sanitizationText = other.sanitizationText;
-        this.sanitizeArraysOnly = other.sanitizeArraysOnly;
         this.sanitizeByteCharArraysOnly = other.sanitizeByteCharArraysOnly;
         this.tarInput = other.tarInput;
     }
@@ -102,14 +94,6 @@ public void setSanitizeByteCharArraysOnly(final boolean sanitizeByteCharArraysOn
         this.sanitizeByteCharArraysOnly = sanitizeByteCharArraysOnly;
     }
 
-    public boolean isSanitizeArraysOnly() {
-        return sanitizeArraysOnly;
-    }
-
-    public void setSanitizeArraysOnly(final boolean sanitizeArraysOnly) {
-        this.sanitizeArraysOnly = sanitizeArraysOnly;
-    }
-
     public boolean isTarInput() {
         return tarInput;
     }

src/main/java/com/paypal/heapdumptool/sanitizer/SanitizeStreamFactory.java

@@ -1,7 +1,7 @@
 package com.paypal.heapdumptool.sanitizer;
 
 import org.apache.commons.compress.archivers.tar.TarArchiveInputStream;
-import org.apache.commons.lang3.StringUtils;
+import org.apache.commons.lang3.Strings;
 import org.apache.commons.lang3.Validate;
 
 import java.io.BufferedInputStream;
@@ -50,7 +50,7 @@ public OutputStream newOutputStream() throws IOException {
         if (command.isZipOutput()) {
             final ZipOutputStream zipStream = new ZipOutputStream(output);
             final String name = getOutputFileName();
-            final String entryName = StringUtils.removeEnd(name, ".zip");
+            final String entryName = Strings.CS.removeEnd(name, ".zip");
             zipStream.putNextEntry(new ZipEntry(entryName));
             return zipStream;
         }
@@ -65,7 +65,7 @@ protected InputStream newInputStream(final Path inputFile) throws IOException {
 
     public boolean isStdinInput() {
         final String name = command.getInputFile().getFileName().toString();
-        return StringUtils.equalsAny(name, "-", "stdin", "0");
+        return Strings.CS.equalsAny(name, "-", "stdin", "0");
     }
 
     private static SanitizeCommand validate(final SanitizeCommand command) {

src/test/java/com/paypal/heapdumptool/ApplicationTest.java

@@ -10,6 +10,8 @@
 import org.springframework.boot.test.system.OutputCaptureExtension;
 import picocli.CommandLine;
 
+import java.util.Objects;
+
 import static com.paypal.heapdumptool.ApplicationTestSupport.runApplication;
 import static com.paypal.heapdumptool.ApplicationTestSupport.runApplicationPrivileged;
 import static com.paypal.heapdumptool.capture.PrivilegeEscalator.Escalation.REQUIRED_AND_PROMPTED;
@@ -48,6 +50,7 @@ public void testPrivilegeEscalated(final CapturedOutput output) throws Exception
                   .thenReturn(REQUIRED_AND_PROMPTED);
 
             try (final MockedConstruction<CommandLine> mockedCmd = mockConstruction(CommandLine.class, this::prepare)) {
+                Objects.requireNonNull(mockedCmd);
                 final int exitCode = runApplication("capture", "my-container");
                 assertThat(exitCode).isEqualTo(0);
             }

src/test/java/com/paypal/heapdumptool/sanitizer/HeapDumpSanitizerTest.java

@@ -50,17 +50,8 @@ class HeapDumpSanitizerTest {
     private static final Logger LOGGER = LoggerFactory.getLogger(HeapDumpSanitizerTest.class);
 
     @TempDir
-    static Path tempDir2;
     static Path tempDir;
 
-    static {
-        try {
-            tempDir = Files.createTempDirectory("");
-        } catch (final IOException e) {
-            throw new RuntimeException(e);
-        }
-    }
-
     private final SecretArrays secretArrays = new SecretArrays();
 
     @BeforeEach
@@ -187,21 +178,11 @@ void testSanitizeFieldsOfNonArrayPrimitiveType() throws Exception {
             assertThat(countOfSequence(clearHeapDump, nCopiesLongToBytes(cafegirl(), 1)))
                     .isGreaterThan(500);
         }
-
-        {
-            final byte[] clearHeapDump = loadSanitizedHeapDump("--sanitize-byte-char-arrays-only=false", "--sanitize-arrays-only=true");
-            assertThat(clearHeapDump)
-                    .overridingErrorMessage("sequences do not match") // normal error message would be long and not helpful at all
-                    .containsSequence(nCopiesLongToBytes(deadcow(), 500));
-
-            assertThat(countOfSequence(clearHeapDump, nCopiesLongToBytes(cafegirl(), 1)))
-                    .isGreaterThan(500);
-        }
     }
 
     @Test
     void testSanitizeArraysOnly() throws Exception {
-        final byte[] heapDump = loadSanitizedHeapDump("--sanitize-byte-char-arrays-only=false", "--sanitize-arrays-only=true");
+        final byte[] heapDump = loadSanitizedHeapDump("--sanitize-byte-char-arrays-only=false");
         verifyDoesNotContainsSequence(heapDump, secretArrays.getByteArraySequence());
         verifyDoesNotContainsSequence(heapDump, secretArrays.getCharArraySequence());
         verifyDoesNotContainsSequence(heapDump, secretArrays.getShortArraySequence());

src/test/java/com/paypal/heapdumptool/sanitizer/example/SimpleClass.java

@@ -0,0 +1,37 @@
+package com.paypal.heapdumptool.sanitizer.example;
+
+import java.lang.management.ManagementFactory;
+import java.time.Instant;
+import java.time.temporal.ChronoUnit;
+
+// for manual testing.
+// jcmd 186633 GC.heap_dump /tmp/heap.hprof
+// java -jar heap-dump-tool.jar sanitize /tmp/heap.hprof /tmp/sanitize.hprof --sanitize-byte-char-arrays-only=false
+// verify 0 primitive values
+// verify non-null object refs
+public class SimpleClass {
+
+    private static final Long simpleStaticLong = System.currentTimeMillis();
+    private final Long simpleInstanceLong = simpleStaticLong + 1;
+    private final int simpleInstanceInt = (int) (long) simpleInstanceLong;
+
+    private final Inner inner = new Inner();
+
+    private static class Inner {
+        private static final Long staticLong = System.currentTimeMillis();
+        private final Long instanceLong = staticLong + 1;
+        private final int instanceInt = (int) (long) instanceLong;
+    }
+
+    public static void main(final String... args) throws InterruptedException {
+        final SimpleClass simpleClass = new SimpleClass();
+        final String pid = ManagementFactory.getRuntimeMXBean().getName();
+        while (!Thread.currentThread().isInterrupted()) {
+            System.out.println("Running at " + Instant.now().truncatedTo(ChronoUnit.SECONDS) + " " + pid);
+
+            if (simpleClass.simpleInstanceLong > 0) {
+                Thread.sleep(1_000);
+            }
+        }
+    }
+}