C++: Implement Instruction.isResultConflated

jbj · jbj · commit f6f9afe462da · 2020-03-20T17:57:18.000+01:00
This predicate replaces `isChiForAllAliasedMemory`, which was always
intended to be temporary. A test is added to `IRSanity.qll` to verify
that the new predicate corresponds exactly with (a fixed version of) the
old one.

The implementation of the new predicate,
`Cached::hasConflatedMemoryResult` in `SSAConstruction.qll`, is faster
to compute than the old `isChiForAllAliasedMemory` because it uses
information that's readily available during SSA construction.
diff --git a/cpp/ql/src/semmle/code/cpp/ir/dataflow/DefaultTaintTracking.qll b/cpp/ql/src/semmle/code/cpp/ir/dataflow/DefaultTaintTracking.qll
@@ -183,7 +183,7 @@ private predicate instructionTaintStep(Instruction i1, Instruction i2) {
   i2.(UnaryInstruction).getUnary() = i1
   or
   i2.(ChiInstruction).getPartial() = i1 and
-  not isChiForAllAliasedMemory(i2)
+  not i2.isResultConflated()
   or
   exists(BinaryInstruction bin |
     bin = i2 and
@@ -276,19 +276,6 @@ private predicate modelTaintToParameter(Function f, int parameterIn, int paramet
   )
 }
 
-/**
- * Holds if `chi` is on the chain of chi-instructions for all aliased memory.
- * Taint should not pass through these instructions since they tend to mix up
- * unrelated objects.
- */
-private predicate isChiForAllAliasedMemory(Instruction instr) {
-  instr.(ChiInstruction).getTotal() instanceof AliasedDefinitionInstruction
-  or
-  isChiForAllAliasedMemory(instr.(ChiInstruction).getTotal())
-  or
-  isChiForAllAliasedMemory(instr.(PhiInstruction).getAnInputOperand().getAnyDef())
-}
-
 private predicate modelTaintToReturnValue(Function f, int parameterIn) {
   // Taint flow from parameter to return value
   exists(FunctionInput modelIn, FunctionOutput modelOut |
diff --git a/cpp/ql/src/semmle/code/cpp/ir/implementation/aliased_ssa/IRSanity.qll b/cpp/ql/src/semmle/code/cpp/ir/implementation/aliased_ssa/IRSanity.qll
@@ -272,4 +272,34 @@ module InstructionSanity {
     func = switchInstr.getEnclosingIRFunction() and
     funcText = Language::getIdentityString(func.getFunction())
   }
+
+  /**
+   * Holds if `instr` is on the chain of chi/phi instructions for all aliased
+   * memory.
+   */
+  private predicate isOnAliasedDefinitionChain(Instruction instr) {
+    instr instanceof AliasedDefinitionInstruction
+    or
+    instr.getOpcode() instanceof Opcode::InitializeNonLocal
+    or
+    isOnAliasedDefinitionChain(instr.(ChiInstruction).getTotal())
+    or
+    isOnAliasedDefinitionChain(instr.(PhiInstruction).getAnInputOperand().getAnyDef())
+  }
+
+  private predicate shouldBeConflated(Instruction instr) {
+    isOnAliasedDefinitionChain(instr)
+    or
+    instr instanceof UnmodeledDefinitionInstruction
+  }
+
+  query predicate notMarkedAsConflated(Instruction instr) {
+    shouldBeConflated(instr) and
+    not instr.isResultConflated()
+  }
+
+  query predicate wronglyMarkedAsConflated(Instruction instr) {
+    instr.isResultConflated() and
+    not shouldBeConflated(instr)
+  }
 }
diff --git a/cpp/ql/src/semmle/code/cpp/ir/implementation/aliased_ssa/Instruction.qll b/cpp/ql/src/semmle/code/cpp/ir/implementation/aliased_ssa/Instruction.qll
@@ -321,6 +321,17 @@ class Instruction extends Construction::TInstruction {
     Construction::hasModeledMemoryResult(this)
   }
 
+  /**
+   * Holds if this is an instruction with a memory result that represents a
+   * conflation of more than one object.
+   *
+   * This happens in practice when dereferencing a pointer that cannot be
+   * tracked back to a single local allocation. Such memory is instead modeled
+   * as originating on the `AliasedDefinitionInstruction` at the entry of the
+   * function.
+   */
+  final predicate isResultConflated() { Construction::hasConflatedMemoryResult(this) }
+
   /**
    * Gets the successor of this instruction along the control flow edge
    * specified by `kind`.
diff --git a/cpp/ql/src/semmle/code/cpp/ir/implementation/aliased_ssa/internal/AliasedSSA.qll b/cpp/ql/src/semmle/code/cpp/ir/implementation/aliased_ssa/internal/AliasedSSA.qll
@@ -354,6 +354,7 @@ class AllAliasedMemory extends TAllAliasedMemory, MemoryLocation {
   final override predicate isMayAccess() { isMayAccess = true }
 }
 
+/** A virtual variable that groups all escaped memory within a function. */
 class AliasedVirtualVariable extends AllAliasedMemory, VirtualVariable {
   AliasedVirtualVariable() { not isMayAccess() }
 }
diff --git a/cpp/ql/src/semmle/code/cpp/ir/implementation/aliased_ssa/internal/SSAConstruction.qll b/cpp/ql/src/semmle/code/cpp/ir/implementation/aliased_ssa/internal/SSAConstruction.qll
@@ -65,6 +65,30 @@ private module Cached {
     instruction instanceof ChiInstruction // Chis always have modeled results
   }
 
+  cached
+  predicate hasConflatedMemoryResult(Instruction instruction) {
+    instruction instanceof UnmodeledDefinitionInstruction
+    or
+    instruction instanceof AliasedDefinitionInstruction
+    or
+    instruction.getOpcode() instanceof Opcode::InitializeNonLocal
+    or
+    exists(OldInstruction oldInstruction | instruction = Chi(oldInstruction) |
+      Alias::getResultMemoryLocation(oldInstruction).getVirtualVariable() instanceof
+        Alias::AliasedVirtualVariable
+      or
+      // If there is no memory location for a memory result, then it's unmodeled
+      // and therefore conflated with every other unmodeled instruction.
+      oldInstruction.hasMemoryResult() and
+      not exists(Alias::getResultMemoryLocation(oldInstruction))
+    )
+    or
+    exists(Alias::MemoryLocation location |
+      instruction = Phi(_, location) and
+      location.getVirtualVariable() instanceof Alias::AliasedVirtualVariable
+    )
+  }
+
   cached
   Instruction getRegisterOperandDefinition(Instruction instruction, RegisterOperandTag tag) {
     exists(OldInstruction oldInstruction, OldIR::RegisterOperand oldOperand |
diff --git a/cpp/ql/src/semmle/code/cpp/ir/implementation/raw/IRSanity.qll b/cpp/ql/src/semmle/code/cpp/ir/implementation/raw/IRSanity.qll
@@ -272,4 +272,34 @@ module InstructionSanity {
     func = switchInstr.getEnclosingIRFunction() and
     funcText = Language::getIdentityString(func.getFunction())
   }
+
+  /**
+   * Holds if `instr` is on the chain of chi/phi instructions for all aliased
+   * memory.
+   */
+  private predicate isOnAliasedDefinitionChain(Instruction instr) {
+    instr instanceof AliasedDefinitionInstruction
+    or
+    instr.getOpcode() instanceof Opcode::InitializeNonLocal
+    or
+    isOnAliasedDefinitionChain(instr.(ChiInstruction).getTotal())
+    or
+    isOnAliasedDefinitionChain(instr.(PhiInstruction).getAnInputOperand().getAnyDef())
+  }
+
+  private predicate shouldBeConflated(Instruction instr) {
+    isOnAliasedDefinitionChain(instr)
+    or
+    instr instanceof UnmodeledDefinitionInstruction
+  }
+
+  query predicate notMarkedAsConflated(Instruction instr) {
+    shouldBeConflated(instr) and
+    not instr.isResultConflated()
+  }
+
+  query predicate wronglyMarkedAsConflated(Instruction instr) {
+    instr.isResultConflated() and
+    not shouldBeConflated(instr)
+  }
 }
diff --git a/cpp/ql/src/semmle/code/cpp/ir/implementation/raw/Instruction.qll b/cpp/ql/src/semmle/code/cpp/ir/implementation/raw/Instruction.qll
@@ -321,6 +321,17 @@ class Instruction extends Construction::TInstruction {
     Construction::hasModeledMemoryResult(this)
   }
 
+  /**
+   * Holds if this is an instruction with a memory result that represents a
+   * conflation of more than one object.
+   *
+   * This happens in practice when dereferencing a pointer that cannot be
+   * tracked back to a single local allocation. Such memory is instead modeled
+   * as originating on the `AliasedDefinitionInstruction` at the entry of the
+   * function.
+   */
+  final predicate isResultConflated() { Construction::hasConflatedMemoryResult(this) }
+
   /**
    * Gets the successor of this instruction along the control flow edge
    * specified by `kind`.
diff --git a/cpp/ql/src/semmle/code/cpp/ir/implementation/raw/internal/IRConstruction.qll b/cpp/ql/src/semmle/code/cpp/ir/implementation/raw/internal/IRConstruction.qll
@@ -61,6 +61,15 @@ private module Cached {
   cached
   predicate hasModeledMemoryResult(Instruction instruction) { none() }
 
+  cached
+  predicate hasConflatedMemoryResult(Instruction instruction) {
+    instruction instanceof UnmodeledDefinitionInstruction
+    or
+    instruction instanceof AliasedDefinitionInstruction
+    or
+    instruction.getOpcode() instanceof Opcode::InitializeNonLocal
+  }
+
   cached
   Expr getInstructionConvertedResultExpression(Instruction instruction) {
     exists(TranslatedExpr translatedExpr |
diff --git a/cpp/ql/src/semmle/code/cpp/ir/implementation/unaliased_ssa/IRSanity.qll b/cpp/ql/src/semmle/code/cpp/ir/implementation/unaliased_ssa/IRSanity.qll
@@ -272,4 +272,34 @@ module InstructionSanity {
     func = switchInstr.getEnclosingIRFunction() and
     funcText = Language::getIdentityString(func.getFunction())
   }
+
+  /**
+   * Holds if `instr` is on the chain of chi/phi instructions for all aliased
+   * memory.
+   */
+  private predicate isOnAliasedDefinitionChain(Instruction instr) {
+    instr instanceof AliasedDefinitionInstruction
+    or
+    instr.getOpcode() instanceof Opcode::InitializeNonLocal
+    or
+    isOnAliasedDefinitionChain(instr.(ChiInstruction).getTotal())
+    or
+    isOnAliasedDefinitionChain(instr.(PhiInstruction).getAnInputOperand().getAnyDef())
+  }
+
+  private predicate shouldBeConflated(Instruction instr) {
+    isOnAliasedDefinitionChain(instr)
+    or
+    instr instanceof UnmodeledDefinitionInstruction
+  }
+
+  query predicate notMarkedAsConflated(Instruction instr) {
+    shouldBeConflated(instr) and
+    not instr.isResultConflated()
+  }
+
+  query predicate wronglyMarkedAsConflated(Instruction instr) {
+    instr.isResultConflated() and
+    not shouldBeConflated(instr)
+  }
 }
diff --git a/cpp/ql/src/semmle/code/cpp/ir/implementation/unaliased_ssa/Instruction.qll b/cpp/ql/src/semmle/code/cpp/ir/implementation/unaliased_ssa/Instruction.qll
@@ -321,6 +321,17 @@ class Instruction extends Construction::TInstruction {
     Construction::hasModeledMemoryResult(this)
   }
 
+  /**
+   * Holds if this is an instruction with a memory result that represents a
+   * conflation of more than one object.
+   *
+   * This happens in practice when dereferencing a pointer that cannot be
+   * tracked back to a single local allocation. Such memory is instead modeled
+   * as originating on the `AliasedDefinitionInstruction` at the entry of the
+   * function.
+   */
+  final predicate isResultConflated() { Construction::hasConflatedMemoryResult(this) }
+
   /**
    * Gets the successor of this instruction along the control flow edge
    * specified by `kind`.
diff --git a/cpp/ql/src/semmle/code/cpp/ir/implementation/unaliased_ssa/internal/SSAConstruction.qll b/cpp/ql/src/semmle/code/cpp/ir/implementation/unaliased_ssa/internal/SSAConstruction.qll
@@ -65,6 +65,30 @@ private module Cached {
     instruction instanceof ChiInstruction // Chis always have modeled results
   }
 
+  cached
+  predicate hasConflatedMemoryResult(Instruction instruction) {
+    instruction instanceof UnmodeledDefinitionInstruction
+    or
+    instruction instanceof AliasedDefinitionInstruction
+    or
+    instruction.getOpcode() instanceof Opcode::InitializeNonLocal
+    or
+    exists(OldInstruction oldInstruction | instruction = Chi(oldInstruction) |
+      Alias::getResultMemoryLocation(oldInstruction).getVirtualVariable() instanceof
+        Alias::AliasedVirtualVariable
+      or
+      // If there is no memory location for a memory result, then it's unmodeled
+      // and therefore conflated with every other unmodeled instruction.
+      oldInstruction.hasMemoryResult() and
+      not exists(Alias::getResultMemoryLocation(oldInstruction))
+    )
+    or
+    exists(Alias::MemoryLocation location |
+      instruction = Phi(_, location) and
+      location.getVirtualVariable() instanceof Alias::AliasedVirtualVariable
+    )
+  }
+
   cached
   Instruction getRegisterOperandDefinition(Instruction instruction, RegisterOperandTag tag) {
     exists(OldInstruction oldInstruction, OldIR::RegisterOperand oldOperand |
diff --git a/cpp/ql/src/semmle/code/cpp/ir/implementation/unaliased_ssa/internal/SimpleSSA.qll b/cpp/ql/src/semmle/code/cpp/ir/implementation/unaliased_ssa/internal/SimpleSSA.qll
@@ -61,6 +61,11 @@ class MemoryLocation extends TMemoryLocation {
 
 class VirtualVariable extends MemoryLocation { }
 
+/** A virtual variable that groups all escaped memory within a function. */
+class AliasedVirtualVariable extends VirtualVariable {
+  AliasedVirtualVariable() { none() }
+}
+
 Overlap getOverlap(MemoryLocation def, MemoryLocation use) {
   def = use and result instanceof MustExactlyOverlap
   or
diff --git a/cpp/ql/test/library-tests/ir/ir/aliased_ssa_sanity_unsound.expected b/cpp/ql/test/library-tests/ir/ir/aliased_ssa_sanity_unsound.expected
@@ -20,6 +20,8 @@ lostReachability
 backEdgeCountMismatch
 useNotDominatedByDefinition
 switchInstructionWithoutDefaultEdge
+notMarkedAsConflated
+wronglyMarkedAsConflated
 missingCanonicalLanguageType
 multipleCanonicalLanguageTypes
 missingIRType
diff --git a/cpp/ql/test/library-tests/ir/ir/raw_sanity.expected b/cpp/ql/test/library-tests/ir/ir/raw_sanity.expected
@@ -20,6 +20,8 @@ lostReachability
 backEdgeCountMismatch
 useNotDominatedByDefinition
 switchInstructionWithoutDefaultEdge
+notMarkedAsConflated
+wronglyMarkedAsConflated
 missingCanonicalLanguageType
 multipleCanonicalLanguageTypes
 missingIRType
diff --git a/cpp/ql/test/library-tests/ir/ir/unaliased_ssa_sanity_unsound.expected b/cpp/ql/test/library-tests/ir/ir/unaliased_ssa_sanity_unsound.expected
@@ -20,6 +20,8 @@ lostReachability
 backEdgeCountMismatch
 useNotDominatedByDefinition
 switchInstructionWithoutDefaultEdge
+notMarkedAsConflated
+wronglyMarkedAsConflated
 missingCanonicalLanguageType
 multipleCanonicalLanguageTypes
 missingIRType
diff --git a/cpp/ql/test/library-tests/ir/ssa/aliased_ssa_sanity_unsound.expected b/cpp/ql/test/library-tests/ir/ssa/aliased_ssa_sanity_unsound.expected
@@ -16,6 +16,8 @@ lostReachability
 backEdgeCountMismatch
 useNotDominatedByDefinition
 switchInstructionWithoutDefaultEdge
+notMarkedAsConflated
+wronglyMarkedAsConflated
 missingCanonicalLanguageType
 multipleCanonicalLanguageTypes
 missingIRType
diff --git a/cpp/ql/test/library-tests/ir/ssa/unaliased_ssa_sanity_unsound.expected b/cpp/ql/test/library-tests/ir/ssa/unaliased_ssa_sanity_unsound.expected
@@ -16,6 +16,8 @@ lostReachability
 backEdgeCountMismatch
 useNotDominatedByDefinition
 switchInstructionWithoutDefaultEdge
+notMarkedAsConflated
+wronglyMarkedAsConflated
 missingCanonicalLanguageType
 multipleCanonicalLanguageTypes
 missingIRType
diff --git a/cpp/ql/test/library-tests/syntax-zoo/aliased_ssa_sanity.expected b/cpp/ql/test/library-tests/syntax-zoo/aliased_ssa_sanity.expected
@@ -568,6 +568,8 @@ lostReachability
 backEdgeCountMismatch
 useNotDominatedByDefinition
 switchInstructionWithoutDefaultEdge
+notMarkedAsConflated
+wronglyMarkedAsConflated
 missingCanonicalLanguageType
 multipleCanonicalLanguageTypes
 missingIRType
diff --git a/cpp/ql/test/library-tests/syntax-zoo/raw_sanity.expected b/cpp/ql/test/library-tests/syntax-zoo/raw_sanity.expected
@@ -631,6 +631,8 @@ useNotDominatedByDefinition
 | try_catch.cpp:21:13:21:24 | Address | Operand 'Address' is not dominated by its definition in function '$@'. | try_catch.cpp:19:6:19:23 | IR: throw_from_nonstmt | void throw_from_nonstmt(int) |
 | vla.c:3:27:3:30 | Address | Operand 'Address' is not dominated by its definition in function '$@'. | vla.c:3:5:3:8 | IR: main | int main(int, char**) |
 switchInstructionWithoutDefaultEdge
+notMarkedAsConflated
+wronglyMarkedAsConflated
 missingCanonicalLanguageType
 multipleCanonicalLanguageTypes
 missingIRType
diff --git a/cpp/ql/test/library-tests/syntax-zoo/unaliased_ssa_sanity.expected b/cpp/ql/test/library-tests/syntax-zoo/unaliased_ssa_sanity.expected
@@ -577,6 +577,8 @@ lostReachability
 backEdgeCountMismatch
 useNotDominatedByDefinition
 switchInstructionWithoutDefaultEdge
+notMarkedAsConflated
+wronglyMarkedAsConflated
 missingCanonicalLanguageType
 multipleCanonicalLanguageTypes
 missingIRType
diff --git a/csharp/ql/src/semmle/code/csharp/ir/implementation/raw/IRSanity.qll b/csharp/ql/src/semmle/code/csharp/ir/implementation/raw/IRSanity.qll
@@ -272,4 +272,34 @@ module InstructionSanity {
     func = switchInstr.getEnclosingIRFunction() and
     funcText = Language::getIdentityString(func.getFunction())
   }
+
+  /**
+   * Holds if `instr` is on the chain of chi/phi instructions for all aliased
+   * memory.
+   */
+  private predicate isOnAliasedDefinitionChain(Instruction instr) {
+    instr instanceof AliasedDefinitionInstruction
+    or
+    instr.getOpcode() instanceof Opcode::InitializeNonLocal
+    or
+    isOnAliasedDefinitionChain(instr.(ChiInstruction).getTotal())
+    or
+    isOnAliasedDefinitionChain(instr.(PhiInstruction).getAnInputOperand().getAnyDef())
+  }
+
+  private predicate shouldBeConflated(Instruction instr) {
+    isOnAliasedDefinitionChain(instr)
+    or
+    instr instanceof UnmodeledDefinitionInstruction
+  }
+
+  query predicate notMarkedAsConflated(Instruction instr) {
+    shouldBeConflated(instr) and
+    not instr.isResultConflated()
+  }
+
+  query predicate wronglyMarkedAsConflated(Instruction instr) {
+    instr.isResultConflated() and
+    not shouldBeConflated(instr)
+  }
 }
diff --git a/csharp/ql/src/semmle/code/csharp/ir/implementation/raw/Instruction.qll b/csharp/ql/src/semmle/code/csharp/ir/implementation/raw/Instruction.qll
diff --git a/csharp/ql/src/semmle/code/csharp/ir/implementation/unaliased_ssa/IRSanity.qll b/csharp/ql/src/semmle/code/csharp/ir/implementation/unaliased_ssa/IRSanity.qll
diff --git a/csharp/ql/src/semmle/code/csharp/ir/implementation/unaliased_ssa/Instruction.qll b/csharp/ql/src/semmle/code/csharp/ir/implementation/unaliased_ssa/Instruction.qll
diff --git a/csharp/ql/src/semmle/code/csharp/ir/implementation/unaliased_ssa/internal/SSAConstruction.qll b/csharp/ql/src/semmle/code/csharp/ir/implementation/unaliased_ssa/internal/SSAConstruction.qll
diff --git a/csharp/ql/src/semmle/code/csharp/ir/implementation/unaliased_ssa/internal/SimpleSSA.qll b/csharp/ql/src/semmle/code/csharp/ir/implementation/unaliased_ssa/internal/SimpleSSA.qll

Original file line number	Diff line number	Diff line change
`@@ -354,6 +354,7 @@ class AllAliasedMemory extends TAllAliasedMemory, MemoryLocation {`
`354`	`354`	`final override predicate isMayAccess() { isMayAccess = true }`
`355`	`355`	`}`
`356`	`356`
	`357`	`+/** A virtual variable that groups all escaped memory within a function. */`
`357`	`358`	`class AliasedVirtualVariable extends AllAliasedMemory, VirtualVariable {`
`358`	`359`	`AliasedVirtualVariable() { not isMayAccess() }`
`359`	`360`	`}`