Merge pull request #2463 from geoffw0/overflowcalc

CPP: Allocation and Deallocation libraries

Author: jbj

change-notes/1.24/analysis-cpp.md

@@ -13,11 +13,17 @@ The following changes in version 1.24 affect C/C++ analysis in all applications.
 
 | **Query**                  | **Expected impact**    | **Change**                                                       |
 |----------------------------|------------------------|------------------------------------------------------------------|
+| Buffer not sufficient for string (`cpp/overflow-calculated`) | More true positive results | This query now identifies a wider variety of buffer allocations using the `semmle.code.cpp.models.interfaces.Allocation` library. |
+| No space for zero terminator (`cpp/no-space-for-terminator`) | More true positive results | This query now identifies a wider variety of buffer allocations using the `semmle.code.cpp.models.interfaces.Allocation` library. |
+| Memory is never freed (`cpp/memory-never-freed`) | More true positive results | This query now identifies a wider variety of buffer allocations using the `semmle.code.cpp.models.interfaces.Allocation` library. |
+| Memory may not be freed (`cpp/memory-may-not-be-freed`) | More true positive results | This query now identifies a wider variety of buffer allocations using the `semmle.code.cpp.models.interfaces.Allocation` library. |
 | Hard-coded Japanese era start date (`cpp/japanese-era/exact-era-date`) |  | This query is no longer run on LGTM. |
 | No space for zero terminator (`cpp/no-space-for-terminator`) | Fewer false positive results | This query has been modified to be more conservative when identifying which pointers point to null-terminated strings.  This approach produces fewer, more accurate results. |
 
 ## Changes to libraries
 
+* Created the `semmle.code.cpp.models.interfaces.Allocation` library to model allocation such as `new` expressions and calls to `malloc`. This in intended to replace the functionality in `semmle.code.cpp.commons.Alloc` with a more consistent and useful interface.
+* Created the `semmle.code.cpp.models.interfaces.Deallocation` library to model deallocation such as `delete` expressions and calls to `free`. This in intended to replace the functionality in `semmle.code.cpp.commons.Alloc` with a more consistent and useful interface.
 * The new class `StackVariable` should be used in place of `LocalScopeVariable`
   in most cases. The difference is that `StackVariable` does not include
   variables declared with `static` or `thread_local`.

cpp/ql/src/Critical/MemoryFreed.qll

