CPP: Make some repairs manually.

geoffw0 · geoffw0 · commit 6234b26496e7 · 2019-04-23T14:45:27.000+01:00
diff --git a/cpp/ql/src/Critical/FileMayNotBeClosed.ql b/cpp/ql/src/Critical/FileMayNotBeClosed.ql
@@ -26,18 +26,20 @@ class MinusOne extends NullValue {
  */
 predicate mayCallFunction(Expr call, Function f) {
   call.(FunctionCall).getTarget() = f or
-  call.(VariableCall).getVariable().getAnAssignedValue().getAChild*().(FunctionAccess).getTarget() =
-    f
+  call.(VariableCall).getVariable().getAnAssignedValue().
+    getAChild*().(FunctionAccess).getTarget() = f
 }
 
 predicate fopenCallOrIndirect(Expr e) {
   // direct fopen call
   fopenCall(e) and
+
   // We are only interested in fopen calls that are
   // actually closed somehow, as FileNeverClosed
   // will catch those that aren't.
   fopenCallMayBeClosed(e)
   or
+
   exists(ReturnStmt rtn |
     // indirect fopen call
     mayCallFunction(e, rtn.getEnclosingFunction()) and
@@ -84,6 +86,7 @@ class FOpenVariableReachability extends LocalScopeVariableReachabilityWithReassi
     exists(node.(AnalysedExpr).getNullSuccessor(v)) or
     fcloseCallOrIndirect(node, v) or
     assignedToFieldOrGlobal(v, node) or
+
     // node may be used directly in query
     v.getFunction() = node.(ReturnStmt).getEnclosingFunction()
   }
@@ -119,10 +122,12 @@ class FOpenReachability extends LocalScopeVariableReachabilityExt {
   }
 
   override predicate isBarrier(
-    ControlFlowNode source, ControlFlowNode node, ControlFlowNode next, LocalScopeVariable v
-  ) {
+    ControlFlowNode source, ControlFlowNode node, ControlFlowNode next,
+    LocalScopeVariable v)
+  {
     isSource(source, v) and
     next = node.getASuccessor() and
+
     // the file (stored in any variable `v0`) opened at `source` is closed or
     // assigned to a global at node, or NULL checked on the edge node -> next.
     exists(LocalScopeVariable v0 | fopenVariableReaches(v0, source, node) |
@@ -167,4 +172,6 @@ where
     fopenVariableReaches(v, def, ret) and
     ret.getAChild*() = v.getAnAccess()
   )
-select def, "The file opened here may not be closed at $@.", ret, "this exit point"
+select
+  def, "The file opened here may not be closed at $@.",
+  ret, "this exit point"
diff --git a/cpp/ql/src/Critical/MemoryFreed.qll b/cpp/ql/src/Critical/MemoryFreed.qll
@@ -22,4 +22,7 @@ class FreedExpr extends PointsToExpr {
   override predicate interesting() { freed(this) }
 }
 
-predicate allocMayBeFreed(Expr alloc) { isAllocationExpr(alloc) and anythingPointsTo(alloc) }
+predicate allocMayBeFreed(Expr alloc) {
+  isAllocationExpr(alloc) and
+  anythingPointsTo(alloc)
+}
diff --git a/cpp/ql/src/Critical/MemoryMayNotBeFreed.ql b/cpp/ql/src/Critical/MemoryMayNotBeFreed.ql
@@ -18,18 +18,20 @@ import semmle.code.cpp.controlflow.LocalScopeVariableReachability
  */
 predicate mayCallFunction(Expr call, Function f) {
   call.(FunctionCall).getTarget() = f or
-  call.(VariableCall).getVariable().getAnAssignedValue().getAChild*().(FunctionAccess).getTarget() =
-    f
+  call.(VariableCall).getVariable().getAnAssignedValue().
+    getAChild*().(FunctionAccess).getTarget() = f
 }
 
 predicate allocCallOrIndirect(Expr e) {
   // direct alloc call
   isAllocationExpr(e) and
+
   // We are only interested in alloc calls that are
   // actually freed somehow, as MemoryNeverFreed
   // will catch those that aren't.
   allocMayBeFreed(e)
   or
+
   exists(ReturnStmt rtn |
     // indirect alloc call
     mayCallFunction(e, rtn.getEnclosingFunction()) and
@@ -62,6 +64,7 @@ predicate verifiedRealloc(FunctionCall reallocCall, Variable v, ControlFlowNode
       newV.getAnAssignedValue() = reallocCall and
       node.(AnalysedExpr).getNonNullSuccessor(newV) = verified and
       // note: this case uses naive flow logic (getAnAssignedValue).
+
       // special case: if the result of the 'realloc' is assigned to the
       // same variable, we don't descriminate properly between the old
       // and the new allocation; better to not consider this a free at
@@ -113,6 +116,7 @@ class AllocVariableReachability extends LocalScopeVariableReachabilityWithReassi
     exists(node.(AnalysedExpr).getNullSuccessor(v)) or
     freeCallOrIndirect(node, v) or
     assignedToFieldOrGlobal(v, node) or
+
     // node may be used directly in query
     v.getFunction() = node.(ReturnStmt).getEnclosingFunction()
   }
@@ -148,10 +152,12 @@ class AllocReachability extends LocalScopeVariableReachabilityExt {
   }
 
   override predicate isBarrier(
-    ControlFlowNode source, ControlFlowNode node, ControlFlowNode next, LocalScopeVariable v
-  ) {
+    ControlFlowNode source, ControlFlowNode node, ControlFlowNode next,
+    LocalScopeVariable v)
+  {
     isSource(source, v) and
     next = node.getASuccessor() and
+
     // the memory (stored in any variable `v0`) allocated at `source` is freed or
     // assigned to a global at node, or NULL checked on the edge node -> next.
     exists(LocalScopeVariable v0 | allocatedVariableReaches(v0, source, node) |
@@ -196,4 +202,6 @@ where
     allocatedVariableReaches(v, def, ret) and
     ret.getAChild*() = v.getAnAccess()
   )
-select def, "The memory allocated here may not be released at $@.", ret, "this exit point"
+select
+  def, "The memory allocated here may not be released at $@.",
+  ret, "this exit point"
diff --git a/cpp/ql/src/Critical/NotInitialised.ql b/cpp/ql/src/Critical/NotInitialised.ql
@@ -11,6 +11,7 @@
 import cpp
 
 // See also InitialisationNotRun.ql and GlobalUseBeforeInit.ql
+
 /**
  * Holds if `s` defines variable `v` (conservative).
  */
diff --git a/cpp/ql/src/Critical/OverflowDestination.ql b/cpp/ql/src/Critical/OverflowDestination.ql
@@ -33,10 +33,12 @@ predicate sourceSized(FunctionCall fc, Expr src) {
     fc.getArgument(2) = size and
     src = v.getAnAccess() and
     size.getAChild+() = v.getAnAccess() and
+
     // exception: `dest` is also referenced in the size argument
     not exists(Variable other |
       dest = other.getAnAccess() and size.getAChild+() = other.getAnAccess()
     ) and
+
     // exception: `src` and `dest` are both arrays of the same type and size
     not exists(ArrayType srctype, ArrayType desttype |
       dest.getType().getUnderlyingType() = desttype and
diff --git a/cpp/ql/src/Critical/OverflowStatic.ql b/cpp/ql/src/Critical/OverflowStatic.ql
@@ -33,6 +33,7 @@ class BufferAccess extends ArrayExpr {
       staticBuffer(this.getArrayBase(), _, size) and
       size != 0
     ) and
+
     // exclude accesses in macro implementation of `strcmp`,
     // which are carefully controlled but can look dangerous.
     not exists(Macro m |

Original file line number	Diff line number	Diff line change
`@@ -22,4 +22,7 @@ class FreedExpr extends PointsToExpr {`
`22`	`22`	`override predicate interesting() { freed(this) }`
`23`	`23`	`}`
`24`	`24`
`25`		`-predicate allocMayBeFreed(Expr alloc) { isAllocationExpr(alloc) and anythingPointsTo(alloc) }`
	`25`	`+predicate allocMayBeFreed(Expr alloc) {`
	`26`	`+ isAllocationExpr(alloc) and`
	`27`	`+ anythingPointsTo(alloc)`
	`28`	`+}`