muzzle field and method matching #422

realark · 2018-08-01T16:28:53Z

Merging to integration branch.

Add new muzzle checks:

Class access flags
Methods on class and access flags
Fields on class and access flags
Fixed a bug in the PatchLogger caught by muzzle.
Use a caching type pool strategy to reduce calls to getting classloader resources

With these changes muzzle can now check everything classLoaderHasClasses can check. There's still work to be done to hook up the version scan plugin, update the version ranges, and remove classLoaderHas* checks. That work will go into a later PR against the same integration branch.

mar-kolya · 2018-08-03T15:06:52Z

dd-java-agent/agent-tooling/src/main/java/datadog/trace/agent/tooling/Instrumenter.java

+              log.debug(
+                  "Instrumentation muzzled: {} -- {} on {}",
+                  instrumentationPrimaryName,
+                  this.getClass().getName(),


I think this thises would get wiped out by our current IDEA config - but I guess not by gradle formatter

Removing for consistency.

mar-kolya · 2018-08-03T15:08:20Z

dd-java-agent/agent-tooling/src/main/java/datadog/trace/agent/tooling/Utils.java

+    } finally {
+      findLoadedClassMethod.setAccessible(false);
+    }
+  }


Wasn 't the plan to remove these methods to avoid Java10 problems?

Oops, I brought that back when I resolved a merge conflict. I forgot that we intentionally removed it.

mar-kolya · 2018-08-06T14:56:40Z

dd-java-agent/agent-tooling/src/main/java/datadog/trace/agent/tooling/muzzle/Reference.java

 import java.util.HashSet;
 import java.util.List;
 import java.util.Set;
+import net.bytebuddy.jar.asm.Opcodes;
+import net.bytebuddy.jar.asm.Type;

 /** An immutable reference to a jvm class. */


This didn't change in this PR, but this is not really 'immutable' because as far as I can see it uses mutable collections.

Reference's immutability is more of a convention than a strict enforcement. I'd like to address this in another "cleanup" PR for easier reviewing.

mar-kolya · 2018-08-06T14:59:26Z

dd-java-agent/agent-tooling/src/main/java/datadog/trace/agent/tooling/muzzle/Reference.java

