Merge branch 'main' into rust-experiment

Author: Paolo Tranquilli

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

@@ -57,7 +57,7 @@ private int isSource(Expr bufferExpr, Element why) {
   exists(Type bufferType |
     // buffer is the address of a variable
     why = bufferExpr.(AddressOfExpr).getAddressable() and
-    bufferType = why.(Variable).getType() and
+    bufferType = why.(Variable).getUnspecifiedType() and
     result = bufferType.getSize() and
     not bufferType instanceof ReferenceType and
     not any(Union u).getAMemberVariable() = why

cpp/ql/lib/semmle/code/cpp/ir/implementation/raw/internal/TranslatedFunction.qll

@@ -209,8 +209,13 @@ class TranslatedFunction extends TranslatedRootElement, TTranslatedFunction {
       (
         // Only generate the `Unwind` instruction if there is any exception
         // handling present in the function.
-        exists(TryStmt try | try.getEnclosingFunction() = func) or
+        exists(TryOrMicrosoftTryStmt try | try.getEnclosingFunction() = func)
+        or
         exists(ThrowExpr throw | throw.getEnclosingFunction() = func)
+        or
+        exists(FunctionCall call | call.getEnclosingFunction() = func |
+          getTranslatedExpr(call).(TranslatedCallExpr).mayThrowException()
+        )
       )
       or
       tag = AliasedUseTag() and

cpp/ql/lib/semmle/code/cpp/ir/implementation/raw/internal/TranslatedStmt.qll

@@ -79,11 +79,6 @@ class TranslatedMicrosoftTryExceptHandler extends TranslatedElement,
     tag = TryExceptCompareOneBranch() and
     opcode instanceof Opcode::ConditionalBranch and
     resultType = getVoidType()
-    or
-    // unwind stack
-    tag = UnwindTag() and
-    opcode instanceof Opcode::Unwind and
-    resultType = getVoidType()
   }
 
   final override Instruction getInstructionRegisterOperand(InstructionTag tag, OperandTag operandTag) {
@@ -156,7 +151,7 @@ class TranslatedMicrosoftTryExceptHandler extends TranslatedElement,
       // TODO: This is not really correct. The semantics of `EXCEPTION_CONTINUE_EXECUTION` is that
       // we should continue execution at the point where the exception occurred. But we don't have
       // any instruction to model this behavior.
-      result = this.getInstruction(UnwindTag())
+      result = this.getExceptionSuccessorInstruction(any(GotoEdge edge))
       or
       kind instanceof FalseEdge and
       result = this.getInstruction(TryExceptGenerateZero())
@@ -176,7 +171,7 @@ class TranslatedMicrosoftTryExceptHandler extends TranslatedElement,
     tag = TryExceptCompareZeroBranch() and
     (
       kind instanceof TrueEdge and
-      result = this.getInstruction(UnwindTag())
+      result = this.getExceptionSuccessorInstruction(any(GotoEdge edge))
       or
       kind instanceof FalseEdge and
       result = this.getInstruction(TryExceptGenerateOne())
@@ -196,10 +191,6 @@ class TranslatedMicrosoftTryExceptHandler extends TranslatedElement,
     tag = TryExceptCompareOneBranch() and
     kind instanceof TrueEdge and
     result = this.getTranslatedHandler().getFirstInstruction(any(GotoEdge edge))
-    or
-    // Unwind -> Parent
-    tag = UnwindTag() and
-    result = this.getParent().getChildSuccessor(this, kind)
   }
 
   override Instruction getChildSuccessorInternal(TranslatedElement child, EdgeKind kind) {
@@ -215,8 +206,6 @@ class TranslatedMicrosoftTryExceptHandler extends TranslatedElement,
 
   override Instruction getALastInstructionInternal() {
     result = this.getTranslatedHandler().getALastInstruction()
-    or
-    result = this.getInstruction(UnwindTag())
   }
 
   private TranslatedExpr getTranslatedCondition() {
@@ -236,6 +225,12 @@ class TranslatedMicrosoftTryExceptHandler extends TranslatedElement,
   }
 
   final override Function getFunction() { result = tryExcept.getEnclosingFunction() }
+
+  override Instruction getExceptionSuccessorInstruction(EdgeKind kind) {
+    // A throw from within a `__except` block flows to the handler for the parent of
+    // the `__try`.
+    result = this.getParent().getParent().getExceptionSuccessorInstruction(kind)
+  }
 }
 
 abstract class TranslatedStmt extends TranslatedElement, TTranslatedStmt {
@@ -583,7 +578,7 @@ class TranslatedNoValueReturnStmt extends TranslatedReturnStmt, TranslatedVariab
 /**
  * A C/C++ `try` statement, or a `__try __except` or `__try __finally` statement.
  */
-private class TryOrMicrosoftTryStmt extends Stmt {
+class TryOrMicrosoftTryStmt extends Stmt {
   TryOrMicrosoftTryStmt() {
     this instanceof TryStmt or
     this instanceof MicrosoftTryStmt

cpp/ql/lib/semmle/code/cpp/security/BufferAccess.qll

@@ -14,7 +14,11 @@ int getPointedSize(Type t) {
  * BufferWrite differ.
  */
 abstract class BufferAccess extends Expr {
-  BufferAccess() { not this.isUnevaluated() }
+  BufferAccess() {
+    not this.isUnevaluated() and
+    //A buffer access must be reachable (not in dead code)
+    reachable(this)
+  }
 
   abstract string getName();
 
@@ -26,6 +30,8 @@ abstract class BufferAccess extends Expr {
    * - 1 = buffer range [0, getSize) is accessed entirely.
    * - 2 = buffer range [0, getSize) may be accessed partially or entirely.
    * - 3 = buffer is accessed at offset getSize - 1.
+   * - 4 = buffer is accessed with null terminator read protections
+   *       (does not read past null terminator, regardless of access size)
    */
   abstract Expr getBuffer(string bufferDesc, int accessType);
 
@@ -128,7 +134,7 @@ class StrncpyBA extends BufferAccess {
     or
     result = this.(FunctionCall).getArgument(1) and
     bufferDesc = "source buffer" and
-    accessType = 2
+    accessType = 4
   }
 
   override Expr getSizeExpr() { result = this.(FunctionCall).getArgument(2) }

cpp/ql/src/Critical/OverflowStatic.ql

@@ -42,7 +42,9 @@ class BufferAccess extends ArrayExpr {
     not exists(Macro m |
       m.getName() = "strcmp" and
       m.getAnInvocation().getAnExpandedElement() = this
-    )
+    ) and
+    //A buffer access must be reachable (not in dead code)
+    reachable(this)
   }
 
   int bufferSize() { staticBuffer(this.getArrayBase(), _, result) }

cpp/ql/src/Security/CWE/CWE-119/OverflowBuffer.ql

@@ -26,6 +26,7 @@ from
   BufferAccess ba, string bufferDesc, int accessSize, int accessType, Element bufferAlloc,
   int bufferSize, string message
 where
+  accessType != 4 and
   accessSize = ba.getSize() and
   bufferSize = getBufferSize(ba.getBuffer(bufferDesc, accessType), bufferAlloc) and
   (

cpp/ql/src/change-notes/2024-09-04-overflow-buffer-false-positives.md

@@ -0,0 +1,5 @@
+---
+category: minorAnalysis
+---
+* Removed false positives caused by buffer accesses in unreachable code
+* Removed false positives caused by inconsistent type checking

cpp/ql/test/library-tests/ir/ir/aliased_ir.expected

@@ -3167,41 +3167,45 @@ ir.c:
 #   36|     v36_3(void)           = Call[ExRaiseAccessViolation]            : func:r36_1, 0:r36_2
 #   36|     m36_4(unknown)        = ^CallSideEffect                         : ~m32_4
 #   36|     m36_5(unknown)        = Chi                                     : total:m32_4, partial:m36_4
-#-----|   Exception -> Block 3
+#-----|   Exception -> Block 4
 
-#   39|   Block 1
+#   32|   Block 1
+#   32|     v32_5(void) = Unwind       : 
+#   32|     v32_6(void) = AliasedUse   : ~m40_5
+#   32|     v32_7(void) = ExitFunction : 
+
+#   39|   Block 2
 #   39|     r39_1(int)  = Constant[0]       : 
-#   39|     r39_2(bool) = CompareEQ         : r38_2, r39_1
+#   39|     r39_2(bool) = CompareEQ         : r38_1, r39_1
 #   39|     v39_3(void) = ConditionalBranch : r39_2
-#-----|   False (back edge) -> Block 2
-#-----|   True -> Block 5
+#-----|   False -> Block 3
+#-----|   True -> Block 6
 
-#   39|   Block 2
+#   39|   Block 3
 #   39|     r39_4(int)  = Constant[1]       : 
-#   39|     r39_5(bool) = CompareEQ         : r38_2, r39_4
+#   39|     r39_5(bool) = CompareEQ         : r38_1, r39_4
 #   39|     v39_6(void) = ConditionalBranch : r39_5
-#-----|   False -> Block 5
-#-----|   True (back edge) -> Block 4
-
-#   38|   Block 3
-#   38|     m38_1(unknown) = Phi               : from 0:~m36_5, from 4:~m40_5
-#   38|     r38_2(int)     = Constant[1]       : 
-#   39|     r39_7(int)     = Constant[-1]      : 
-#   39|     r39_8(bool)    = CompareEQ         : r38_2, r39_7
-#   39|     v39_9(void)    = ConditionalBranch : r39_8
-#-----|   False (back edge) -> Block 1
+#-----|   False -> Block 6
 #-----|   True -> Block 5
 
-#   40|   Block 4
+#   38|   Block 4
+#   38|     r38_1(int)  = Constant[1]       : 
+#   39|     r39_7(int)  = Constant[-1]      : 
+#   39|     r39_8(bool) = CompareEQ         : r38_1, r39_7
+#   39|     v39_9(void) = ConditionalBranch : r39_8
+#-----|   False -> Block 2
+#-----|   True -> Block 6
+
+#   40|   Block 5
 #   40|     r40_1(glval<unknown>) = FunctionAddress[ExRaiseAccessViolation] : 
 #   40|     r40_2(int)            = Constant[1]                             : 
 #   40|     v40_3(void)           = Call[ExRaiseAccessViolation]            : func:r40_1, 0:r40_2
-#   40|     m40_4(unknown)        = ^CallSideEffect                         : ~m38_1
-#   40|     m40_5(unknown)        = Chi                                     : total:m38_1, partial:m40_4
-#-----|   Exception (back edge) -> Block 3
+#   40|     m40_4(unknown)        = ^CallSideEffect                         : ~m36_5
+#   40|     m40_5(unknown)        = Chi                                     : total:m36_5, partial:m40_4
+#-----|   Exception -> Block 1
 
-#   32|   Block 5
-#   32|     v32_5(void) = Unreached : 
+#   32|   Block 6
+#   32|     v32_8(void) = Unreached : 
 
 #   44| void try_with_finally()
 #   44|   Block 0
@@ -3261,6 +3265,12 @@ ir.c:
 #   81|     v81_3(void)           = Call[ExRaiseAccessViolation]            : func:r81_1, 0:r81_2
 #   81|     m81_4(unknown)        = ^CallSideEffect                         : ~m80_4
 #   81|     m81_5(unknown)        = Chi                                     : total:m80_4, partial:m81_4
+#-----|   Exception -> Block 1
+
+#   80|   Block 1
+#   80|     v80_5(void) = Unwind       : 
+#   80|     v80_6(void) = AliasedUse   : ~m81_5
+#   80|     v80_7(void) = ExitFunction : 
 
 ir.cpp:
 #    1| void Constants()

cpp/ql/test/library-tests/ir/ir/aliased_ssa_consistency.expected

@@ -8,25 +8,8 @@ sideEffectWithoutPrimary
 instructionWithoutSuccessor
 | ir.c:62:5:62:26 | Chi: call to ExRaiseAccessViolation | Instruction 'Chi: call to ExRaiseAccessViolation' has no successors in function '$@'. | ir.c:57:6:57:30 | void throw_in_try_with_finally() | void throw_in_try_with_finally() |
 | ir.c:73:5:73:26 | Chi: call to ExRaiseAccessViolation | Instruction 'Chi: call to ExRaiseAccessViolation' has no successors in function '$@'. | ir.c:70:6:70:39 | void throw_in_try_with_throw_in_finally() | void throw_in_try_with_throw_in_finally() |
-| ir.c:81:3:81:24 | Chi: call to ExRaiseAccessViolation | Instruction 'Chi: call to ExRaiseAccessViolation' has no successors in function '$@'. | ir.c:80:6:80:27 | void raise_access_violation() | void raise_access_violation() |
 ambiguousSuccessors
 unexplainedLoop
-| ir.c:38:13:38:37 | Constant: 1 | Instruction 'Constant: 1' is part of an unexplained loop in function '$@'. | ir.c:32:6:32:32 | void unexplained_loop_regression() | void unexplained_loop_regression() |
-| ir.c:38:13:38:37 | Phi: 1 | Instruction 'Phi: 1' is part of an unexplained loop in function '$@'. | ir.c:32:6:32:32 | void unexplained_loop_regression() | void unexplained_loop_regression() |
-| ir.c:39:3:41:3 | CompareEQ: { ... } | Instruction 'CompareEQ: { ... }' is part of an unexplained loop in function '$@'. | ir.c:32:6:32:32 | void unexplained_loop_regression() | void unexplained_loop_regression() |
-| ir.c:39:3:41:3 | CompareEQ: { ... } | Instruction 'CompareEQ: { ... }' is part of an unexplained loop in function '$@'. | ir.c:32:6:32:32 | void unexplained_loop_regression() | void unexplained_loop_regression() |
-| ir.c:39:3:41:3 | CompareEQ: { ... } | Instruction 'CompareEQ: { ... }' is part of an unexplained loop in function '$@'. | ir.c:32:6:32:32 | void unexplained_loop_regression() | void unexplained_loop_regression() |
-| ir.c:39:3:41:3 | ConditionalBranch: { ... } | Instruction 'ConditionalBranch: { ... }' is part of an unexplained loop in function '$@'. | ir.c:32:6:32:32 | void unexplained_loop_regression() | void unexplained_loop_regression() |
-| ir.c:39:3:41:3 | ConditionalBranch: { ... } | Instruction 'ConditionalBranch: { ... }' is part of an unexplained loop in function '$@'. | ir.c:32:6:32:32 | void unexplained_loop_regression() | void unexplained_loop_regression() |
-| ir.c:39:3:41:3 | ConditionalBranch: { ... } | Instruction 'ConditionalBranch: { ... }' is part of an unexplained loop in function '$@'. | ir.c:32:6:32:32 | void unexplained_loop_regression() | void unexplained_loop_regression() |
-| ir.c:39:3:41:3 | Constant: { ... } | Instruction 'Constant: { ... }' is part of an unexplained loop in function '$@'. | ir.c:32:6:32:32 | void unexplained_loop_regression() | void unexplained_loop_regression() |
-| ir.c:39:3:41:3 | Constant: { ... } | Instruction 'Constant: { ... }' is part of an unexplained loop in function '$@'. | ir.c:32:6:32:32 | void unexplained_loop_regression() | void unexplained_loop_regression() |
-| ir.c:39:3:41:3 | Constant: { ... } | Instruction 'Constant: { ... }' is part of an unexplained loop in function '$@'. | ir.c:32:6:32:32 | void unexplained_loop_regression() | void unexplained_loop_regression() |
-| ir.c:40:5:40:26 | Call: call to ExRaiseAccessViolation | Instruction 'Call: call to ExRaiseAccessViolation' is part of an unexplained loop in function '$@'. | ir.c:32:6:32:32 | void unexplained_loop_regression() | void unexplained_loop_regression() |
-| ir.c:40:5:40:26 | CallSideEffect: call to ExRaiseAccessViolation | Instruction 'CallSideEffect: call to ExRaiseAccessViolation' is part of an unexplained loop in function '$@'. | ir.c:32:6:32:32 | void unexplained_loop_regression() | void unexplained_loop_regression() |
-| ir.c:40:5:40:26 | Chi: call to ExRaiseAccessViolation | Instruction 'Chi: call to ExRaiseAccessViolation' is part of an unexplained loop in function '$@'. | ir.c:32:6:32:32 | void unexplained_loop_regression() | void unexplained_loop_regression() |
-| ir.c:40:5:40:26 | FunctionAddress: call to ExRaiseAccessViolation | Instruction 'FunctionAddress: call to ExRaiseAccessViolation' is part of an unexplained loop in function '$@'. | ir.c:32:6:32:32 | void unexplained_loop_regression() | void unexplained_loop_regression() |
-| ir.c:40:28:40:28 | Constant: 1 | Instruction 'Constant: 1' is part of an unexplained loop in function '$@'. | ir.c:32:6:32:32 | void unexplained_loop_regression() | void unexplained_loop_regression() |
 unnecessaryPhiInstruction
 memoryOperandDefinitionIsUnmodeled
 operandAcrossFunctions
@@ -37,9 +20,6 @@ containsLoopOfForwardEdges
 missingIRType
 multipleIRTypes
 lostReachability
-| ir.c:39:3:41:3 | Constant: { ... } | Block 'Constant: { ... }' is not reachable by traversing only forward edges in function '$@'. | ir.c:32:6:32:32 | void unexplained_loop_regression() | void unexplained_loop_regression() |
-| ir.c:39:3:41:3 | Constant: { ... } | Block 'Constant: { ... }' is not reachable by traversing only forward edges in function '$@'. | ir.c:32:6:32:32 | void unexplained_loop_regression() | void unexplained_loop_regression() |
-| ir.c:40:5:40:26 | FunctionAddress: call to ExRaiseAccessViolation | Block 'FunctionAddress: call to ExRaiseAccessViolation' is not reachable by traversing only forward edges in function '$@'. | ir.c:32:6:32:32 | void unexplained_loop_regression() | void unexplained_loop_regression() |
 backEdgeCountMismatch
 useNotDominatedByDefinition
 switchInstructionWithoutDefaultEdge