A15-2-2: Avoid infinite interpretation edge case

lcartey · lcartey · commit da1d12eb1d6a · 2023-08-30T11:17:57.000+01:00
In CodeQL CLI 2.12.7 there is a bug which causes an infinite loop
during results interpretation when a result includes more than maxPaths
paths and also includes a path with no edges i.e. where the source and
sink node are the same.

To avoid this edge case, if we report a path where the source and sink
are the same (i.e the throwingExpr directly throws an exception), we
adjust the sink node to report the constructor, which creates a one
step path from the throwingExprFlowNode to the constructor node.

This also means we can delete the `nodes` query predicate, as we only
included it to enable zero-path elements to display.
diff --git a/change_notes/2023-08-30-a15-2-2-no-zero-paths.md b/change_notes/2023-08-30-a15-2-2-no-zero-paths.md
@@ -0,0 +1 @@
+ - `A15-2-2` - all results now include an associated exception flow path to avoid a CodeQL CLI bug in 2.12.7. This includes results where an exception is thrown directly in the constructor.
diff --git a/cpp/autosar/src/rules/A15-2-2/ConstructorErrorLeavesObjectInInvalidState.ql b/cpp/autosar/src/rules/A15-2-2/ConstructorErrorLeavesObjectInInvalidState.ql
@@ -78,6 +78,14 @@ class DeleteWrapperFunction extends Function {
   Parameter getADeleteParameter() { result = p }
 }
 
+class ExceptionThrowingConstructor extends ExceptionThrowingFunction, Constructor {
+  ExceptionThrowingConstructor() {
+    exists(getAFunctionThrownType(this, _)) and
+    // The constructor is within the users source code
+    exists(getFile().getRelativePath())
+  }
+}
+
 class ExceptionThrownInConstructor extends ExceptionThrowingExpr {
   Constructor c;
 
@@ -90,24 +98,20 @@ class ExceptionThrownInConstructor extends ExceptionThrowingExpr {
   Constructor getConstructor() { result = c }
 }
 
-/**
- * Add the `nodes` predicate to ensure results with an empty path are still reported.
- */
-query predicate nodes(ExceptionFlowNode node) { any() }
-
 from
-  Constructor c, ExceptionThrownInConstructor throwingExpr, NewAllocationExpr newExpr,
-  ExceptionFlowNode exceptionSource, ExceptionFlowNode functionNode
+  ExceptionThrowingConstructor c, ExceptionThrownInConstructor throwingExpr,
+  NewAllocationExpr newExpr, ExceptionFlowNode exceptionSource,
+  ExceptionFlowNode throwingExprFlowNode, ExceptionFlowNode reportingNode
 where
   not isExcluded(c, Exceptions2Package::constructorErrorLeavesObjectInInvalidStateQuery()) and
   not isNoExceptTrue(c) and
   // Constructor must exit with an exception
   c = throwingExpr.getConstructor() and
-  throwingExpr.hasExceptionFlowReflexive(exceptionSource, functionNode, _) and
+  throwingExpr.hasExceptionFlowReflexive(exceptionSource, throwingExprFlowNode, _) and
   exists(ExceptionFlowNode mid |
     edges*(exceptionSource, mid) and
     newExpr.getASuccessor+() = mid.asThrowingExpr() and
-    edges*(mid, functionNode) and
+    edges*(mid, throwingExprFlowNode) and
     not exists(ExceptionFlowNode prior | edges(prior, mid) |
       prior.asCatchBlock().getEnclosingFunction() = c
     )
@@ -126,7 +130,16 @@ where
     DataFlow::localFlow(DataFlow::exprNode(newExpr), DataFlow::exprNode(deletedExpr)) and
     newExpr.getASuccessor+() = deletedExpr and
     deletedExpr.getASuccessor+() = throwingExpr
-  )
-select c, exceptionSource, functionNode, "Constructor throws $@ and allocates memory at $@",
+  ) and
+  // In CodeQL CLI 2.12.7 there is a bug which causes an infinite loop during results interpretation
+  // when a result includes more than maxPaths paths and also includes a path with no edges i.e.
+  // where the source and sink node are the same.
+  // To avoid this edge case, if we report a path where the source and sink are the same (i.e the
+  // throwingExpr directly throws an exception), we adjust the sink node to report the constructor,
+  // which creates a one step path from the throwingExprFlowNode to the constructor node.
+  if throwingExprFlowNode = exceptionSource
+  then reportingNode.asFunction() = c and edges(throwingExprFlowNode, reportingNode)
+  else reportingNode = throwingExprFlowNode
+select c, exceptionSource, reportingNode, "Constructor throws $@ and allocates memory at $@",
   throwingExpr, throwingExpr.(ThrowingExpr).getAnExceptionType().getExceptionName(), newExpr,
   "alloc"