@@ -79,7 +80,7 @@ public Reference merge(Reference anotherReference) {

    return new Reference(
        merge(sources, anotherReference.sources),
-        merge(flags, anotherReference.flags),
+        mergeFlags(flags, anotherReference.flags),


This method has TODO in it, was that TODO meant to be addressed?

merge(fields, anotherReference.fields), merge(methods, anotherReference.methods)

these two lines below simply merge sets. Code in withField and withMethod does 'deep merging'. Is this expected? If so - could you please consider adding comment explaining why?

Merging should be a deep merge. Fixing.

The TODO in flag merging is an optimization. No need to check for PRIVATE_OR_HIGHER if PUBLIC is already being checked.

mar-kolya · 2018-08-06T15:01:02Z

dd-java-agent/agent-tooling/src/main/java/datadog/trace/agent/tooling/muzzle/Reference.java

@@ -195,111 +175,361 @@ String getMismatchDetails() {
        return "Missing class " + className;
      }
    }
+
+    public static class MissingFlag extends Mismatch {


It would have been very useful if classes had javadoc describing their meaning.

mar-kolya · 2018-08-06T15:06:14Z

dd-java-agent/agent-tooling/src/main/java/datadog/trace/agent/tooling/muzzle/Reference.java

@@ -195,111 +175,361 @@ String getMismatchDetails() {
        return "Missing class " + className;
      }
    }
+
+    public static class MissingFlag extends Mismatch {
+      final Flag expectedFlag;


It looks like majority (all?) fields on Mismatches can be made final.

mar-kolya · 2018-08-06T15:07:46Z

dd-java-agent/agent-tooling/src/main/java/datadog/trace/agent/tooling/muzzle/Reference.java

+      public ReferenceCheckError(
+          Exception e, Reference referenceBeingChecked, ClassLoader classLoaderBeingChecked) {
+        super(new Source[0]);
+        this.referenceCheckExcetpion = e;


Typo: Exception

mar-kolya

I think this looks very good. I've left a few comments, but some of them are just typos and others are just suggestions to improve design a little bit.

mar-kolya · 2018-08-06T15:12:33Z

dd-java-agent/agent-tooling/src/main/java/datadog/trace/agent/tooling/muzzle/Reference.java

+
+      @Override
+      public boolean matches(int asmFlags) {
+        return (Opcodes.ACC_PUBLIC & asmFlags) != 0 || (Opcodes.ACC_PROTECTED & asmFlags) != 0;


Instead of repeating (Opcodes.ACC_PUBLIC & asmFlags) != 0 you could consider using PUBLIC.matches

mar-kolya · 2018-08-06T15:15:16Z

dd-java-agent/agent-tooling/src/main/java/datadog/trace/agent/tooling/muzzle/Reference.java

-    NON_STATIC,
-    INTERFACE,
-    NON_INTERFACE
+  public enum Flag {


I think overall this whole file is quite large now and covers multiple concerns which makes it harder to read. You may want consider splitting 'helper' classes like this one into a separate package.

Good idea. I'll save that for a cleanup PR.

mar-kolya · 2018-08-06T15:19:34Z

dd-java-agent/agent-tooling/src/main/java/datadog/trace/agent/tooling/muzzle/Reference.java

+        Source[] sources, Flag[] flags, String name, Type returnType, Type[] parameterTypes) {
+      this(
+          new HashSet<>(Arrays.asList(sources)),
+          new HashSet<>(Arrays.asList(flags)),


This implementation makes instances mutable. Is there any reason not to:

Use Guava immutable containers

Use actual container types (lists) as inputs, not arrays: ImmutableList.of(value1, value1) is quite convenient.

mar-kolya · 2018-08-06T15:21:21Z

dd-java-agent/agent-tooling/src/main/java/datadog/trace/agent/tooling/muzzle/Reference.java

+    @Override
+    public String toString() {
+      // <init>()V
+      // toString()Ljava/lang/String;


Is this comment meant to be here? If so, I'm not sure I can follow the meaning of it

That should have been removed.

mar-kolya · 2018-08-06T15:25:53Z

dd-java-agent/agent-tooling/src/main/java/datadog/trace/agent/tooling/muzzle/Reference.java

-          }
-        }
-        return true;
+        return toString().equals(o.toString());


Could you please add a comment outlining why this is a valid way of comparing these objects?

Refactored the code to make it cleaner.

mar-kolya · 2018-08-06T16:58:14Z

...a-agent/agent-tooling/src/main/java/datadog/trace/agent/tooling/muzzle/ReferenceCreator.java

+                  .withFlag(computeMinimumClassAccess(refSourceType, paramType))
+                  .build());
+        }
+      }


Looks like some copy-paste is going on here and there might be an opportunity for some helper methods - see comments above.

mar-kolya · 2018-08-06T17:04:05Z

...a-agent/agent-tooling/src/main/java/datadog/trace/agent/tooling/muzzle/ReferenceMatcher.java

@@ -56,7 +63,7 @@ public boolean matches(final ClassLoader loader) {
            // Don't reference-check helper classes.
            // They will be injected by the instrumentation's HelperInjector.
            if (!helperClassNames.contains(reference.getClassName())) {


This is slightly unclear: wouldn't this prevent references of helper classes from being checked as well?

No, just the helper class itself won't be checked. Any reference created by the helper will be checked.

mar-kolya · 2018-08-06T17:10:31Z

...a-agent/agent-tooling/src/main/java/datadog/trace/agent/tooling/muzzle/ReferenceMatcher.java

+      }
+    }
+    if (typeOnClasspath.getSuperClass() != null) {
+      FieldDescription.InDefinedShape fieldOnSupertype =


Wouldn't this see private fields on super type as local private fields which is probably wrong?

Yes, but then an access mismatch will be raised. I think a mismatched access error is clearer than a missing field error. I'll add a comment explaining that.

mar-kolya · 2018-08-06T17:13:04Z

...a-agent/agent-tooling/src/main/java/datadog/trace/agent/tooling/muzzle/ReferenceMatcher.java

+        return methodOnSupertype;
+      }
+    }
+    return null;


This seems to be to large extent the same logic as findField - which goes back to my previous comment this there might be an opportunity to refactor things so field and method are joined into one 'member' type.

That might be best long-term, but for now I don't see much benefit of joining the two types.

mar-kolya · 2018-08-06T17:18:04Z

dd-java-agent/testing/src/test/groovy/muzzle/ReferenceCreatorTest.groovy

+      }
+    }
+    return null
+  }