@@ -1,14 +1,7 @@
 import semmle.code.cpp.pointsto.PointsTo
 
 private predicate freed(Expr e) {
-  exists(FunctionCall fc, Expr arg |
-    freeCall(fc, arg) and
-    arg = e
-  )
-  or
-  exists(DeleteExpr de | de.getExpr() = e)
-  or
-  exists(DeleteArrayExpr de | de.getExpr() = e)
+  e = any(DeallocationExpr de).getFreedExpr()
   or
   exists(ExprCall c |
     // cautiously assume that any ExprCall could be a freeCall.
@@ -22,7 +15,4 @@ class FreedExpr extends PointsToExpr {
   override predicate interesting() { freed(this) }
 }
 
-predicate allocMayBeFreed(Expr alloc) {
-  isAllocationExpr(alloc) and
-  anythingPointsTo(alloc)
-}
+predicate allocMayBeFreed(AllocationExpr alloc) { anythingPointsTo(alloc) }

cpp/ql/src/Critical/MemoryMayNotBeFreed.ql

@@ -24,7 +24,7 @@ predicate mayCallFunction(Expr call, Function f) {
 
 predicate allocCallOrIndirect(Expr e) {
   // direct alloc call
-  isAllocationExpr(e) and
+  e.(AllocationExpr).requiresDealloc() and
   // We are only interested in alloc calls that are
   // actually freed somehow, as MemoryNeverFreed
   // will catch those that aren't.
@@ -53,8 +53,7 @@ predicate allocCallOrIndirect(Expr e) {
  * can cause memory leaks.
  */
 predicate verifiedRealloc(FunctionCall reallocCall, Variable v, ControlFlowNode verified) {
-  reallocCall.getTarget().hasGlobalOrStdName("realloc") and
-  reallocCall.getArgument(0) = v.getAnAccess() and
+  reallocCall.(AllocationExpr).getReallocPtr() = v.getAnAccess() and
   (
     exists(Variable newV, ControlFlowNode node |
       // a realloc followed by a null check at 'node' (return the non-null
@@ -71,23 +70,19 @@ predicate verifiedRealloc(FunctionCall reallocCall, Variable v, ControlFlowNode
     or
     // a realloc(ptr, 0), which always succeeds and frees
     // (return the realloc itself)
-    reallocCall.getArgument(1).getValue() = "0" and
+    reallocCall.(AllocationExpr).getReallocPtr().getValue() = "0" and
     verified = reallocCall
   )
 }
 
 predicate freeCallOrIndirect(ControlFlowNode n, Variable v) {
   // direct free call
-  freeCall(n, v.getAnAccess()) and
-  not n.(FunctionCall).getTarget().hasGlobalOrStdName("realloc")
+  n.(DeallocationExpr).getFreedExpr() = v.getAnAccess() and
+  not exists(n.(AllocationExpr).getReallocPtr())
   or
   // verified realloc call
   verifiedRealloc(_, v, n)
   or
-  n.(DeleteExpr).getExpr() = v.getAnAccess()
-  or
-  n.(DeleteArrayExpr).getExpr() = v.getAnAccess()
-  or
   exists(FunctionCall midcall, Function mid, int arg |
     // indirect free call
     n.(Call).getArgument(arg) = v.getAnAccess() and

cpp/ql/src/Critical/MemoryNeverFreed.ql

@@ -11,6 +11,8 @@
 
 import MemoryFreed
 
-from Expr alloc
-where isAllocationExpr(alloc) and not allocMayBeFreed(alloc)
+from AllocationExpr alloc
+where
+  alloc.requiresDealloc() and
+  not allocMayBeFreed(alloc)
 select alloc, "This memory is never freed"

cpp/ql/src/Critical/OverflowCalculated.ql

@@ -11,26 +11,16 @@
  */
 
 import cpp
-
-class MallocCall extends FunctionCall {
-  MallocCall() { this.getTarget().hasGlobalOrStdName("malloc") }
-
-  Expr getAllocatedSize() {
-    if this.getArgument(0) instanceof VariableAccess
-    then
-      exists(StackVariable v, ControlFlowNode def |
-        definitionUsePair(v, def, this.getArgument(0)) and
-        exprDefinition(v, def, result)
-      )
-    else result = this.getArgument(0)
-  }
-}
+import semmle.code.cpp.dataflow.DataFlow
+import semmle.code.cpp.models.interfaces.Allocation
 
 predicate spaceProblem(FunctionCall append, string msg) {
-  exists(MallocCall malloc, StrlenCall strlen, AddExpr add, FunctionCall insert, Variable buffer |
+  exists(
+    AllocationExpr malloc, StrlenCall strlen, AddExpr add, FunctionCall insert, Variable buffer
+  |
     add.getAChild() = strlen and
     exists(add.getAChild().getValue()) and
-    malloc.getAllocatedSize() = add and
+    DataFlow::localExprFlow(add, malloc.getSizeExpr()) and
     buffer.getAnAccess() = strlen.getStringExpr() and
     (
       insert.getTarget().hasGlobalOrStdName("strcpy") or

cpp/ql/src/Security/CWE/CWE-131/NoSpaceForZeroTerminator.ql

@@ -17,16 +17,11 @@
 import cpp
 import semmle.code.cpp.dataflow.DataFlow
 import semmle.code.cpp.models.interfaces.ArrayFunction
+import semmle.code.cpp.models.interfaces.Allocation
 
-class MallocCall extends FunctionCall {
-  MallocCall() { this.getTarget().hasGlobalOrStdName("malloc") }
-
-  Expr getAllocatedSize() { result = this.getArgument(0) }
-}
-
-predicate terminationProblem(MallocCall malloc, string msg) {
+predicate terminationProblem(AllocationExpr malloc, string msg) {
   // malloc(strlen(...))
-  exists(StrlenCall strlen | DataFlow::localExprFlow(strlen, malloc.getAllocatedSize())) and
+  exists(StrlenCall strlen | DataFlow::localExprFlow(strlen, malloc.getSizeExpr())) and
   // flows into a null-terminated string function
   exists(ArrayFunction af, FunctionCall fc, int arg |
     DataFlow::localExprFlow(malloc, fc.getArgument(arg)) and

cpp/ql/src/semmle/code/cpp/commons/Alloc.qll

@@ -1,207 +1,64 @@
 import cpp
+import semmle.code.cpp.models.interfaces.Allocation
+import semmle.code.cpp.models.interfaces.Deallocation
 
 /**
  * A library routine that allocates memory.
+ *
+ * DEPRECATED: Use the `AllocationFunction` class instead of this predicate.
  */
-predicate allocationFunction(Function f) {
-  exists(string name |
-    f.hasGlobalOrStdName(name) and
-    (
-      name = "malloc" or
-      name = "calloc" or
-      name = "realloc" or
-      name = "strdup" or
-      name = "wcsdup"
-    )
-    or
-    f.hasGlobalName(name) and
-    (
-      name = "_strdup" or
-      name = "_wcsdup" or
-      name = "_mbsdup" or
-      name = "ExAllocatePool" or
-      name = "ExAllocatePoolWithTag" or
-      name = "ExAllocatePoolWithTagPriority" or
-      name = "ExAllocatePoolWithQuota" or
-      name = "ExAllocatePoolWithQuotaTag" or
-      name = "ExAllocateFromLookasideListEx" or
-      name = "ExAllocateFromPagedLookasideList" or
-      name = "ExAllocateFromNPagedLookasideList" or
-      name = "ExAllocateTimer" or
-      name = "IoAllocateMdl" or
-      name = "IoAllocateWorkItem" or
-      name = "IoAllocateErrorLogEntry" or
-      name = "MmAllocateContiguousMemory" or
-      name = "MmAllocateContiguousNodeMemory" or
-      name = "MmAllocateContiguousMemorySpecifyCache" or
-      name = "MmAllocateContiguousMemorySpecifyCacheNode" or
-      name = "MmAllocateNonCachedMemory" or
-      name = "MmAllocateMappingAddress" or
-      name = "MmAllocatePagesForMdl" or
-      name = "MmAllocatePagesForMdlEx" or
-      name = "MmAllocateNodePagesForMdlEx" or
-      name = "MmMapLockedPagesWithReservedMapping" or
-      name = "MmMapLockedPages" or
-      name = "MmMapLockedPagesSpecifyCache" or
-      name = "LocalAlloc" or
-      name = "LocalReAlloc" or
-      name = "GlobalAlloc" or
-      name = "GlobalReAlloc" or
-      name = "HeapAlloc" or
-      name = "HeapReAlloc" or
-      name = "VirtualAlloc" or
-      name = "CoTaskMemAlloc" or
-      name = "CoTaskMemRealloc" or
-      name = "kmem_alloc" or
-      name = "kmem_zalloc" or
-      name = "pool_get" or
-      name = "pool_cache_get"
-    )
-  )
-}
+deprecated predicate allocationFunction(Function f) { f instanceof AllocationFunction }
 
 /**
  * A call to a library routine that allocates memory.
+ *
+ * DEPRECATED: Use `AllocationExpr` instead (this also includes `new` expressions).
  */
-predicate allocationCall(FunctionCall fc) {
-  allocationFunction(fc.getTarget()) and
-  (
-    // realloc(ptr, 0) only frees the pointer
-    fc.getTarget().hasGlobalOrStdName("realloc") implies not fc.getArgument(1).getValue() = "0"
-  )
-}
+deprecated predicate allocationCall(FunctionCall fc) { fc instanceof AllocationExpr }
 
 /**
  * A library routine that frees memory.
  */
-predicate freeFunction(Function f, int argNum) {
-  exists(string name |
-    f.hasGlobalName(name) and
-    (
-      name = "free" and argNum = 0
-      or
-      name = "realloc" and argNum = 0
-      or
-      name = "kmem_free" and argNum = 0
-      or
-      name = "pool_put" and argNum = 1
-      or
-      name = "pool_cache_put" and argNum = 1
-    )
-    or
-    f.hasGlobalOrStdName(name) and
-    (
-      name = "ExFreePoolWithTag" and argNum = 0
-      or
-      name = "ExFreeToLookasideListEx" and argNum = 1
-      or
-      name = "ExFreeToPagedLookasideList" and argNum = 1
-      or
-      name = "ExFreeToNPagedLookasideList" and argNum = 1
-      or
-      name = "ExDeleteTimer" and argNum = 0
-      or
-      name = "IoFreeMdl" and argNum = 0
-      or
-      name = "IoFreeWorkItem" and argNum = 0
-      or
-      name = "IoFreeErrorLogEntry" and argNum = 0
-      or
-      name = "MmFreeContiguousMemory" and argNum = 0
-      or
-      name = "MmFreeContiguousMemorySpecifyCache" and argNum = 0
-      or
-      name = "MmFreeNonCachedMemory" and argNum = 0
-      or
-      name = "MmFreeMappingAddress" and argNum = 0
-      or
-      name = "MmFreePagesFromMdl" and argNum = 0
-      or
-      name = "MmUnmapReservedMapping" and argNum = 0
-      or
-      name = "MmUnmapLockedPages" and argNum = 0
-      or
-      name = "LocalFree" and argNum = 0
-      or
-      name = "GlobalFree" and argNum = 0
-      or
-      name = "HeapFree" and argNum = 2
-      or
-      name = "VirtualFree" and argNum = 0
-      or
-      name = "CoTaskMemFree" and argNum = 0
-      or
-      name = "SysFreeString" and argNum = 0
-      or
-      name = "LocalReAlloc" and argNum = 0
-      or
-      name = "GlobalReAlloc" and argNum = 0
-      or
-      name = "HeapReAlloc" and argNum = 2
-      or
-      name = "CoTaskMemRealloc" and argNum = 0
-    )
-  )
-}
+predicate freeFunction(Function f, int argNum) { argNum = f.(DeallocationFunction).getFreedArg() }
 
 /**
  * A call to a library routine that frees memory.
  */
-predicate freeCall(FunctionCall fc, Expr arg) {
-  exists(int argNum |
-    freeFunction(fc.getTarget(), argNum) and
-    arg = fc.getArgument(argNum)
-  )
-}
+predicate freeCall(FunctionCall fc, Expr arg) { arg = fc.(DeallocationExpr).getFreedExpr() }
 
 /**
  * Is e some kind of allocation or deallocation (`new`, `alloc`, `realloc`, `delete`, `free` etc)?
  */
-predicate isMemoryManagementExpr(Expr e) { isAllocationExpr(e) or isDeallocationExpr(e) }
+predicate isMemoryManagementExpr(Expr e) { isAllocationExpr(e) or e instanceof DeallocationExpr }
 
 /**
  * Is e an allocation from stdlib.h (`malloc`, `realloc` etc)?
+ *
+ * DEPRECATED: Use `AllocationExpr` instead (this also includes `new` expressions).
  */
-predicate isStdLibAllocationExpr(Expr e) { allocationCall(e) }
+deprecated predicate isStdLibAllocationExpr(Expr e) { allocationCall(e) }
 
 /**
  * Is e some kind of allocation (`new`, `alloc`, `realloc` etc)?
  */
 predicate isAllocationExpr(Expr e) {
-  allocationCall(e)
+  e.(FunctionCall) instanceof AllocationExpr
   or
   e = any(NewOrNewArrayExpr new | not exists(new.getPlacementPointer()))
 }
 
 /**
  * Is e some kind of allocation (`new`, `alloc`, `realloc` etc) with a fixed size?
+ *
+ * DEPRECATED: Use `AllocationExpr.getSizeBytes()` instead.
  */
-predicate isFixedSizeAllocationExpr(Expr allocExpr, int size) {
-  exists(FunctionCall fc, string name | fc = allocExpr and name = fc.getTarget().getName() |
-    name = "malloc" and
-    size = fc.getArgument(0).getValue().toInt()
-    or
-    name = "alloca" and
-    size = fc.getArgument(0).getValue().toInt()
-    or
-    name = "calloc" and
-    size = fc.getArgument(0).getValue().toInt() * fc.getArgument(1).getValue().toInt()
-    or
-    name = "realloc" and
-    size = fc.getArgument(1).getValue().toInt() and
-    size > 0 // realloc(ptr, 0) only frees the pointer
-  )
-  or
-  size = allocExpr.(NewExpr).getAllocatedType().getSize()
-  or
-  size = allocExpr.(NewArrayExpr).getAllocatedType().getSize()
+deprecated predicate isFixedSizeAllocationExpr(Expr allocExpr, int size) {
+  size = allocExpr.(AllocationExpr).getSizeBytes()
 }
 
 /**
  * Is e some kind of deallocation (`delete`, `free`, `realloc` etc)?
+ *
+ * DEPRECATED: Use `DeallocationExpr` instead.
  */
-predicate isDeallocationExpr(Expr e) {
-  freeCall(e, _) or
-  e instanceof DeleteExpr or
-  e instanceof DeleteArrayExpr
-}
+deprecated predicate isDeallocationExpr(Expr e) { e instanceof DeallocationExpr }

cpp/ql/src/semmle/code/cpp/commons/Buffer.qll

@@ -85,7 +85,7 @@ int getBufferSize(Expr bufferExpr, Element why) {
   )
   or
   // buffer is a fixed size dynamic allocation
-  isFixedSizeAllocationExpr(bufferExpr, result) and
+  result = bufferExpr.(AllocationExpr).getSizeBytes() and
   why = bufferExpr
   or
   exists(DataFlow::ExprNode bufferExprNode |

cpp/ql/src/semmle/code/cpp/models/Models.qll

@@ -1,3 +1,5 @@
+private import implementations.Allocation
+private import implementations.Deallocation
 private import implementations.Fread
 private import implementations.IdentityFunction
 private import implementations.Inet