diff --git a/cpp/autosar/test/rules/A15-2-2/ConstructorErrorLeavesObjectInInvalidState.expected b/cpp/autosar/test/rules/A15-2-2/ConstructorErrorLeavesObjectInInvalidState.expected
@@ -3,60 +3,40 @@ edges
 | test.cpp:13:7:13:28 | throw ... [exception] | test.cpp:14:33:16:5 | { ... } [exception] |
 | test.cpp:14:33:16:5 | { ... } [bad_alloc] | test.cpp:15:7:15:11 | re-throw exception  [bad_alloc] |
 | test.cpp:14:33:16:5 | { ... } [exception] | test.cpp:15:7:15:11 | re-throw exception  [exception] |
+| test.cpp:15:7:15:11 | re-throw exception  [bad_alloc] | test.cpp:9:3:9:8 | ClassA [bad_alloc] |
+| test.cpp:15:7:15:11 | re-throw exception  [exception] | test.cpp:9:3:9:8 | ClassA [exception] |
 | test.cpp:25:16:25:27 | new [bad_alloc] | test.cpp:27:33:30:5 | { ... } [bad_alloc] |
 | test.cpp:26:7:26:28 | throw ... [exception] | test.cpp:27:33:30:5 | { ... } [exception] |
 | test.cpp:27:33:30:5 | { ... } [bad_alloc] | test.cpp:29:7:29:11 | re-throw exception  [bad_alloc] |
 | test.cpp:27:33:30:5 | { ... } [exception] | test.cpp:29:7:29:11 | re-throw exception  [exception] |
+| test.cpp:29:7:29:11 | re-throw exception  [bad_alloc] | test.cpp:23:3:23:8 | ClassB [bad_alloc] |
+| test.cpp:29:7:29:11 | re-throw exception  [exception] | test.cpp:23:3:23:8 | ClassB [exception] |
 | test.cpp:44:16:44:27 | call to CreateMember [bad_alloc] | test.cpp:46:33:48:5 | { ... } [bad_alloc] |
 | test.cpp:45:7:45:28 | throw ... [exception] | test.cpp:46:33:48:5 | { ... } [exception] |
 | test.cpp:46:33:48:5 | { ... } [bad_alloc] | test.cpp:47:7:47:11 | re-throw exception  [bad_alloc] |
 | test.cpp:46:33:48:5 | { ... } [exception] | test.cpp:47:7:47:11 | re-throw exception  [exception] |
+| test.cpp:47:7:47:11 | re-throw exception  [bad_alloc] | test.cpp:41:3:41:8 | ClassC [bad_alloc] |
+| test.cpp:47:7:47:11 | re-throw exception  [exception] | test.cpp:41:3:41:8 | ClassC [exception] |
 | test.cpp:58:16:58:27 | call to CreateMember [bad_alloc] | test.cpp:60:33:63:5 | { ... } [bad_alloc] |
 | test.cpp:59:7:59:28 | throw ... [exception] | test.cpp:60:33:63:5 | { ... } [exception] |
 | test.cpp:60:33:63:5 | { ... } [bad_alloc] | test.cpp:62:7:62:11 | re-throw exception  [bad_alloc] |
 | test.cpp:60:33:63:5 | { ... } [exception] | test.cpp:62:7:62:11 | re-throw exception  [exception] |