Why are these helpers needed? From the looks of it in current implementation Set's .get should do exactly the same

IMO It makes the test code easier to read.

Plus misc cleanup.

Thanks for the review! Cleanup feedback backlogged and other feedback addressed.

mar-kolya · 2018-08-14T15:09:18Z

dd-java-agent/agent-tooling/src/main/java/datadog/trace/agent/tooling/muzzle/Reference.java

+        merged.set(i, merged.get(i).merge(field));
+      }
+    }
+    return new HashSet<>(merged);


This is an odd way of merging sets, that also may be slow. Would you consider using something like new HashSet<>(Sets.union(fields2, fields1))?

Yeah, I see now why this has been done this way, please ignore that comment

realark added comp: core Tracer core tag: do not merge Do not merge changes labels Aug 1, 2018

Andrew Kent added 7 commits August 1, 2018 17:25

Add notes of potential references to ReferenceCreator

d257fdb

Optimize Instrumenter logging

a8027dd

Muzzle method ref creation and matching

57c1c0d

Muzzle method and access matching

8497b81

Muzzle field matching

711e8e3

Delegate bootstrap resource loading for unloaded types

30fda8a

Add more methods to PatchLogger

c1ec598

realark force-pushed the ark/muzzle-fields-and-methods branch from 51cfe37 to c1ec598 Compare August 2, 2018 00:36

Replace UnloadedType with ByteBuddy TypeDescription

0cbef2d

realark changed the base branch from ark/muzzle-integration-branch to master August 3, 2018 02:36

realark changed the base branch from master to ark/muzzle-integration-branch August 3, 2018 02:36

realark force-pushed the ark/muzzle-fields-and-methods branch from 17f343f to 4087187 Compare August 3, 2018 03:00

realark removed the tag: do not merge Do not merge changes label Aug 3, 2018

realark requested a review from mar-kolya August 3, 2018 03:03

realark force-pushed the ark/muzzle-fields-and-methods branch 3 times, most recently from 53be9dc to 0cbef2d Compare August 3, 2018 07:49

mar-kolya reviewed Aug 3, 2018

View reviewed changes

Remove findLoadedClass

50ed54c

realark force-pushed the ark/muzzle-fields-and-methods branch from 3441638 to 50ed54c Compare August 3, 2018 21:19

mar-kolya reviewed Aug 6, 2018

View reviewed changes

mar-kolya previously requested changes Aug 6, 2018

View reviewed changes

Deep-merge Reference fields and methods

8421dfb

Plus misc cleanup.

mar-kolya reviewed Aug 14, 2018

View reviewed changes

mar-kolya approved these changes Aug 14, 2018

View reviewed changes

realark merged commit 18e713e into ark/muzzle-integration-branch Aug 14, 2018

realark deleted the ark/muzzle-fields-and-methods branch August 14, 2018 18:20

tylerbenson added this to the 0.13.0 milestone Aug 23, 2018

muzzle field and method matching #422

muzzle field and method matching #422

Conversation

realark commented Aug 1, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mar-kolya left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mar-kolya Aug 6, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

realark commented Aug 1, 2018 •

edited

mar-kolya Aug 6, 2018 •

edited