Merge pull request #17430 from jketema/fix-finally-inconsistency

C++: Fix `__finally` related inconsistencies

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

@@ -767,7 +767,10 @@ newtype TTranslatedElement =
   } or
   // A statement
   TTranslatedStmt(Stmt stmt) { translateStmt(stmt) } or
+  // The `__except` block of a `__try __except` statement
   TTranslatedMicrosoftTryExceptHandler(MicrosoftTryExceptStmt stmt) or
+  // The `__finally` block of a `__try __finally` statement
+  TTranslatedMicrosoftTryFinallyHandler(MicrosoftTryFinallyStmt stmt) or
   // A function
   TTranslatedFunction(Function func) { translateFunction(func) } or
   // A constructor init list

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

@@ -233,6 +233,62 @@ class TranslatedMicrosoftTryExceptHandler extends TranslatedElement,
   }
 }
 
+TranslatedMicrosoftTryFinallyHandler getTranslatedMicrosoftTryFinallyHandler(
+  MicrosoftTryFinallyStmt tryFinally
+) {
+  result.getAst() = tryFinally.getFinally()
+}
+
+class TranslatedMicrosoftTryFinallyHandler extends TranslatedElement,
+  TTranslatedMicrosoftTryFinallyHandler
+{
+  MicrosoftTryFinallyStmt tryFinally;
+
+  TranslatedMicrosoftTryFinallyHandler() {
+    this = TTranslatedMicrosoftTryFinallyHandler(tryFinally)
+  }
+
+  final override string toString() { result = tryFinally.toString() }
+
+  final override Locatable getAst() { result = tryFinally.getFinally() }
+
+  override Instruction getFirstInstruction(EdgeKind kind) {
+    result = this.getTranslatedFinally().getFirstInstruction(kind)
+  }
+
+  override Instruction getALastInstructionInternal() {
+    result = this.getTranslatedFinally().getALastInstruction()
+  }
+
+  override Instruction getChildSuccessorInternal(TranslatedElement child, EdgeKind kind) {
+    child = this.getTranslatedFinally() and
+    result = this.getParent().getChildSuccessor(this, kind)
+  }
+
+  override Instruction getInstructionSuccessorInternal(InstructionTag tag, EdgeKind kind) { none() }
+
+  override TranslatedElement getChild(int id) {
+    id = 0 and
+    result = this.getTranslatedFinally()
+  }
+
+  override predicate hasInstruction(Opcode opcode, InstructionTag tag, CppType resultType) {
+    none()
+  }
+
+  final override Function getFunction() { result = tryFinally.getEnclosingFunction() }
+
+  private TranslatedStmt getTranslatedFinally() {
+    result = getTranslatedStmt(tryFinally.getFinally())
+  }
+
+  override Instruction getExceptionSuccessorInstruction(EdgeKind kind) {
+    // A throw from within a `__finally` block flows to the handler for the parent of
+    // the `__try`.
+    result = this.getParent().getParent().getExceptionSuccessorInstruction(kind)
+  }
+}
+
 abstract class TranslatedStmt extends TranslatedElement, TTranslatedStmt {
   Stmt stmt;
 
@@ -611,7 +667,9 @@ class TryOrMicrosoftTryStmt extends Stmt {
   }
 
   /** Gets the `finally` statement (usually a BlockStmt), if any. */
-  Stmt getFinally() { result = this.(MicrosoftTryFinallyStmt).getFinally() }
+  TranslatedElement getTranslatedFinally() {
+    result = getTranslatedMicrosoftTryFinallyHandler(this)
+  }
 }
 
 /**
@@ -681,11 +739,14 @@ class TranslatedTryStmt extends TranslatedStmt {
 
   final override Instruction getExceptionSuccessorInstruction(EdgeKind kind) {
     result = this.getHandler(0).getFirstInstruction(kind)
+    or
+    not exists(this.getHandler(_)) and
+    result = this.getFinally().getFirstInstruction(kind)
   }
 
   private TranslatedElement getHandler(int index) { result = stmt.getTranslatedHandler(index) }
 
-  private TranslatedStmt getFinally() { result = getTranslatedStmt(stmt.getFinally()) }
+  private TranslatedElement getFinally() { result = stmt.getTranslatedFinally() }
 
   private TranslatedStmt getBody() { result = getTranslatedStmt(stmt.getStmt()) }
 }

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

@@ -3241,6 +3241,16 @@ ir.c:
 #   62|     v62_3(void)           = Call[ExRaiseAccessViolation]            : func:r62_1, 0:r62_2
 #   62|     m62_4(unknown)        = ^CallSideEffect                         : ~m57_4
 #   62|     m62_5(unknown)        = Chi                                     : total:m57_4, partial:m62_4
+#-----|   Exception -> Block 1
+
+#   66|   Block 1
+#   66|     r66_1(int)        = Constant[1]        : 
+#   66|     r66_2(glval<int>) = VariableAddress[x] : 
+#   66|     m66_3(int)        = Store[x]           : &:r66_2, r66_1
+#   68|     v68_1(void)       = NoOp               : 
+#   57|     v57_5(void)       = ReturnVoid         : 
+#   57|     v57_6(void)       = AliasedUse         : ~m62_5
+#   57|     v57_7(void)       = ExitFunction       : 
 
 #   70| void throw_in_try_with_throw_in_finally()
 #   70|   Block 0
@@ -3253,6 +3263,20 @@ ir.c:
 #   73|     v73_3(void)           = Call[ExRaiseAccessViolation]            : func:r73_1, 0:r73_2
 #   73|     m73_4(unknown)        = ^CallSideEffect                         : ~m70_4
 #   73|     m73_5(unknown)        = Chi                                     : total:m70_4, partial:m73_4
+#-----|   Exception -> Block 2
+
+#   70|   Block 1
+#   70|     v70_5(void) = Unwind       : 
+#   70|     v70_6(void) = AliasedUse   : ~m76_5
+#   70|     v70_7(void) = ExitFunction : 
+
+#   76|   Block 2
+#   76|     r76_1(glval<unknown>) = FunctionAddress[ExRaiseAccessViolation] : 
+#   76|     r76_2(int)            = Constant[0]                             : 
+#   76|     v76_3(void)           = Call[ExRaiseAccessViolation]            : func:r76_1, 0:r76_2
+#   76|     m76_4(unknown)        = ^CallSideEffect                         : ~m73_5
+#   76|     m76_5(unknown)        = Chi                                     : total:m73_5, partial:m76_4
+#-----|   Exception -> Block 1
 
 #   80| void raise_access_violation()
 #   80|   Block 0

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

@@ -6,8 +6,6 @@ missingOperandType
 duplicateChiOperand
 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() |
 ambiguousSuccessors
 unexplainedLoop
 unnecessaryPhiInstruction

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

@@ -6,8 +6,6 @@ missingOperandType
 duplicateChiOperand
 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() |
 ambiguousSuccessors
 unexplainedLoop
 unnecessaryPhiInstruction

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

@@ -6,9 +6,6 @@ missingOperandType
 duplicateChiOperand
 sideEffectWithoutPrimary
 instructionWithoutSuccessor
-| ir.c:62:5:62:26 | CallSideEffect: call to ExRaiseAccessViolation | Instruction 'CallSideEffect: 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 | CallSideEffect: call to ExRaiseAccessViolation | Instruction 'CallSideEffect: 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:76:5:76:26 | CallSideEffect: call to ExRaiseAccessViolation | Instruction 'CallSideEffect: 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() |
 ambiguousSuccessors
 unexplainedLoop
 unnecessaryPhiInstruction

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

@@ -3022,6 +3022,7 @@ ir.c:
 #   62|     r62_2(int)            = Constant[0]                             : 
 #   62|     v62_3(void)           = Call[ExRaiseAccessViolation]            : func:r62_1, 0:r62_2
 #   62|     mu62_4(unknown)       = ^CallSideEffect                         : ~m?
+#-----|   Exception -> Block 3
 
 #   57|   Block 1
 #   57|     v57_4(void) = AliasedUse   : ~m?
@@ -3048,6 +3049,7 @@ ir.c:
 #   73|     r73_2(int)            = Constant[0]                             : 
 #   73|     v73_3(void)           = Call[ExRaiseAccessViolation]            : func:r73_1, 0:r73_2
 #   73|     mu73_4(unknown)       = ^CallSideEffect                         : ~m?
+#-----|   Exception -> Block 3
 
 #   70|   Block 1
 #   70|     v70_4(void) = AliasedUse   : ~m?
@@ -3062,6 +3064,7 @@ ir.c:
 #   76|     r76_2(int)            = Constant[0]                             : 
 #   76|     v76_3(void)           = Call[ExRaiseAccessViolation]            : func:r76_1, 0:r76_2
 #   76|     mu76_4(unknown)       = ^CallSideEffect                         : ~m?
+#-----|   Exception -> Block 2
 
 #   78|   Block 4
 #   78|     v78_1(void) = NoOp       : 

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

@@ -6,8 +6,6 @@ missingOperandType
 duplicateChiOperand
 sideEffectWithoutPrimary
 instructionWithoutSuccessor
-| ir.c:62:5:62:26 | CallSideEffect: call to ExRaiseAccessViolation | Instruction 'CallSideEffect: 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 | CallSideEffect: call to ExRaiseAccessViolation | Instruction 'CallSideEffect: 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() |
 ambiguousSuccessors
 unexplainedLoop
 unnecessaryPhiInstruction

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

@@ -6,8 +6,6 @@ missingOperandType
 duplicateChiOperand
 sideEffectWithoutPrimary
 instructionWithoutSuccessor
-| ir.c:62:5:62:26 | CallSideEffect: call to ExRaiseAccessViolation | Instruction 'CallSideEffect: 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 | CallSideEffect: call to ExRaiseAccessViolation | Instruction 'CallSideEffect: 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() |
 ambiguousSuccessors
 unexplainedLoop
 unnecessaryPhiInstruction

cpp/ql/test/library-tests/syntax-zoo/aliased_ssa_consistency.expected

@@ -7,8 +7,6 @@ missingOperandType
 duplicateChiOperand
 sideEffectWithoutPrimary
 instructionWithoutSuccessor
-| ms_try_mix.cpp:38:5:38:5 | Chi: c106 | Instruction 'Chi: c106' has no successors in function '$@'. | ms_try_mix.cpp:29:6:29:19 | void ms_finally_mix(int) | void ms_finally_mix(int) |
-| ms_try_mix.cpp:53:5:53:11 | ThrowValue: throw ... | Instruction 'ThrowValue: throw ...' has no successors in function '$@'. | ms_try_mix.cpp:49:6:49:28 | void ms_empty_finally_at_end() | void ms_empty_finally_at_end() |
 | stmt_expr.cpp:27:5:27:15 | Store: ... = ... | Instruction 'Store: ... = ...' has no successors in function '$@'. | stmt_expr.cpp:21:13:21:13 | void stmtexpr::g(int) | void stmtexpr::g(int) |
 ambiguousSuccessors
 unexplainedLoop

cpp/ql/test/library-tests/syntax-zoo/raw_consistency.expected

@@ -8,8 +8,6 @@ missingOperandType
 duplicateChiOperand
 sideEffectWithoutPrimary
 instructionWithoutSuccessor
-| ms_try_mix.cpp:38:5:38:5 | IndirectMayWriteSideEffect: c106 | Instruction 'IndirectMayWriteSideEffect: c106' has no successors in function '$@'. | ms_try_mix.cpp:29:6:29:19 | void ms_finally_mix(int) | void ms_finally_mix(int) |
-| ms_try_mix.cpp:53:5:53:11 | ThrowValue: throw ... | Instruction 'ThrowValue: throw ...' has no successors in function '$@'. | ms_try_mix.cpp:49:6:49:28 | void ms_empty_finally_at_end() | void ms_empty_finally_at_end() |
 | stmt_expr.cpp:27:5:27:15 | Store: ... = ... | Instruction 'Store: ... = ...' has no successors in function '$@'. | stmt_expr.cpp:21:13:21:13 | void stmtexpr::g(int) | void stmtexpr::g(int) |
 | stmt_expr.cpp:29:11:32:11 | CopyValue: (statement expression) | Instruction 'CopyValue: (statement expression)' has no successors in function '$@'. | stmt_expr.cpp:21:13:21:13 | void stmtexpr::g(int) | void stmtexpr::g(int) |
 | stmt_in_type.cpp:5:53:5:53 | Constant: 1 | Instruction 'Constant: 1' has no successors in function '$@'. | stmt_in_type.cpp:2:6:2:12 | void cpp_fun() | void cpp_fun() |

cpp/ql/test/library-tests/syntax-zoo/unaliased_ssa_consistency.expected

@@ -7,8 +7,6 @@ missingOperandType
 duplicateChiOperand
 sideEffectWithoutPrimary
 instructionWithoutSuccessor
-| ms_try_mix.cpp:38:5:38:5 | IndirectMayWriteSideEffect: c106 | Instruction 'IndirectMayWriteSideEffect: c106' has no successors in function '$@'. | ms_try_mix.cpp:29:6:29:19 | void ms_finally_mix(int) | void ms_finally_mix(int) |
-| ms_try_mix.cpp:53:5:53:11 | ThrowValue: throw ... | Instruction 'ThrowValue: throw ...' has no successors in function '$@'. | ms_try_mix.cpp:49:6:49:28 | void ms_empty_finally_at_end() | void ms_empty_finally_at_end() |
 | stmt_expr.cpp:27:5:27:15 | Store: ... = ... | Instruction 'Store: ... = ...' has no successors in function '$@'. | stmt_expr.cpp:21:13:21:13 | void stmtexpr::g(int) | void stmtexpr::g(int) |
 ambiguousSuccessors
 unexplainedLoop