+| test.cpp:62:7:62:11 | re-throw exception  [bad_alloc] | test.cpp:55:3:55:8 | ClassD [bad_alloc] |
+| test.cpp:62:7:62:11 | re-throw exception  [exception] | test.cpp:55:3:55:8 | ClassD [exception] |
+| test.cpp:77:11:77:20 | new [bad_alloc] | test.cpp:75:3:75:8 | ClassE [bad_alloc] |
+| test.cpp:78:11:78:20 | new [bad_alloc] | test.cpp:75:3:75:8 | ClassE [bad_alloc] |
 | test.cpp:80:13:80:22 | new [bad_alloc] | test.cpp:82:33:86:5 | { ... } [bad_alloc] |
 | test.cpp:81:13:81:22 | new [bad_alloc] | test.cpp:82:33:86:5 | { ... } [bad_alloc] |
 | test.cpp:82:33:86:5 | { ... } [bad_alloc] | test.cpp:85:7:85:11 | re-throw exception  [bad_alloc] |
-nodes
-| test.cpp:12:16:12:27 | new [bad_alloc] |
-| test.cpp:13:7:13:28 | throw ... [exception] |
-| test.cpp:14:33:16:5 | { ... } [bad_alloc] |
-| test.cpp:14:33:16:5 | { ... } [exception] |
-| test.cpp:15:7:15:11 | re-throw exception  [bad_alloc] |
-| test.cpp:15:7:15:11 | re-throw exception  [exception] |
-| test.cpp:25:16:25:27 | new [bad_alloc] |
-| test.cpp:26:7:26:28 | throw ... [exception] |
-| test.cpp:27:33:30:5 | { ... } [bad_alloc] |
-| test.cpp:27:33:30:5 | { ... } [exception] |
-| test.cpp:29:7:29:11 | re-throw exception  [bad_alloc] |
-| test.cpp:29:7:29:11 | re-throw exception  [exception] |
-| test.cpp:44:16:44:27 | call to CreateMember [bad_alloc] |
-| test.cpp:45:7:45:28 | throw ... [exception] |
-| test.cpp:46:33:48:5 | { ... } [bad_alloc] |
-| test.cpp:46:33:48:5 | { ... } [exception] |
-| test.cpp:47:7:47:11 | re-throw exception  [bad_alloc] |
-| test.cpp:47:7:47:11 | re-throw exception  [exception] |
-| test.cpp:58:16:58:27 | call to CreateMember [bad_alloc] |
-| test.cpp:59:7:59:28 | throw ... [exception] |
-| test.cpp:60:33:63:5 | { ... } [bad_alloc] |
-| test.cpp:60:33:63:5 | { ... } [exception] |
-| test.cpp:62:7:62:11 | re-throw exception  [bad_alloc] |
-| test.cpp:62:7:62:11 | re-throw exception  [exception] |
-| test.cpp:77:11:77:20 | new [bad_alloc] |
-| test.cpp:78:11:78:20 | new [bad_alloc] |
-| test.cpp:80:13:80:22 | new [bad_alloc] |
-| test.cpp:81:13:81:22 | new [bad_alloc] |
-| test.cpp:82:33:86:5 | { ... } [bad_alloc] |
-| test.cpp:85:7:85:11 | re-throw exception  [bad_alloc] |
-| test.cpp:87:11:87:20 | new [bad_alloc] |
+| test.cpp:85:7:85:11 | re-throw exception  [bad_alloc] | test.cpp:75:3:75:8 | ClassE [bad_alloc] |
+| test.cpp:87:11:87:20 | new [bad_alloc] | test.cpp:75:3:75:8 | ClassE [bad_alloc] |
 #select
 | test.cpp:9:3:9:8 | ClassA | test.cpp:13:7:13:28 | throw ... [exception] | test.cpp:15:7:15:11 | re-throw exception  [exception] | Constructor throws $@ and allocates memory at $@ | test.cpp:15:7:15:11 | re-throw exception  | std::bad_alloc | test.cpp:12:16:12:27 | new | alloc |
 | test.cpp:9:3:9:8 | ClassA | test.cpp:13:7:13:28 | throw ... [exception] | test.cpp:15:7:15:11 | re-throw exception  [exception] | Constructor throws $@ and allocates memory at $@ | test.cpp:15:7:15:11 | re-throw exception  | std::exception | test.cpp:12:16:12:27 | new | alloc |
 | test.cpp:41:3:41:8 | ClassC | test.cpp:45:7:45:28 | throw ... [exception] | test.cpp:47:7:47:11 | re-throw exception  [exception] | Constructor throws $@ and allocates memory at $@ | test.cpp:47:7:47:11 | re-throw exception  | std::bad_alloc | test.cpp:44:16:44:27 | call to CreateMember | alloc |
 | test.cpp:41:3:41:8 | ClassC | test.cpp:45:7:45:28 | throw ... [exception] | test.cpp:47:7:47:11 | re-throw exception  [exception] | Constructor throws $@ and allocates memory at $@ | test.cpp:47:7:47:11 | re-throw exception  | std::exception | test.cpp:44:16:44:27 | call to CreateMember | alloc |
