From 9d2ef42767df6e0a3e619a96d50a5bcfb2d7cd40 Mon Sep 17 00:00:00 2001 From: Julian Schuette Date: Thu, 16 Jul 2020 13:15:39 +0200 Subject: [PATCH 1/9] Fix: TypeResolver must reset() TypeParser and TypeManager at cleanup() as otherwise types from previous runs will remain stored in the TypeManager singleton. Even worse, these types will have assigned RecordDeclarations that contain the whole AST of previous runs and will mess up later analysis results. --- src/main/java/de/fraunhofer/aisec/cpg/passes/TypeResolver.java | 2 ++ 1 file changed, 2 insertions(+) diff --git a/src/main/java/de/fraunhofer/aisec/cpg/passes/TypeResolver.java b/src/main/java/de/fraunhofer/aisec/cpg/passes/TypeResolver.java index 0e1607e0b7..eff7140d98 100644 --- a/src/main/java/de/fraunhofer/aisec/cpg/passes/TypeResolver.java +++ b/src/main/java/de/fraunhofer/aisec/cpg/passes/TypeResolver.java @@ -254,5 +254,7 @@ public void handle(Node node) { public void cleanup() { this.firstOrderTypes.clear(); this.typeState.clear(); + TypeParser.reset(); + TypeManager.reset(); } } From b1862f018542fa5f7dc557fce029803f5e0dbde0 Mon Sep 17 00:00:00 2001 From: Julian Schuette Date: Thu, 16 Jul 2020 13:21:48 +0200 Subject: [PATCH 2/9] Fix: Nullability of TypeManager.frontend is unclear. There is code that assumes it to be non-null, but it is also explicitly set to null somewhere else. --- .../de/fraunhofer/aisec/cpg/graph/TypeManager.java | 11 ++++++++--- 1 file changed, 8 insertions(+), 3 deletions(-) diff --git a/src/main/java/de/fraunhofer/aisec/cpg/graph/TypeManager.java b/src/main/java/de/fraunhofer/aisec/cpg/graph/TypeManager.java index d0a4a72226..28609a6d7b 100644 --- a/src/main/java/de/fraunhofer/aisec/cpg/graph/TypeManager.java +++ b/src/main/java/de/fraunhofer/aisec/cpg/graph/TypeManager.java @@ -38,6 +38,8 @@ import java.util.stream.Collectors; import org.apache.commons.lang3.StringUtils; import org.apache.commons.lang3.builder.ToStringBuilder; +import org.checkerframework.checker.nullness.qual.NonNull; +import org.checkerframework.checker.nullness.qual.Nullable; import org.slf4j.Logger; import org.slf4j.LoggerFactory; @@ -49,14 +51,16 @@ public class TypeManager { List.of("byte", "short", "int", "long", "float", "double", "boolean", "char"); private static final Pattern funPointerPattern = Pattern.compile("\\(?\\*(?[^()]+)\\)?\\(.*\\)"); - private static TypeManager INSTANCE = new TypeManager(); + @NonNull private static TypeManager INSTANCE = new TypeManager(); public enum Language { JAVA, CXX } - private Map typeToRecord = new HashMap<>(); + @NonNull private Map typeToRecord = new HashMap<>(); + + @NonNull private Map> typeState = new HashMap<>(); // Stores all the unique types ObjectType as Key and Reference-/PointerTypes // as Values @@ -97,7 +101,7 @@ public static TypeManager getInstance() { return INSTANCE; } - public void setLanguageFrontend(LanguageFrontend frontend) { + public void setLanguageFrontend(@NonNull LanguageFrontend frontend) { this.frontend = frontend; } @@ -290,6 +294,7 @@ public Language getLanguage() { } } + @Nullable public LanguageFrontend getFrontend() { return frontend; } From 3f0ca96bc44fbbd8df3e918a120cae0c88d0f3f5 Mon Sep 17 00:00:00 2001 From: Julian Schuette Date: Thu, 16 Jul 2020 13:24:33 +0200 Subject: [PATCH 3/9] Fix: Tests assume that TypeManager is NOT reset()'ed after analysis (contradictory to normally expected behavior). Mockito cannot easily switch this off, as TypeManager.reset() is static). We thus introduce a "disableCleanup" config switch. --- .../fraunhofer/aisec/cpg/TranslationManager.java | 16 +++++++++------- .../java/de/fraunhofer/aisec/cpg/TestUtils.java | 1 + 2 files changed, 10 insertions(+), 7 deletions(-) diff --git a/src/main/java/de/fraunhofer/aisec/cpg/TranslationManager.java b/src/main/java/de/fraunhofer/aisec/cpg/TranslationManager.java index 4e03f17202..2a1cd4de6b 100644 --- a/src/main/java/de/fraunhofer/aisec/cpg/TranslationManager.java +++ b/src/main/java/de/fraunhofer/aisec/cpg/TranslationManager.java @@ -106,15 +106,17 @@ public CompletableFuture analyze() { throw new CompletionException(ex); } finally { outerBench.stop(); - log.debug("Cleaning up {} Passes", passesNeedCleanup.size()); - passesNeedCleanup.forEach(Pass::cleanup); + if (!this.config.disableCleanup) { + log.debug("Cleaning up {} Passes", passesNeedCleanup.size()); + passesNeedCleanup.forEach(Pass::cleanup); - if (frontendsNeedCleanup != null) { - log.debug("Cleaning up {} Frontends", frontendsNeedCleanup.size()); - frontendsNeedCleanup.forEach(LanguageFrontend::cleanup); - } + if (frontendsNeedCleanup != null) { + log.debug("Cleaning up {} Frontends", frontendsNeedCleanup.size()); + frontendsNeedCleanup.forEach(LanguageFrontend::cleanup); + } - TypeManager.getInstance().cleanup(); + TypeManager.getInstance().cleanup(); + } } return result; }); diff --git a/src/test/java/de/fraunhofer/aisec/cpg/TestUtils.java b/src/test/java/de/fraunhofer/aisec/cpg/TestUtils.java index 9a8cb14fef..35a5875247 100644 --- a/src/test/java/de/fraunhofer/aisec/cpg/TestUtils.java +++ b/src/test/java/de/fraunhofer/aisec/cpg/TestUtils.java @@ -101,6 +101,7 @@ public static List analyze( .sourceLocations(files) .topLevel(topLevel.toFile()) .loadIncludes(true) + .disableCleanup() .debugParser(true) .failOnError(true); if (usePasses) { From 578be39e5c6a1f10fb8ad39fe87af7b6f39a29b4 Mon Sep 17 00:00:00 2001 From: Julian Schuette Date: Thu, 16 Jul 2020 13:59:12 +0200 Subject: [PATCH 4/9] Add missing changes --- .../aisec/cpg/TranslationConfiguration.java | 15 +++++++++++++-- 1 file changed, 13 insertions(+), 2 deletions(-) diff --git a/src/main/java/de/fraunhofer/aisec/cpg/TranslationConfiguration.java b/src/main/java/de/fraunhofer/aisec/cpg/TranslationConfiguration.java index 98d79b4276..c6b206e946 100644 --- a/src/main/java/de/fraunhofer/aisec/cpg/TranslationConfiguration.java +++ b/src/main/java/de/fraunhofer/aisec/cpg/TranslationConfiguration.java @@ -74,6 +74,13 @@ public class TranslationConfiguration { */ public final List includeBlacklist; + /** + * Switch off cleaning up TypeManager memory after analysis. + * + *

Set this to {@code true} only for testing. + */ + public boolean disableCleanup = false; + /** should the code of a node be shown as parameter in the node * */ public final boolean codeInNodes; @@ -104,7 +111,8 @@ private TranslationConfiguration( List includeWhitelist, List includeBlacklist, List passes, - boolean codeInNodes) { + boolean codeInNodes, + boolean disableCleanup) { this.symbols = symbols; this.sourceLocations = sourceLocations; this.topLevel = topLevel; @@ -117,6 +125,7 @@ private TranslationConfiguration( this.passes = passes != null ? passes : new ArrayList<>(); // Make sure to init this AFTER sourceLocations has been set this.codeInNodes = codeInNodes; + this.disableCleanup = disableCleanup; } public static Builder builder() { @@ -166,6 +175,7 @@ public static class Builder { private List includeBlacklist = new ArrayList<>(); private List passes = new ArrayList<>(); private boolean codeInNodes = true; + private boolean disableCleanup = false; public Builder symbols(Map symbols) { this.symbols = symbols; @@ -326,7 +336,8 @@ public TranslationConfiguration build() { includeWhitelist, includeBlacklist, passes, - codeInNodes); + codeInNodes, + disableCleanup); } } } From 8bf84a4846cbdefdd739a54211a7d12f2342542b Mon Sep 17 00:00:00 2001 From: Julian Schuette Date: Thu, 16 Jul 2020 14:14:01 +0200 Subject: [PATCH 5/9] Add more missing lines --- .../de/fraunhofer/aisec/cpg/TranslationConfiguration.java | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/src/main/java/de/fraunhofer/aisec/cpg/TranslationConfiguration.java b/src/main/java/de/fraunhofer/aisec/cpg/TranslationConfiguration.java index c6b206e946..72c0016fcc 100644 --- a/src/main/java/de/fraunhofer/aisec/cpg/TranslationConfiguration.java +++ b/src/main/java/de/fraunhofer/aisec/cpg/TranslationConfiguration.java @@ -266,6 +266,11 @@ public Builder includeWhitelist(String includeFile) { return this; } + public Builder disableCleanup() { + this.disableCleanup = true; + return this; + } + /** * Adds the specified file to the include blacklist. Relative and absolute paths are supported. * From 92fbe176a7043599da59e927a67e1238a7801702 Mon Sep 17 00:00:00 2001 From: Julian Schuette Date: Thu, 16 Jul 2020 15:23:34 +0200 Subject: [PATCH 6/9] Fix: Correctly testing for empty strings --- .../java/de/fraunhofer/aisec/cpg/graph/type/TypeParser.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/main/java/de/fraunhofer/aisec/cpg/graph/type/TypeParser.java b/src/main/java/de/fraunhofer/aisec/cpg/graph/type/TypeParser.java index 23b8120da0..9bfe462b5a 100644 --- a/src/main/java/de/fraunhofer/aisec/cpg/graph/type/TypeParser.java +++ b/src/main/java/de/fraunhofer/aisec/cpg/graph/type/TypeParser.java @@ -665,7 +665,7 @@ public static Type createFrom(String type, boolean resolveAlias) { // Check if Problems during Parsing if (type.contains("?") || type.contains("org.eclipse.cdt.internal.core.dom.parser.ProblemType@") - || type.length() == 0) { + || type.trim().length() == 0) { return UnknownType.getUnknownType(); } From 458867c30ff57e4d346d708b3c32e495f2f95477 Mon Sep 17 00:00:00 2001 From: Julian Schuette Date: Thu, 16 Jul 2020 15:36:25 +0200 Subject: [PATCH 7/9] Fix: IndexOutOfBoundsError for "const auto" type --- .../java/de/fraunhofer/aisec/cpg/graph/type/TypeParser.java | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/src/main/java/de/fraunhofer/aisec/cpg/graph/type/TypeParser.java b/src/main/java/de/fraunhofer/aisec/cpg/graph/type/TypeParser.java index 9bfe462b5a..f0c8476059 100644 --- a/src/main/java/de/fraunhofer/aisec/cpg/graph/type/TypeParser.java +++ b/src/main/java/de/fraunhofer/aisec/cpg/graph/type/TypeParser.java @@ -712,6 +712,12 @@ public static Type createFrom(String type, boolean resolveAlias) { Type.Qualifier qualifier = calcQualifier(qualifierList, null); // Once all preceding known keywords (if any) are handled the next word must be the TypeName + if (counter >= typeBlocks.size()) { + // TODO Note that "const auto bla = ..." will end here with typeName="const" as "auto" is not + // supported. + return UnknownType.getUnknownType(); + } + assert counter < typeBlocks.size(); String typeName = typeBlocks.get(counter); counter++; From 90d66752a5418af9679371d49f9529fd406d19bb Mon Sep 17 00:00:00 2001 From: Julian Schuette Date: Thu, 16 Jul 2020 15:48:35 +0200 Subject: [PATCH 8/9] Minor: Cleanup, proper method name, @Nullable --- .../aisec/cpg/graph/type/TypeParser.java | 15 +++++++++------ 1 file changed, 9 insertions(+), 6 deletions(-) diff --git a/src/main/java/de/fraunhofer/aisec/cpg/graph/type/TypeParser.java b/src/main/java/de/fraunhofer/aisec/cpg/graph/type/TypeParser.java index f0c8476059..bbac83383c 100644 --- a/src/main/java/de/fraunhofer/aisec/cpg/graph/type/TypeParser.java +++ b/src/main/java/de/fraunhofer/aisec/cpg/graph/type/TypeParser.java @@ -32,6 +32,8 @@ import java.util.function.Supplier; import java.util.regex.Matcher; import java.util.regex.Pattern; +import org.checkerframework.checker.nullness.qual.NonNull; +import org.checkerframework.checker.nullness.qual.Nullable; /** * Class responsible for parsing the type definition and create the same Type as described by the @@ -227,7 +229,8 @@ private static int findMatching(char openBracket, char closeBracket, String stri * @param type separated type string * @return true if function pointer structure is found in typeString, false if not */ - private static Matcher isFunctionPointer(List type) { + @Nullable + private static Matcher getFunctionPtrMatcher(@NonNull List type) { StringBuilder typeStringBuilder = new StringBuilder(); for (String typePart : type) { @@ -313,7 +316,8 @@ private static void processBlockUntilLastSplit( * @param type string with the entire type definition * @return list of strings in which every piece of type information is one element of the list */ - public static List separate(String type) { + @NonNull + public static List separate(@NonNull String type) { // Remove :: CPP operator, use . instead type = type.replace("::", "."); @@ -661,7 +665,7 @@ private static ObjectType.Modifier determineModifier( * @param resolveAlias should replace with original type in typedefs * @return new type representing the type string */ - public static Type createFrom(String type, boolean resolveAlias) { + public static Type createFrom(@NonNull String type, boolean resolveAlias) { // Check if Problems during Parsing if (type.contains("?") || type.contains("org.eclipse.cdt.internal.core.dom.parser.ProblemType@") @@ -713,8 +717,7 @@ public static Type createFrom(String type, boolean resolveAlias) { // Once all preceding known keywords (if any) are handled the next word must be the TypeName if (counter >= typeBlocks.size()) { - // TODO Note that "const auto bla = ..." will end here with typeName="const" as "auto" is not - // supported. + // Note that "const auto ..." will end here with typeName="const" as auto is not supported. return UnknownType.getUnknownType(); } assert counter < typeBlocks.size(); @@ -725,7 +728,7 @@ public static Type createFrom(String type, boolean resolveAlias) { TypeManager typeManager = TypeManager.getInstance(); // Check if type is FunctionPointer - Matcher funcptr = isFunctionPointer(typeBlocks.subList(counter, typeBlocks.size())); + Matcher funcptr = getFunctionPtrMatcher(typeBlocks.subList(counter, typeBlocks.size())); if (funcptr != null) { Type returnType = createFrom(typeName, false); From bf31210f0e60144866d9246caeea22c3e9b7e1d2 Mon Sep 17 00:00:00 2001 From: Julian Schuette Date: Thu, 16 Jul 2020 16:02:50 +0200 Subject: [PATCH 9/9] Minor: No semantic changes, just a bunch of static Nullness checks and some minor corrections --- .../aisec/cpg/graph/TypeManager.java | 5 +- .../aisec/cpg/graph/type/TypeParser.java | 55 +++++++++++-------- 2 files changed, 36 insertions(+), 24 deletions(-) diff --git a/src/main/java/de/fraunhofer/aisec/cpg/graph/TypeManager.java b/src/main/java/de/fraunhofer/aisec/cpg/graph/TypeManager.java index 28609a6d7b..b1b8695ae1 100644 --- a/src/main/java/de/fraunhofer/aisec/cpg/graph/TypeManager.java +++ b/src/main/java/de/fraunhofer/aisec/cpg/graph/TypeManager.java @@ -73,6 +73,7 @@ public static void reset() { INSTANCE = new TypeManager(); } + @NonNull public Map> getTypeState() { return typeState; } @@ -182,7 +183,8 @@ private Set unwrapTypes(Collection types, WrapState wrapState) { } } - public Optional getCommonType(Collection types) { + @NonNull + public Optional getCommonType(@NonNull Collection types) { boolean sameType = types.stream().map(t -> t.getClass().getCanonicalName()).collect(Collectors.toSet()).size() @@ -286,6 +288,7 @@ private Set getAncestors(RecordDeclaration record, int depth) { return ancestors; } + @NonNull public Language getLanguage() { if (frontend instanceof JavaLanguageFrontend) { return Language.JAVA; diff --git a/src/main/java/de/fraunhofer/aisec/cpg/graph/type/TypeParser.java b/src/main/java/de/fraunhofer/aisec/cpg/graph/type/TypeParser.java index bbac83383c..2d5accfdbb 100644 --- a/src/main/java/de/fraunhofer/aisec/cpg/graph/type/TypeParser.java +++ b/src/main/java/de/fraunhofer/aisec/cpg/graph/type/TypeParser.java @@ -41,10 +41,6 @@ */ public class TypeParser { - private TypeParser() { - throw new IllegalStateException("Do not instantiate the TypeParser"); - } - public static final String UNKNOWN_TYPE_STRING = "UNKNOWN"; private static final List primitives = List.of("byte", "short", "int", "long", "float", "double", "boolean", "char"); @@ -65,6 +61,10 @@ private TypeParser() { private static final String ELABORATED_TYPE_UNION = "union"; private static final String ELABORATED_TYPE_ENUM = "enum"; + private TypeParser() { + throw new IllegalStateException("Do not instantiate the TypeParser"); + } + public static void reset() { TypeParser.languageSupplier = () -> TypeManager.getInstance().getLanguage(); } @@ -237,7 +237,7 @@ private static Matcher getFunctionPtrMatcher(@NonNull List type) { typeStringBuilder.append(typePart); } - String typeString = typeStringBuilder.toString().strip(); + String typeString = typeStringBuilder.toString().trim(); Matcher matcher = functionPtrRegex.matcher(typeString); if (matcher.find()) { @@ -247,7 +247,7 @@ private static Matcher getFunctionPtrMatcher(@NonNull List type) { } private static boolean isIncompleteType(String typeName) { - return typeName.strip().equals("void"); + return typeName.trim().equals("void"); } private static boolean isUnknownType(String typeName) { @@ -261,7 +261,8 @@ private static boolean isUnknownType(String typeName) { * @param type typeString * @return typeString without spaces in the generic Expression */ - private static String fixGenerics(String type) { + @NonNull + private static String fixGenerics(@NonNull String type) { StringBuilder out = new StringBuilder(); int bracketCount = 0; int iterator = 0; @@ -303,7 +304,7 @@ private static String fixGenerics(String type) { } private static void processBlockUntilLastSplit( - String type, int lastSplit, int newPosition, List typeBlocks) { + @NonNull String type, int lastSplit, int newPosition, @NonNull List typeBlocks) { String substr = type.substring(lastSplit, newPosition); if (substr.length() != 0) { typeBlocks.add(substr); @@ -323,9 +324,9 @@ public static List separate(@NonNull String type) { type = type.replace("::", "."); type = type.split("=")[0]; - // Guarantee that there is no arbitraty number of whitespces + // Guarantee that there is no arbitrary number of whitespaces String[] typeSubpart = type.split(" "); - type = String.join(" ", typeSubpart).strip(); + type = String.join(" ", typeSubpart).trim(); List typeBlocks = new ArrayList<>(); @@ -395,13 +396,13 @@ public static List separate(@NonNull String type) { private static List getParameterList(String parameterList) { if (parameterList.startsWith("(") && parameterList.endsWith(")")) { - parameterList = parameterList.strip().substring(1, parameterList.strip().length() - 1); + parameterList = parameterList.trim().substring(1, parameterList.trim().length() - 1); } List parameters = new ArrayList<>(); String[] parametersSplit = parameterList.split(","); for (String parameter : parametersSplit) { if (parameter.length() > 0) { - parameters.add(createFrom(parameter.strip(), true)); + parameters.add(createFrom(parameter.trim(), true)); } } @@ -414,7 +415,7 @@ private static List getGenerics(String typeName) { List genericList = new ArrayList<>(); String[] parametersSplit = generics.split(","); for (String parameter : parametersSplit) { - genericList.add(createFrom(parameter.strip(), true)); + genericList.add(createFrom(parameter.trim(), true)); } return genericList; @@ -461,12 +462,13 @@ private static Type performBracketContentAction(Type finalType, String part) { * Makes sure to apply Expressions containing brackets that change the binding of operators e.g. * () can change the binding order of operators * - * @param finalType Modifications are applyed to this type which is the result of the preceding + * @param finalType Modifications are applied to this type which is the result of the preceding * type calculations * @param bracketExpressions List of Strings containing bracket expressions * @return modified finalType performing the resolution of the bracket expressions */ - private static Type resolveBracketExpression(Type finalType, List bracketExpressions) { + private static Type resolveBracketExpression( + @NonNull Type finalType, @NonNull List bracketExpressions) { for (String bracketExpression : bracketExpressions) { List splitExpression = separate(bracketExpression.substring(1, bracketExpression.length() - 1)); @@ -479,13 +481,13 @@ private static Type resolveBracketExpression(Type finalType, List bracke } /** - * Help function that removes access modifier from the typeString + * Helper function that removes access modifier from the typeString. * * @param type provided typeString * @return typeString without access modifier */ - private static String clear(String type) { - return type.replaceAll("public|private|protected", "").strip(); + private static String clear(@NonNull String type) { + return type.replaceAll("public|private|protected", "").trim(); } /** @@ -495,7 +497,7 @@ private static String clear(String type) { * @param stringList * @return */ - private static boolean isPrimitiveType(List stringList) { + private static boolean isPrimitiveType(@NonNull List stringList) { for (String s : stringList) { if (primitives.contains(s)) { return true; @@ -511,7 +513,8 @@ private static boolean isPrimitiveType(List stringList) { * @param typeBlocks * @return separated words of compound types are joined into one string */ - private static List joinPrimitive(List typeBlocks) { + @NonNull + private static List joinPrimitive(@NonNull List typeBlocks) { List joinedTypeBlocks = new ArrayList<>(); StringBuilder primitiveType = new StringBuilder(); boolean foundPrimitive = false; @@ -547,7 +550,8 @@ private static List joinPrimitive(List typeBlocks) { * @param newRoot root the chain is swapped with * @return oldchain but root replaced with newRoot */ - public static Type reWrapType(Type oldChain, Type newRoot) { + @NonNull + public static Type reWrapType(@NonNull Type oldChain, @NonNull Type newRoot) { if (oldChain.isFirstOrderType()) { newRoot.setTypeOrigin(oldChain.getTypeOrigin()); } @@ -582,12 +586,16 @@ public static Type reWrapType(Type oldChain, Type newRoot) { * @param string the string representation of the type * @return the type */ - public static Type createIgnoringAlias(String string) { + @NonNull + public static Type createIgnoringAlias(@NonNull String string) { return createFrom(string, false); } + @NonNull private static Type postTypeParsing( - List subPart, Type finalType, List bracketExpressions) { + @NonNull List subPart, + @NonNull Type finalType, + @NonNull List bracketExpressions) { for (String part : subPart) { if (part.equals("*")) { // Creates a Pointer to the finalType @@ -665,6 +673,7 @@ private static ObjectType.Modifier determineModifier( * @param resolveAlias should replace with original type in typedefs * @return new type representing the type string */ + @NonNull public static Type createFrom(@NonNull String type, boolean resolveAlias) { // Check if Problems during Parsing if (type.contains("?")