diff --git a/checker/src/main/java/io/github/eisop/runtimeframework/checker/nullness/NullnessVerifier.java b/checker/src/main/java/io/github/eisop/runtimeframework/checker/nullness/NullnessVerifier.java index 4db17bf..4444328 100644 --- a/checker/src/main/java/io/github/eisop/runtimeframework/checker/nullness/NullnessVerifier.java +++ b/checker/src/main/java/io/github/eisop/runtimeframework/checker/nullness/NullnessVerifier.java @@ -10,8 +10,7 @@ public class NullnessVerifier implements RuntimeVerifier { private static final ClassDesc VERIFIER = ClassDesc.of(NullnessRuntimeVerifier.class.getName()); - private static final ClassDesc ATTRIBUTION_KIND = - ClassDesc.of(AttributionKind.class.getName()); + private static final ClassDesc ATTRIBUTION_KIND = ClassDesc.of(AttributionKind.class.getName()); private static final String METHOD_DEFAULT = "checkNotNull"; private static final MethodTypeDesc DESC_DEFAULT = diff --git a/checker/src/test/resources/test-cases/nullness-bridge/InheritanceBridgeTest.java b/checker/src/test/resources/test-cases/nullness-bridge/InheritanceBridgeTest.java index 2414ee8..5658b17 100644 --- a/checker/src/test/resources/test-cases/nullness-bridge/InheritanceBridgeTest.java +++ b/checker/src/test/resources/test-cases/nullness-bridge/InheritanceBridgeTest.java @@ -28,9 +28,14 @@ public static void main(String[] args) { test.finalAction(null); // cannot bridge final methods, no error here - String unsafe = test.returnAction(); + // :: error: (Return value of inherited method returnAction must be NonNull) + test.returnAction(); + + // :: error: (Return value of inherited method returnAction must be NonNull) // :: error: (Local Variable Assignment (Slot 2) must be NonNull) + String unsafe = test.returnAction(); + // :: error: (Return value of inherited method returnAction must be NonNull) @Nullable String again = test.returnAction(); } @@ -38,4 +43,4 @@ public static void main(String[] args) { public void overrideMe(@NonNull String inputA, @Nullable String inputB) { System.out.println("safe version of this method" + inputA + inputB); } -} \ No newline at end of file +} diff --git a/checker/src/test/resources/test-cases/nullness-field-read/InstanceFieldRead.java b/checker/src/test/resources/test-cases/nullness-field-read/InstanceFieldRead.java index 66ca7fd..77f30a2 100644 --- a/checker/src/test/resources/test-cases/nullness-field-read/InstanceFieldRead.java +++ b/checker/src/test/resources/test-cases/nullness-field-read/InstanceFieldRead.java @@ -8,12 +8,22 @@ static class UncheckedLib { public String poison = null; } + public static void consume(@Nullable Object o) {} + public static void main(String[] args) { UncheckedLib lib = new UncheckedLib(); - String s = lib.poison; + // 1. Read without storage (Argument passing) + // :: error: (Read Field 'poison' must be NonNull) + consume(lib.poison); + + // 2. Assignment + // :: error: (Read Field 'poison' must be NonNull) // :: error: (Local Variable Assignment (Slot 2) must be NonNull) + String s = lib.poison; - @Nullable String q = lib.poison; + // 3. Nullable Assignment (Still checks read) + // :: error: (Read Field 'poison' must be NonNull) + @Nullable String q = lib.poison; } -} +} \ No newline at end of file diff --git a/checker/src/test/resources/test-cases/nullness-field-read/StaticFieldRead.java b/checker/src/test/resources/test-cases/nullness-field-read/StaticFieldRead.java index 99673fb..c33c48d 100644 --- a/checker/src/test/resources/test-cases/nullness-field-read/StaticFieldRead.java +++ b/checker/src/test/resources/test-cases/nullness-field-read/StaticFieldRead.java @@ -1,3 +1,4 @@ +import org.checkerframework.checker.nullness.qual.Nullable; import io.github.eisop.runtimeframework.qual.AnnotatedFor; @AnnotatedFor("nullness") @@ -7,8 +8,16 @@ static class UncheckedLib { public static String POISON = null; } + public static void consume(@Nullable Object o) {} + public static void main(String[] args) { - String s = UncheckedLib.POISON; + // 1. Read without storage + // :: error: (Read Field 'POISON' must be NonNull) + consume(UncheckedLib.POISON); + + // 2. Assignment + // :: error: (Read Field 'POISON' must be NonNull) // :: error: (Local Variable Assignment (Slot 1) must be NonNull) + String s = UncheckedLib.POISON; } -} +} \ No newline at end of file diff --git a/checker/src/test/resources/test-cases/nullness-invoke/InstanceBoundary.java b/checker/src/test/resources/test-cases/nullness-invoke/InstanceBoundary.java index 7443f37..b7eb271 100644 --- a/checker/src/test/resources/test-cases/nullness-invoke/InstanceBoundary.java +++ b/checker/src/test/resources/test-cases/nullness-invoke/InstanceBoundary.java @@ -13,10 +13,11 @@ public static void main(String[] args) { UncheckedLib lib = new UncheckedLib(); String s = lib.getNull(); + // :: error: (Return value of getNull (Boundary) must be NonNull) // :: error: (Local Variable Assignment (Slot 2) must be NonNull) lib.getNull(); - // currently no explicit check on the return if its not stored + // :: error: (Return value of getNull (Boundary) must be NonNull) } } diff --git a/checker/src/test/resources/test-cases/nullness-invoke/NullableBoundary.java b/checker/src/test/resources/test-cases/nullness-invoke/NullableBoundary.java index 38b6cc2..2a7ca5b 100644 --- a/checker/src/test/resources/test-cases/nullness-invoke/NullableBoundary.java +++ b/checker/src/test/resources/test-cases/nullness-invoke/NullableBoundary.java @@ -12,5 +12,6 @@ public static String getNull() { public static void main(String[] args) { @Nullable String s = UncheckedLib.getNull(); + // :: error: (Return value of getNull (Boundary) must be NonNull) } } diff --git a/checker/src/test/resources/test-cases/nullness-invoke/StaticBoundary.java b/checker/src/test/resources/test-cases/nullness-invoke/StaticBoundary.java index 3aa1212..010b75a 100644 --- a/checker/src/test/resources/test-cases/nullness-invoke/StaticBoundary.java +++ b/checker/src/test/resources/test-cases/nullness-invoke/StaticBoundary.java @@ -12,6 +12,7 @@ public static String getNull() { public static void main(String[] args) { String s = UncheckedLib.getNull(); + // :: error: (Return value of getNull (Boundary) must be NonNull) // :: error: (Local Variable Assignment (Slot 1) must be NonNull) } diff --git a/checker/src/test/resources/test-cases/nullness-parameter/FieldArgument.java b/checker/src/test/resources/test-cases/nullness-parameter/FieldArgument.java index cdd6bbe..4050956 100644 --- a/checker/src/test/resources/test-cases/nullness-parameter/FieldArgument.java +++ b/checker/src/test/resources/test-cases/nullness-parameter/FieldArgument.java @@ -10,14 +10,17 @@ static class UncheckedLib { } public static void main(String[] args) { + // :: error: (Read Field 'POISON' must be NonNull) + // :: error: (Parameter 0 must be NonNull) consume(UncheckedLib.POISON); - nullableConsume(UncheckedLib.POISON); + + // :: error: (Read Field 'POISON' must be NonNull) + nullableConsume(UncheckedLib.POISON); } public static void consume(@NonNull String arg) { - // :: error: (Parameter 0 must be NonNull) } public static void nullableConsume(@Nullable String arg) { } -} +} \ No newline at end of file diff --git a/framework/src/main/java/io/github/eisop/runtimeframework/core/AnnotationInstrumenter.java b/framework/src/main/java/io/github/eisop/runtimeframework/core/AnnotationInstrumenter.java index efd39f9..adf349e 100644 --- a/framework/src/main/java/io/github/eisop/runtimeframework/core/AnnotationInstrumenter.java +++ b/framework/src/main/java/io/github/eisop/runtimeframework/core/AnnotationInstrumenter.java @@ -154,8 +154,14 @@ protected void generateUncheckedReturnCheck( @Override protected void generateMethodCallCheck(CodeBuilder b, InvokeInstruction invoke) { - // empty for now, only need to generate checks when a method call is stored somehwhere + RuntimeVerifier target = + policy.getBoundaryCallCheck(invoke.owner().asInternalName(), invoke.typeSymbol()); + if (target != null) { + b.dup(); + target.generateCheck( + b, TypeKind.REFERENCE, "Return value of " + invoke.name().stringValue() + " (Boundary)"); + } } @Override @@ -225,6 +231,16 @@ private void emitBridge(ClassBuilder builder, ParentMethod parentMethod) { ClassDesc.of( parentMethod.owner().thisClass().asInternalName().replace('/', '.')); codeBuilder.invokespecial(parentDesc, methodName, desc); + + RuntimeVerifier returnTarget = policy.getBridgeReturnCheck(parentMethod); + if (returnTarget != null) { + codeBuilder.dup(); + returnTarget.generateCheck( + codeBuilder, + TypeKind.REFERENCE, + "Return value of inherited method " + methodName); + } + returnResult( codeBuilder, ClassDesc.ofDescriptor(desc.returnType().descriptorString())); }); diff --git a/framework/src/main/java/io/github/eisop/runtimeframework/core/RuntimeInstrumenter.java b/framework/src/main/java/io/github/eisop/runtimeframework/core/RuntimeInstrumenter.java index 785570d..7856704 100644 --- a/framework/src/main/java/io/github/eisop/runtimeframework/core/RuntimeInstrumenter.java +++ b/framework/src/main/java/io/github/eisop/runtimeframework/core/RuntimeInstrumenter.java @@ -66,13 +66,7 @@ public void accept(ClassBuilder classBuilder, ClassElement classElement) { } else if (isFieldRead(fInst)) { codeBuilder.with(element); if (isCheckedScope) { - // generateFieldReadCheck(codeBuilder, fInst, classModel); - // Currently disabling field read checks as the GETFIELD - // and GETSTATIC instructions are not actually dangerous - // on their own. Its when we STORE a field we read from - // that an issue could arise - // TODO: consider method of turning on and off different - // boundary sites + generateFieldReadCheck(codeBuilder, fInst, classModel); } } } else if (element instanceof ReturnInstruction rInst) { diff --git a/framework/src/main/java/io/github/eisop/runtimeframework/policy/EnforcementPolicy.java b/framework/src/main/java/io/github/eisop/runtimeframework/policy/EnforcementPolicy.java index c43a62a..da9fdc5 100644 --- a/framework/src/main/java/io/github/eisop/runtimeframework/policy/EnforcementPolicy.java +++ b/framework/src/main/java/io/github/eisop/runtimeframework/policy/EnforcementPolicy.java @@ -44,6 +44,11 @@ default RuntimeVerifier getBoundaryFieldWriteCheck( /** For a bridge we are generating, what check applies to this parameter? */ RuntimeVerifier getBridgeParameterCheck(ParentMethod parentMethod, int paramIndex); + /** For a bridge we are generating, what check applies to the return value? */ + default RuntimeVerifier getBridgeReturnCheck(ParentMethod parentMethod) { + return null; + } + /** Should we check an value being stored into an array? */ RuntimeVerifier getArrayStoreCheck(TypeKind componentType); diff --git a/framework/src/main/java/io/github/eisop/runtimeframework/policy/StandardEnforcementPolicy.java b/framework/src/main/java/io/github/eisop/runtimeframework/policy/StandardEnforcementPolicy.java index 35db6d8..0b843bb 100644 --- a/framework/src/main/java/io/github/eisop/runtimeframework/policy/StandardEnforcementPolicy.java +++ b/framework/src/main/java/io/github/eisop/runtimeframework/policy/StandardEnforcementPolicy.java @@ -5,9 +5,8 @@ import io.github.eisop.runtimeframework.core.ValidationKind; import io.github.eisop.runtimeframework.filter.ClassInfo; import io.github.eisop.runtimeframework.filter.Filter; -import io.github.eisop.runtimeframework.policy.EnforcementPolicy; -import io.github.eisop.runtimeframework.runtime.AttributionKind; import io.github.eisop.runtimeframework.resolution.ParentMethod; +import io.github.eisop.runtimeframework.runtime.AttributionKind; import java.lang.classfile.Annotation; import java.lang.classfile.Attributes; import java.lang.classfile.FieldModel; @@ -146,6 +145,7 @@ public boolean shouldGenerateBridge(ParentMethod parentMethod) { // MethodTypeDesc param parsing var paramTypes = method.methodTypeSymbol().parameterList(); + // 1. Check Parameters for (int i = 0; i < paramTypes.size(); i++) { boolean explicitNoop = false; boolean explicitEnforce = false; @@ -172,6 +172,34 @@ public boolean shouldGenerateBridge(ParentMethod parentMethod) { } } } + + // 2. Check Return Type + TypeKind returnType = TypeKind.from(method.methodTypeSymbol().returnType()); + if (returnType == TypeKind.REFERENCE) { + boolean explicitNoop = false; + boolean explicitEnforce = false; + + List annos = getMethodReturnAnnotations(method); + + for (Annotation anno : annos) { + String desc = anno.classSymbol().descriptorString(); + TypeSystemConfiguration.ConfigEntry entry = configuration.find(desc); + if (entry != null) { + if (entry.kind() == ValidationKind.ENFORCE) explicitEnforce = true; + if (entry.kind() == ValidationKind.NOOP) explicitNoop = true; + } + } + + if (explicitEnforce) return true; + + if (!explicitNoop) { + TypeSystemConfiguration.ConfigEntry defaultEntry = configuration.getDefault(); + if (defaultEntry != null && defaultEntry.kind() == ValidationKind.ENFORCE) { + return true; + } + } + } + return false; } @@ -208,6 +236,33 @@ public RuntimeVerifier getBridgeParameterCheck(ParentMethod parentMethod, int pa return null; } + @Override + public RuntimeVerifier getBridgeReturnCheck(ParentMethod parentMethod) { + MethodModel method = parentMethod.method(); + TypeKind returnType = TypeKind.from(method.methodTypeSymbol().returnType()); + if (returnType != TypeKind.REFERENCE) return null; + + List annos = getMethodReturnAnnotations(method); + + RuntimeVerifier verifier = resolveVerifier(annos); + if (verifier != null) return verifier.withAttribution(AttributionKind.CALLER); + + boolean isExplicitNoop = false; + for (Annotation a : annos) { + TypeSystemConfiguration.ConfigEntry entry = + configuration.find(a.classSymbol().descriptorString()); + if (entry != null && entry.kind() == ValidationKind.NOOP) isExplicitNoop = true; + } + + if (!isExplicitNoop) { + TypeSystemConfiguration.ConfigEntry defaultEntry = configuration.getDefault(); + if (defaultEntry != null && defaultEntry.kind() == ValidationKind.ENFORCE) { + return defaultEntry.verifier().withAttribution(AttributionKind.CALLER); + } + } + return null; + } + // --- Parsing Helpers --- private List getMethodParamAnnotations(MethodModel method, int paramIndex) { List result = new ArrayList<>(); @@ -230,6 +285,24 @@ private List getMethodParamAnnotations(MethodModel method, int param return result; } + private List getMethodReturnAnnotations(MethodModel method) { + List result = new ArrayList<>(); + method + .findAttribute(Attributes.runtimeVisibleAnnotations()) + .ifPresent(attr -> result.addAll(attr.annotations())); + method + .findAttribute(Attributes.runtimeVisibleTypeAnnotations()) + .ifPresent( + attr -> { + for (TypeAnnotation ta : attr.annotations()) { + if (ta.targetInfo().targetType() == TypeAnnotation.TargetType.METHOD_RETURN) { + result.add(ta.annotation()); + } + } + }); + return result; + } + private List getFieldAnnotations(FieldModel field) { List result = new ArrayList<>(); field