-| test.cpp:75:3:75:8 | ClassE | test.cpp:78:11:78:20 | new [bad_alloc] | test.cpp:78:11:78:20 | new [bad_alloc] | Constructor throws $@ and allocates memory at $@ | test.cpp:78:11:78:20 | new | std::bad_alloc | test.cpp:77:11:77:20 | new | alloc |
-| test.cpp:75:3:75:8 | ClassE | test.cpp:87:11:87:20 | new [bad_alloc] | test.cpp:87:11:87:20 | new [bad_alloc] | Constructor throws $@ and allocates memory at $@ | test.cpp:87:11:87:20 | new | std::bad_alloc | test.cpp:77:11:77:20 | new | alloc |
-| test.cpp:75:3:75:8 | ClassE | test.cpp:87:11:87:20 | new [bad_alloc] | test.cpp:87:11:87:20 | new [bad_alloc] | Constructor throws $@ and allocates memory at $@ | test.cpp:87:11:87:20 | new | std::bad_alloc | test.cpp:78:11:78:20 | new | alloc |
-| test.cpp:75:3:75:8 | ClassE | test.cpp:87:11:87:20 | new [bad_alloc] | test.cpp:87:11:87:20 | new [bad_alloc] | Constructor throws $@ and allocates memory at $@ | test.cpp:87:11:87:20 | new | std::bad_alloc | test.cpp:80:13:80:22 | new | alloc |
-| test.cpp:75:3:75:8 | ClassE | test.cpp:87:11:87:20 | new [bad_alloc] | test.cpp:87:11:87:20 | new [bad_alloc] | Constructor throws $@ and allocates memory at $@ | test.cpp:87:11:87:20 | new | std::bad_alloc | test.cpp:81:13:81:22 | new | alloc |
+| test.cpp:75:3:75:8 | ClassE | test.cpp:78:11:78:20 | new [bad_alloc] | test.cpp:75:3:75:8 | ClassE [bad_alloc] | Constructor throws $@ and allocates memory at $@ | test.cpp:78:11:78:20 | new | std::bad_alloc | test.cpp:77:11:77:20 | new | alloc |
+| test.cpp:75:3:75:8 | ClassE | test.cpp:87:11:87:20 | new [bad_alloc] | test.cpp:75:3:75:8 | ClassE [bad_alloc] | Constructor throws $@ and allocates memory at $@ | test.cpp:87:11:87:20 | new | std::bad_alloc | test.cpp:77:11:77:20 | new | alloc |
+| test.cpp:75:3:75:8 | ClassE | test.cpp:87:11:87:20 | new [bad_alloc] | test.cpp:75:3:75:8 | ClassE [bad_alloc] | Constructor throws $@ and allocates memory at $@ | test.cpp:87:11:87:20 | new | std::bad_alloc | test.cpp:78:11:78:20 | new | alloc |
+| test.cpp:75:3:75:8 | ClassE | test.cpp:87:11:87:20 | new [bad_alloc] | test.cpp:75:3:75:8 | ClassE [bad_alloc] | Constructor throws $@ and allocates memory at $@ | test.cpp:87:11:87:20 | new | std::bad_alloc | test.cpp:80:13:80:22 | new | alloc |
+| test.cpp:75:3:75:8 | ClassE | test.cpp:87:11:87:20 | new [bad_alloc] | test.cpp:75:3:75:8 | ClassE [bad_alloc] | Constructor throws $@ and allocates memory at $@ | test.cpp:87:11:87:20 | new | std::bad_alloc | test.cpp:81:13:81:22 | new | alloc |

Original file line number	Diff line number	Diff line change
`@@ -0,0 +1 @@`
	`1`	+ - `A15-2-2` - all results now include an associated exception flow path to avoid a CodeQL CLI bug in 2.12.7. This includes results where an exception is thrown directly in the constructor.