From 7eaad201460a779c21e79b0e427a9a9fed9b230f Mon Sep 17 00:00:00 2001 From: Luke Cartey Date: Thu, 24 Oct 2024 12:00:15 +0100 Subject: [PATCH 1/5] Compatible types: performance improvements Modify the `Compatible.qll` library to improve performance by restricting to function declarations for the same function. Adopt the Compatible library in DCL40-C, which has also improved detection of compatible types. --- .../IncompatibleFunctionDeclarations.ql | 36 ++++++++++--------- change_notes/2024-10-24-compatible-types.md | 2 ++ .../src/codingstandards/cpp/Compatible.qll | 4 +++ 3 files changed, 25 insertions(+), 17 deletions(-) create mode 100644 change_notes/2024-10-24-compatible-types.md diff --git a/c/cert/src/rules/DCL40-C/IncompatibleFunctionDeclarations.ql b/c/cert/src/rules/DCL40-C/IncompatibleFunctionDeclarations.ql index 4660e69b68..20b6e5e59e 100644 --- a/c/cert/src/rules/DCL40-C/IncompatibleFunctionDeclarations.ql +++ b/c/cert/src/rules/DCL40-C/IncompatibleFunctionDeclarations.ql @@ -16,30 +16,32 @@ import cpp import codingstandards.c.cert +import codingstandards.cpp.Compatible import ExternalIdentifiers -//checks if they are incompatible based on return type, number of parameters and parameter types -predicate checkMatchingFunction(FunctionDeclarationEntry d, FunctionDeclarationEntry d2) { - not d.getType() = d2.getType() - or - not d.getNumberOfParameters() = d2.getNumberOfParameters() - or - exists(ParameterDeclarationEntry p, ParameterDeclarationEntry p2, int i | - d.getParameterDeclarationEntry(i) = p and - d2.getParameterDeclarationEntry(i) = p2 and - not p.getType() = p2.getType() - ) -} - from ExternalIdentifiers d, FunctionDeclarationEntry f1, FunctionDeclarationEntry f2 where not isExcluded(f1, Declarations2Package::incompatibleFunctionDeclarationsQuery()) and not isExcluded(f2, Declarations2Package::incompatibleFunctionDeclarationsQuery()) and - f1 = d.getADeclarationEntry() and - f2 = d.getADeclarationEntry() and not f1 = f2 and - f1.getLocation().getStartLine() >= f2.getLocation().getStartLine() and + f1.getDeclaration() = d and + f2.getDeclaration() = d and f1.getName() = f2.getName() and - checkMatchingFunction(f1, f2) + ( + //return type check + not typesCompatible(f1.getType(), f2.getType()) + or + //parameter type check + parameterTypesIncompatible(f1, f2) + or + not f1.getNumberOfParameters() = f2.getNumberOfParameters() + ) and + // Apply ordering on start line, trying to avoid the optimiser applying this join too early + // in the pipeline + exists(int f1Line, int f2Line | + f1.getLocation().hasLocationInfo(_, f1Line, _, _, _) and + f2.getLocation().hasLocationInfo(_, f2Line, _, _, _) and + f1Line >= f2Line + ) select f1, "The object $@ is not compatible with re-declaration $@", f1, f1.getName(), f2, f2.getName() diff --git a/change_notes/2024-10-24-compatible-types.md b/change_notes/2024-10-24-compatible-types.md new file mode 100644 index 0000000000..05afbd64d9 --- /dev/null +++ b/change_notes/2024-10-24-compatible-types.md @@ -0,0 +1,2 @@ + - `DCL40-C` - `IncompatibleFunctionDeclarations.ql`: + - Reduce false positives by identifying compatible integer arithmetic types (e.g. "signed int" and "int"). \ No newline at end of file diff --git a/cpp/common/src/codingstandards/cpp/Compatible.qll b/cpp/common/src/codingstandards/cpp/Compatible.qll index 12a53965fe..0f6e2108ff 100644 --- a/cpp/common/src/codingstandards/cpp/Compatible.qll +++ b/cpp/common/src/codingstandards/cpp/Compatible.qll @@ -1,5 +1,7 @@ import cpp +pragma[noinline] +pragma[nomagic] predicate typesCompatible(Type t1, Type t2) { t1 = t2 or @@ -8,6 +10,7 @@ predicate typesCompatible(Type t1, Type t2) { } predicate parameterTypesIncompatible(FunctionDeclarationEntry f1, FunctionDeclarationEntry f2) { + f1.getDeclaration() = f2.getDeclaration() and exists(ParameterDeclarationEntry p1, ParameterDeclarationEntry p2, int i | p1 = f1.getParameterDeclarationEntry(i) and p2 = f2.getParameterDeclarationEntry(i) @@ -17,6 +20,7 @@ predicate parameterTypesIncompatible(FunctionDeclarationEntry f1, FunctionDeclar } predicate parameterNamesIncompatible(FunctionDeclarationEntry f1, FunctionDeclarationEntry f2) { + f1.getDeclaration() = f2.getDeclaration() and exists(ParameterDeclarationEntry p1, ParameterDeclarationEntry p2, int i | p1 = f1.getParameterDeclarationEntry(i) and p2 = f2.getParameterDeclarationEntry(i) From dfdfe9b6984e8d83c977e65f4bd6e3f97d82f047 Mon Sep 17 00:00:00 2001 From: Luke Cartey Date: Thu, 24 Oct 2024 12:06:38 +0100 Subject: [PATCH 2/5] NotDistinctIdentifier: Performance optimization Hint optimizer to perform join of exclusions after determining results. --- .../notdistinctidentifier/NotDistinctIdentifier.qll | 12 +++++++++++- 1 file changed, 11 insertions(+), 1 deletion(-) diff --git a/cpp/common/src/codingstandards/cpp/rules/notdistinctidentifier/NotDistinctIdentifier.qll b/cpp/common/src/codingstandards/cpp/rules/notdistinctidentifier/NotDistinctIdentifier.qll index 093b804e0f..102c53428b 100644 --- a/cpp/common/src/codingstandards/cpp/rules/notdistinctidentifier/NotDistinctIdentifier.qll +++ b/cpp/common/src/codingstandards/cpp/rules/notdistinctidentifier/NotDistinctIdentifier.qll @@ -12,6 +12,16 @@ abstract class NotDistinctIdentifierSharedQuery extends Query { } Query getQuery() { result instanceof NotDistinctIdentifierSharedQuery } +bindingset[d, d2] +pragma[inline_late] +private predicate after(ExternalIdentifiers d, ExternalIdentifiers d2) { + exists(int dStartLine, int d2StartLine | + d.getLocation().hasLocationInfo(_, dStartLine, _, _, _) and + d2.getLocation().hasLocationInfo(_, d2StartLine, _, _, _) and + dStartLine >= d2StartLine + ) +} + query predicate problems( ExternalIdentifiers d, string message, ExternalIdentifiers d2, string nameplaceholder ) { @@ -20,10 +30,10 @@ query predicate problems( d.getName().length() >= 31 and d2.getName().length() >= 31 and not d = d2 and - d.getLocation().getStartLine() >= d2.getLocation().getStartLine() and d.getSignificantName() = d2.getSignificantName() and not d.getName() = d2.getName() and nameplaceholder = d2.getName() and + after(d, d2) and message = "External identifer " + d.getName() + " is nondistinct in characters at or over 31 limit, compared to $@" From 8f909177a25d6a68435603aa01922ef8a10383bd Mon Sep 17 00:00:00 2001 From: Luke Cartey Date: Thu, 24 Oct 2024 12:07:35 +0100 Subject: [PATCH 3/5] MSC39-C: Hint optimizer to join isExcluded after determining results --- ...CallVaArgOnAVaListThatHasAnIndeterminateValue.ql | 13 ++++++++++--- 1 file changed, 10 insertions(+), 3 deletions(-) diff --git a/c/cert/src/rules/MSC39-C/DoNotCallVaArgOnAVaListThatHasAnIndeterminateValue.ql b/c/cert/src/rules/MSC39-C/DoNotCallVaArgOnAVaListThatHasAnIndeterminateValue.ql index 821b79c8e4..2fc334ba50 100644 --- a/c/cert/src/rules/MSC39-C/DoNotCallVaArgOnAVaListThatHasAnIndeterminateValue.ql +++ b/c/cert/src/rules/MSC39-C/DoNotCallVaArgOnAVaListThatHasAnIndeterminateValue.ql @@ -71,12 +71,19 @@ predicate sameSource(VaAccess e1, VaAccess e2) { ) } +/** + * Extracted to avoid poor magic join ordering on the `isExcluded` predicate. + */ +predicate query(VaAccess va_acc, VaArgArg va_arg, FunctionCall fc) { + sameSource(va_acc, va_arg) and + fc = preceedsFC(va_acc) and + fc.getTarget().calls*(va_arg.getEnclosingFunction()) +} + from VaAccess va_acc, VaArgArg va_arg, FunctionCall fc where not isExcluded(va_acc, Contracts7Package::doNotCallVaArgOnAVaListThatHasAnIndeterminateValueQuery()) and - sameSource(va_acc, va_arg) and - fc = preceedsFC(va_acc) and - fc.getTarget().calls*(va_arg.getEnclosingFunction()) + query(va_acc, va_arg, fc) select va_acc, "The value of " + va_acc.toString() + " is indeterminate after the $@.", fc, fc.toString() From a124f6346fe76fd398660c092c8162c1eea27a50 Mon Sep 17 00:00:00 2001 From: Luke Cartey Date: Thu, 24 Oct 2024 12:08:03 +0100 Subject: [PATCH 4/5] Rule 9.4: Performance optimization of aggregate initializer pairs Refactored calculation to work top down, instead of bottom up, which ensures we are always comparing elements from within the same initializer. --- ...dInitializationOfAggregateObjectElement.ql | 60 ++++++++++++------- 1 file changed, 40 insertions(+), 20 deletions(-) diff --git a/c/misra/src/rules/RULE-9-4/RepeatedInitializationOfAggregateObjectElement.ql b/c/misra/src/rules/RULE-9-4/RepeatedInitializationOfAggregateObjectElement.ql index 4f72d6720b..8663843a78 100644 --- a/c/misra/src/rules/RULE-9-4/RepeatedInitializationOfAggregateObjectElement.ql +++ b/c/misra/src/rules/RULE-9-4/RepeatedInitializationOfAggregateObjectElement.ql @@ -60,35 +60,55 @@ int getMaxDepth(ArrayAggregateLiteral al) { else result = 1 + max(Expr child | child = al.getAnElementExpr(_) | getMaxDepth(child)) } -// internal recursive predicate for `hasMultipleInitializerExprsForSameIndex` predicate hasMultipleInitializerExprsForSameIndexInternal( - ArrayAggregateLiteral al1, ArrayAggregateLiteral al2, Expr out_al1_expr, Expr out_al2_expr + ArrayAggregateLiteral root, Expr e1, Expr e2 ) { - exists(int shared_index, Expr al1_expr, Expr al2_expr | - // an `Expr` initializing an element of the same index in both `al1` and `al2` - shared_index = [0 .. al1.getArraySize() - 1] and - al1_expr = al1.getAnElementExpr(shared_index) and - al2_expr = al2.getAnElementExpr(shared_index) and - // but not the same `Expr` - not al1_expr = al2_expr and - ( - // case A - the children are not aggregate literals - // holds if `al1` and `al2` both hold for .getElement[sharedIndex] - not al1_expr instanceof ArrayAggregateLiteral and - out_al1_expr = al1_expr and - out_al2_expr = al2_expr - or - // case B - `al1` and `al2` both have an aggregate literal child at the same index, so recurse - hasMultipleInitializerExprsForSameIndexInternal(al1_expr, al2_expr, out_al1_expr, out_al2_expr) - ) + root = e1 and root = e2 + or + exists(ArrayAggregateLiteral parent1, ArrayAggregateLiteral parent2, int shared_index | + hasMultipleInitializerExprsForSameIndexInternal(root, parent1, parent2) and + shared_index = [0 .. parent1.getArraySize() - 1] and + e1 = parent1.getAnElementExpr(shared_index) and + e2 = parent2.getAnElementExpr(shared_index) ) } +// // internal recursive predicate for `hasMultipleInitializerExprsForSameIndex` +// predicate hasMultipleInitializerExprsForSameIndexInternal( +// ArrayAggregateLiteral al1, ArrayAggregateLiteral al2, Expr out_al1_expr, Expr out_al2_expr +// ) { +// exists(int shared_index, Expr al1_expr, Expr al2_expr | +// // an `Expr` initializing an element of the same index in both `al1` and `al2` +// shared_index = [0 .. al1.getArraySize() - 1] and +// al1_expr = al1.getAnElementExpr(shared_index) and +// al2_expr = al2.getAnElementExpr(shared_index) and +// // but not the same `Expr` +// not al1_expr = al2_expr and +// ( +// // case A - the children are not aggregate literals +// // holds if `al1` and `al2` both hold for .getElement[sharedIndex] +// not al1_expr instanceof ArrayAggregateLiteral and +// out_al1_expr = al1_expr and +// out_al2_expr = al2_expr +// or +// // case B - `al1` and `al2` both have an aggregate literal child at the same index, so recurse +// hasMultipleInitializerExprsForSameIndexInternal(al1_expr, al2_expr, out_al1_expr, out_al2_expr) +// ) +// ) +// } /** * Holds if `expr1` and `expr2` both initialize the same array element of `root`. */ predicate hasMultipleInitializerExprsForSameIndex(ArrayAggregateLiteral root, Expr expr1, Expr expr2) { - hasMultipleInitializerExprsForSameIndexInternal(root, root, expr1, expr2) + hasMultipleInitializerExprsForSameIndexInternal(root, expr1, expr2) and + not root = expr1 and + not root = expr2 and + not expr1 = expr2 and + ( + not expr1 instanceof ArrayAggregateLiteral + or + not expr2 instanceof ArrayAggregateLiteral + ) } /** From 10b84ec9ec41aea5fb96114cadf24662c410ac71 Mon Sep 17 00:00:00 2001 From: Luke Cartey <5377966+lcartey@users.noreply.github.com> Date: Thu, 24 Oct 2024 16:17:48 +0100 Subject: [PATCH 5/5] Remove commented-out code --- ...dInitializationOfAggregateObjectElement.ql | 24 +------------------ 1 file changed, 1 insertion(+), 23 deletions(-) diff --git a/c/misra/src/rules/RULE-9-4/RepeatedInitializationOfAggregateObjectElement.ql b/c/misra/src/rules/RULE-9-4/RepeatedInitializationOfAggregateObjectElement.ql index 8663843a78..dfe3fd8fff 100644 --- a/c/misra/src/rules/RULE-9-4/RepeatedInitializationOfAggregateObjectElement.ql +++ b/c/misra/src/rules/RULE-9-4/RepeatedInitializationOfAggregateObjectElement.ql @@ -60,6 +60,7 @@ int getMaxDepth(ArrayAggregateLiteral al) { else result = 1 + max(Expr child | child = al.getAnElementExpr(_) | getMaxDepth(child)) } +// internal recursive predicate for `hasMultipleInitializerExprsForSameIndex` predicate hasMultipleInitializerExprsForSameIndexInternal( ArrayAggregateLiteral root, Expr e1, Expr e2 ) { @@ -73,29 +74,6 @@ predicate hasMultipleInitializerExprsForSameIndexInternal( ) } -// // internal recursive predicate for `hasMultipleInitializerExprsForSameIndex` -// predicate hasMultipleInitializerExprsForSameIndexInternal( -// ArrayAggregateLiteral al1, ArrayAggregateLiteral al2, Expr out_al1_expr, Expr out_al2_expr -// ) { -// exists(int shared_index, Expr al1_expr, Expr al2_expr | -// // an `Expr` initializing an element of the same index in both `al1` and `al2` -// shared_index = [0 .. al1.getArraySize() - 1] and -// al1_expr = al1.getAnElementExpr(shared_index) and -// al2_expr = al2.getAnElementExpr(shared_index) and -// // but not the same `Expr` -// not al1_expr = al2_expr and -// ( -// // case A - the children are not aggregate literals -// // holds if `al1` and `al2` both hold for .getElement[sharedIndex] -// not al1_expr instanceof ArrayAggregateLiteral and -// out_al1_expr = al1_expr and -// out_al2_expr = al2_expr -// or -// // case B - `al1` and `al2` both have an aggregate literal child at the same index, so recurse -// hasMultipleInitializerExprsForSameIndexInternal(al1_expr, al2_expr, out_al1_expr, out_al2_expr) -// ) -// ) -// } /** * Holds if `expr1` and `expr2` both initialize the same array element of `root`. */