Merge branch 'master' into rdmarsh/cpp/ir-flow-through-outparams

change-notes/1.24/analysis-csharp.md

@@ -18,6 +18,7 @@ The following changes in version 1.24 affect C# analysis in all applications.
 | **Query**                    | **Expected impact**    | **Change**                        |
 |------------------------------|------------------------|-----------------------------------|
 | Useless assignment to local variable (`cs/useless-assignment-to-local`) | Fewer false positive results | Results have been removed when the variable is named `_` in a `foreach` statement. | 
+| Potentially dangerous use of non-short-circuit logic (`cs/non-short-circuit`) | Fewer false positive results | Results have been removed when the expression contains an `out` parameter. |
 | Dereferenced variable may be null (`cs/dereferenced-value-may-be-null`) | More results | Results are reported from parameters with a default value of `null`. |
 
 ## Removal of old queries

change-notes/1.24/analysis-java.md

@@ -10,10 +10,11 @@ The following changes in version 1.24 affect Java analysis in all applications.
 
 | **Query**                   | **Tags**  | **Purpose**                                                        |
 |-----------------------------|-----------|--------------------------------------------------------------------|
-| Disabled Spring CSRF protection (`java/spring-disabled-csrf-protection`) | security, external/cwe/cwe-352 | Finds disabled Cross-Site Request Forgery (CSRF) protection in Spring. |
+| Disabled Spring CSRF protection (`java/spring-disabled-csrf-protection`) | security, external/cwe/cwe-352 | Finds disabled Cross-Site Request Forgery (CSRF) protection in Spring. Results are shown on LGTM by default. |
 | Failure to use HTTPS or SFTP URL in Maven artifact upload/download (`java/maven/non-https-url`) | security, external/cwe/cwe-300, external/cwe/cwe-319, external/cwe/cwe-494, external/cwe/cwe-829 | Finds use of insecure protocols during Maven dependency resolution. Results are shown on LGTM by default. |
+| LDAP query built from user-controlled sources (`java/ldap-injection`) | security, external/cwe/cwe-090 | Finds LDAP queries vulnerable to injection of unsanitized user-controlled input. Results are shown on LGTM by default. |
 | Left shift by more than the type width (`java/lshift-larger-than-type-width`) | correctness | Finds left shifts of ints by 32 bits or more and left shifts of longs by 64 bits or more. Results are shown on LGTM by default. |
-| Suspicious date format (`java/suspicious-date-format`) | correctness | Finds date format patterns that use placeholders that are likely to be incorrect. |
+| Suspicious date format (`java/suspicious-date-format`) | correctness | Finds date format patterns that use placeholders that are likely to be incorrect. Results are shown on LGTM by default. |
 
 ## Changes to existing queries
 

change-notes/1.24/analysis-javascript.md

@@ -42,6 +42,7 @@
 | Expression has no effect (`js/useless-expression`) | Fewer false positive results | The query now recognizes block-level flow type annotations and ignores the first statement of a try block. |
 | Use of call stack introspection in strict mode (`js/strict-mode-call-stack-introspection`) | Fewer false positive results | The query no longer flags expression statements. |
 | Missing CSRF middleware (`js/missing-token-validation`) | Fewer false positive results | The query reports fewer duplicates and only flags handlers that explicitly access cookie data. |
+| Uncontrolled data used in path expression (`js/path-injection`) | More results | This query now recognizes additional ways dangerous paths can be constructed. |
 
 ## Changes to libraries
 

cpp/ql/src/semmle/code/cpp/dataflow/internal/DataFlowImpl.qll

@@ -2041,12 +2041,12 @@ private class PathNodeMid extends PathNode, TPathNodeMid {
     result = getSuccMid()
     or
     // a final step to a sink via zero steps means we merge the last two steps to prevent trivial-looking edges
-    exists(PathNodeMid mid |
+    exists(PathNodeMid mid, PathNodeSink sink |
       mid = getSuccMid() and
-      mid.getNode() = result.getNode() and
+      mid.getNode() = sink.getNode() and
       mid.getAp() instanceof AccessPathNil and
-      result instanceof PathNodeSink and
-      result.getConfiguration() = unbind(mid.getConfiguration())
+      sink.getConfiguration() = unbind(mid.getConfiguration()) and
+      result = sink
     )
   }
 

cpp/ql/src/semmle/code/cpp/dataflow/internal/DataFlowImpl2.qll

@@ -2041,12 +2041,12 @@ private class PathNodeMid extends PathNode, TPathNodeMid {
     result = getSuccMid()
     or
     // a final step to a sink via zero steps means we merge the last two steps to prevent trivial-looking edges
-    exists(PathNodeMid mid |
+    exists(PathNodeMid mid, PathNodeSink sink |
       mid = getSuccMid() and
-      mid.getNode() = result.getNode() and
+      mid.getNode() = sink.getNode() and
       mid.getAp() instanceof AccessPathNil and
-      result instanceof PathNodeSink and
-      result.getConfiguration() = unbind(mid.getConfiguration())
+      sink.getConfiguration() = unbind(mid.getConfiguration()) and
+      result = sink
     )
   }
 

cpp/ql/src/semmle/code/cpp/dataflow/internal/DataFlowImpl3.qll

@@ -2041,12 +2041,12 @@ private class PathNodeMid extends PathNode, TPathNodeMid {
     result = getSuccMid()
     or
     // a final step to a sink via zero steps means we merge the last two steps to prevent trivial-looking edges
-    exists(PathNodeMid mid |
+    exists(PathNodeMid mid, PathNodeSink sink |
       mid = getSuccMid() and
-      mid.getNode() = result.getNode() and
+      mid.getNode() = sink.getNode() and
       mid.getAp() instanceof AccessPathNil and
-      result instanceof PathNodeSink and
-      result.getConfiguration() = unbind(mid.getConfiguration())
+      sink.getConfiguration() = unbind(mid.getConfiguration()) and
+      result = sink
     )
   }
 

cpp/ql/src/semmle/code/cpp/dataflow/internal/DataFlowImpl4.qll

@@ -2041,12 +2041,12 @@ private class PathNodeMid extends PathNode, TPathNodeMid {
     result = getSuccMid()
     or
     // a final step to a sink via zero steps means we merge the last two steps to prevent trivial-looking edges
-    exists(PathNodeMid mid |
+    exists(PathNodeMid mid, PathNodeSink sink |
       mid = getSuccMid() and
-      mid.getNode() = result.getNode() and
+      mid.getNode() = sink.getNode() and
       mid.getAp() instanceof AccessPathNil and
-      result instanceof PathNodeSink and
-      result.getConfiguration() = unbind(mid.getConfiguration())
+      sink.getConfiguration() = unbind(mid.getConfiguration()) and
+      result = sink
     )
   }
 

cpp/ql/src/semmle/code/cpp/dataflow/internal/DataFlowImplLocal.qll

@@ -2041,12 +2041,12 @@ private class PathNodeMid extends PathNode, TPathNodeMid {
     result = getSuccMid()
     or
     // a final step to a sink via zero steps means we merge the last two steps to prevent trivial-looking edges
-    exists(PathNodeMid mid |
+    exists(PathNodeMid mid, PathNodeSink sink |
       mid = getSuccMid() and
-      mid.getNode() = result.getNode() and
+      mid.getNode() = sink.getNode() and
       mid.getAp() instanceof AccessPathNil and
-      result instanceof PathNodeSink and
-      result.getConfiguration() = unbind(mid.getConfiguration())
+      sink.getConfiguration() = unbind(mid.getConfiguration()) and
+      result = sink
     )
   }
 

cpp/ql/src/semmle/code/cpp/dataflow/internal/DataFlowUtil.qll

@@ -6,7 +6,6 @@ private import cpp
 private import semmle.code.cpp.dataflow.internal.FlowVar
 private import semmle.code.cpp.models.interfaces.DataFlow
 private import semmle.code.cpp.controlflow.Guards
-private import semmle.code.cpp.valuenumbering.GlobalValueNumbering
 
 cached
 private newtype TNode =
@@ -689,9 +688,9 @@ class BarrierGuard extends GuardCondition {
 
   /** Gets a node guarded by this guard. */
   final ExprNode getAGuardedNode() {
-    exists(GVN value, boolean branch |
-      result.getExpr() = value.getAnExpr() and
-      this.checks(value.getAnExpr(), branch) and
+    exists(SsaDefinition def, Variable v, boolean branch |
+      result.getExpr() = def.getAUse(v) and
+      this.checks(def.getAUse(v), branch) and
       this.controls(result.getExpr().getBasicBlock(), branch)
     )
   }

cpp/ql/src/semmle/code/cpp/ir/dataflow/DefaultTaintTracking.qll

@@ -21,6 +21,40 @@ private predicate predictableInstruction(Instruction instr) {
   predictableInstruction(instr.(UnaryInstruction).getUnary())
 }
 
+/**
+ * Functions that we should only allow taint to flow through (to the return
+ * value) if all but the source argument are 'predictable'.  This is done to
+ * emulate the old security library's implementation rather than due to any
+ * strong belief that this is the right approach.
+ *
+ * Note that the list itself is not very principled; it consists of all the
+ * functions listed in the old security library's [default] `isPureFunction`
+ * that have more than one argument, but are not in the old taint tracking
+ * library's `returnArgument` predicate.  In addition, `strlen` is included
+ * because it's also a special case in flow to return values.
+ */
+predicate predictableOnlyFlow(string name) {
+  name = "strcasestr" or
+  name = "strchnul" or
+  name = "strchr" or
+  name = "strchrnul" or
+  name = "strcmp" or
+  name = "strcspn" or
+  name = "strlen" or // special case
+  name = "strncmp" or
+  name = "strndup" or
+  name = "strnlen" or
+  name = "strrchr" or
+  name = "strspn" or
+  name = "strstr" or
+  name = "strtod" or
+  name = "strtof" or
+  name = "strtol" or
+  name = "strtoll" or
+  name = "strtoq" or
+  name = "strtoul"
+}
+
 private DataFlow::Node getNodeForSource(Expr source) {
   isUserInput(source, _) and
   (
@@ -123,15 +157,16 @@ private predicate nodeIsBarrier(DataFlow::Node node) {
 
 private predicate instructionTaintStep(Instruction i1, Instruction i2) {
   // Expressions computed from tainted data are also tainted
-  i2 =
-    any(CallInstruction call |
-      isPureFunction(call.getStaticCallTarget().getName()) and
-      call.getAnArgument() = i1 and
-      forall(Instruction arg | arg = call.getAnArgument() | arg = i1 or predictableInstruction(arg)) and
-      // flow through `strlen` tends to cause dubious results, if the length is
-      // bounded.
-      not call.getStaticCallTarget().getName() = "strlen"
-    )
+  exists(CallInstruction call, int argIndex | call = i2 |
+    isPureFunction(call.getStaticCallTarget().getName()) and
+    i1 = getACallArgumentOrIndirection(call, argIndex) and
+    forall(Instruction arg | arg = call.getAnArgument() |
+      arg = getACallArgumentOrIndirection(call, argIndex) or predictableInstruction(arg)
+    ) and
+    // flow through `strlen` tends to cause dubious results, if the length is
+    // bounded.
+    not call.getStaticCallTarget().getName() = "strlen"
+  )
   or
   // Flow through pointer dereference
   i2.(LoadInstruction).getSourceAddress() = i1
@@ -174,7 +209,8 @@ private predicate instructionTaintStep(Instruction i1, Instruction i2) {
     any(CallInstruction call |
       exists(int indexIn |
         modelTaintToReturnValue(call.getStaticCallTarget(), indexIn) and
-        i1 = getACallArgumentOrIndirection(call, indexIn)
+        i1 = getACallArgumentOrIndirection(call, indexIn) and
+        not predictableOnlyFlow(call.getStaticCallTarget().getName())
       )
     )
   or

cpp/ql/src/semmle/code/cpp/ir/dataflow/internal/DataFlowImpl.qll

@@ -2041,12 +2041,12 @@ private class PathNodeMid extends PathNode, TPathNodeMid {
     result = getSuccMid()
     or
     // a final step to a sink via zero steps means we merge the last two steps to prevent trivial-looking edges
-    exists(PathNodeMid mid |
+    exists(PathNodeMid mid, PathNodeSink sink |
       mid = getSuccMid() and
-      mid.getNode() = result.getNode() and
+      mid.getNode() = sink.getNode() and
       mid.getAp() instanceof AccessPathNil and
-      result instanceof PathNodeSink and
-      result.getConfiguration() = unbind(mid.getConfiguration())
+      sink.getConfiguration() = unbind(mid.getConfiguration()) and
+      result = sink
     )
   }
 

cpp/ql/src/semmle/code/cpp/ir/dataflow/internal/DataFlowImpl2.qll

@@ -2041,12 +2041,12 @@ private class PathNodeMid extends PathNode, TPathNodeMid {
     result = getSuccMid()
     or
     // a final step to a sink via zero steps means we merge the last two steps to prevent trivial-looking edges
-    exists(PathNodeMid mid |
+    exists(PathNodeMid mid, PathNodeSink sink |
       mid = getSuccMid() and
-      mid.getNode() = result.getNode() and
+      mid.getNode() = sink.getNode() and
       mid.getAp() instanceof AccessPathNil and
-      result instanceof PathNodeSink and
-      result.getConfiguration() = unbind(mid.getConfiguration())
+      sink.getConfiguration() = unbind(mid.getConfiguration()) and
+      result = sink
     )
   }
 

cpp/ql/src/semmle/code/cpp/ir/dataflow/internal/DataFlowImpl3.qll

@@ -2041,12 +2041,12 @@ private class PathNodeMid extends PathNode, TPathNodeMid {
     result = getSuccMid()
     or
     // a final step to a sink via zero steps means we merge the last two steps to prevent trivial-looking edges
-    exists(PathNodeMid mid |
+    exists(PathNodeMid mid, PathNodeSink sink |
       mid = getSuccMid() and
-      mid.getNode() = result.getNode() and
+      mid.getNode() = sink.getNode() and
       mid.getAp() instanceof AccessPathNil and
-      result instanceof PathNodeSink and
-      result.getConfiguration() = unbind(mid.getConfiguration())
+      sink.getConfiguration() = unbind(mid.getConfiguration()) and
+      result = sink
     )
   }
 

cpp/ql/src/semmle/code/cpp/ir/dataflow/internal/DataFlowImpl4.qll

@@ -2041,12 +2041,12 @@ private class PathNodeMid extends PathNode, TPathNodeMid {
     result = getSuccMid()
     or
     // a final step to a sink via zero steps means we merge the last two steps to prevent trivial-looking edges
-    exists(PathNodeMid mid |
+    exists(PathNodeMid mid, PathNodeSink sink |
       mid = getSuccMid() and
-      mid.getNode() = result.getNode() and
+      mid.getNode() = sink.getNode() and
       mid.getAp() instanceof AccessPathNil and
-      result instanceof PathNodeSink and
-      result.getConfiguration() = unbind(mid.getConfiguration())
+      sink.getConfiguration() = unbind(mid.getConfiguration()) and
+      result = sink
     )
   }
 

cpp/ql/src/semmle/code/cpp/ir/dataflow/internal/DataFlowUtil.qll

@@ -6,6 +6,7 @@ private import cpp
 private import semmle.code.cpp.ir.IR
 private import semmle.code.cpp.controlflow.IRGuards
 private import semmle.code.cpp.ir.ValueNumbering
+private import semmle.code.cpp.models.interfaces.DataFlow
 
 /**
  * A newtype wrapper to prevent accidental casts between `Node` and
@@ -289,6 +290,51 @@ private predicate simpleInstructionLocalFlowStep(Instruction iFrom, Instruction
   // Flow through the partial operand belongs in the taint-tracking libraries
   // for now.
   iTo.getAnOperand().(ChiTotalOperand).getDef() = iFrom
+  or
+  // Flow through modeled functions
+  modelFlow(iFrom, iTo)
+}
+
+private predicate modelFlow(Instruction iFrom, Instruction iTo) {
+  exists(
+    CallInstruction call, DataFlowFunction func, FunctionInput modelIn, FunctionOutput modelOut
+  |
+    call.getStaticCallTarget() = func and
+    func.hasDataFlow(modelIn, modelOut)
+  |
+    (
+      modelOut.isReturnValue() and
+      iTo = call
+      or
+      // TODO: Add write side effects for return values
+      modelOut.isReturnValueDeref() and
+      iTo = call
+      or
+      exists(WriteSideEffectInstruction outNode |
+        modelOut.isParameterDeref(outNode.getIndex()) and
+        iTo = outNode and
+        outNode.getPrimaryInstruction() = call
+      )
+      // TODO: add write side effects for qualifiers
+    ) and
+    (
+      exists(int index |
+        modelIn.isParameter(index) and
+        iFrom = call.getPositionalArgument(index)
+      )
+      or
+      exists(int index, ReadSideEffectInstruction read |
+        modelIn.isParameterDeref(index) and
+        read.getIndex() = index and
+        read.getPrimaryInstruction() = call and
+        iFrom = read.getSideEffectOperand().getAnyDef()
+      )
+      or
+      modelIn.isQualifierAddress() and
+      iFrom = call.getThisArgument()
+      // TODO: add read side effects for qualifiers
+    )
+  )
 }
 
 /**

cpp/ql/src/semmle/code/cpp/ir/implementation/Opcode.qll

@@ -63,6 +63,7 @@ private newtype TOpcode =
   TUnmodeledDefinition() or
   TUnmodeledUse() or
   TAliasedDefinition() or
+  TInitializeNonLocal() or
   TAliasedUse() or
   TPhi() or
   TBuiltIn() or
@@ -596,6 +597,14 @@ module Opcode {
     final override MemoryAccessKind getWriteMemoryAccess() { result instanceof EscapedMemoryAccess }
   }
 
+  class InitializeNonLocal extends Opcode, TInitializeNonLocal {
+    final override string toString() { result = "InitializeNonLocal" }
+
+    final override MemoryAccessKind getWriteMemoryAccess() {
+      result instanceof NonLocalMemoryAccess
+    }
+  }
+
   class AliasedUse extends Opcode, TAliasedUse {
     final override string toString() { result = "AliasedUse" }
 

cpp/ql/src/semmle/code/cpp/ir/implementation/aliased_ssa/gvn/ValueNumbering.qll

@@ -52,6 +52,11 @@ newtype TValueNumber =
   ) {
     inheritanceConversionValueNumber(_, irFunc, opcode, baseClass, derivedClass, operand)
   } or
+  TLoadTotalOverlapValueNumber(
+    IRFunction irFunc, IRType type, ValueNumber memOperand, ValueNumber operand
+  ) {
+    loadTotalOverlapValueNumber(_, irFunc, type, memOperand, operand)
+  } or
   TUniqueValueNumber(IRFunction irFunc, Instruction instr) { uniqueValueNumber(instr, irFunc) }
 
 /**
@@ -101,12 +106,18 @@ class ValueNumber extends TValueNumber {
  * The use of `p.x` on line 3 is linked to the definition of `p` on line 1 as well, but is not
  * congruent to that definition because `p.x` accesses only a subset of the memory defined by `p`.
  */
-private class CongruentCopyInstruction extends CopyInstruction {
+class CongruentCopyInstruction extends CopyInstruction {
   CongruentCopyInstruction() {
     this.getSourceValueOperand().getDefinitionOverlap() instanceof MustExactlyOverlap
   }
 }
 
+class LoadTotalOverlapInstruction extends LoadInstruction {
+  LoadTotalOverlapInstruction() {
+    this.getSourceValueOperand().getDefinitionOverlap() instanceof MustTotallyOverlap
+  }
+}
+
 /**
  * Holds if this library knows how to assign a value number to the specified instruction, other than
  * a `unique` value number that is never shared by multiple instructions.
@@ -131,6 +142,8 @@ private predicate numberableInstruction(Instruction instr) {
   instr instanceof PointerArithmeticInstruction
   or
   instr instanceof CongruentCopyInstruction
+  or
+  instr instanceof LoadTotalOverlapInstruction
 }
 
 private predicate variableAddressValueNumber(
@@ -205,6 +218,7 @@ private predicate unaryValueNumber(
   instr.getEnclosingIRFunction() = irFunc and
   not instr instanceof InheritanceConversionInstruction and
   not instr instanceof CopyInstruction and
+  not instr instanceof FieldAddressInstruction and
   instr.getOpcode() = opcode and
   instr.getResultIRType() = type and
   valueNumber(instr.getUnary()) = operand
@@ -221,6 +235,16 @@ private predicate inheritanceConversionValueNumber(
   valueNumber(instr.getUnary()) = operand
 }
 
+private predicate loadTotalOverlapValueNumber(
+  LoadTotalOverlapInstruction instr, IRFunction irFunc, IRType type, ValueNumber memOperand,
+  ValueNumber operand
+) {
+  instr.getEnclosingIRFunction() = irFunc and
+  instr.getResultIRType() = type and
+  valueNumber(instr.getAnOperand().(MemoryOperand).getAnyDef()) = memOperand and
+  valueNumberOfOperand(instr.getAnOperand().(AddressOperand)) = operand
+}
+
 /**
  * Holds if `instr` should be assigned a unique value number because this library does not know how
  * to determine if two instances of that instruction are equivalent.
@@ -313,6 +337,11 @@ private ValueNumber nonUniqueValueNumber(Instruction instr) {
           TPointerArithmeticValueNumber(irFunc, opcode, type, elementSize, leftOperand, rightOperand)
       )
       or
+      exists(IRType type, ValueNumber memOperand, ValueNumber operand |
+        loadTotalOverlapValueNumber(instr, irFunc, type, memOperand, operand) and
+        result = TLoadTotalOverlapValueNumber(irFunc, type, memOperand, operand)
+      )
+      or
       // The value number of a copy is just the value number of its source value.
       result = valueNumber(instr.(CongruentCopyInstruction).getSourceValue())
     )

cpp/ql/src/semmle/code/cpp/ir/implementation/aliased_ssa/internal/AliasedSSA.qll

@@ -26,7 +26,7 @@ private predicate hasResultMemoryAccess(
     type = languageType.getIRType() and
     isIndirectOrBufferMemoryAccess(instr.getResultMemoryAccess()) and
     (if instr.hasResultMayMemoryAccess() then isMayAccess = true else isMayAccess = false) and
-    if exists(type.getByteSize())
+    if type.getByteSize() > 0
     then endBitOffset = Ints::add(startBitOffset, Ints::mul(type.getByteSize(), 8))
     else endBitOffset = Ints::unknown()
   )
@@ -43,7 +43,7 @@ private predicate hasOperandMemoryAccess(
     type = languageType.getIRType() and
     isIndirectOrBufferMemoryAccess(operand.getMemoryAccess()) and
     (if operand.hasMayReadMemoryAccess() then isMayAccess = true else isMayAccess = false) and
-    if exists(type.getByteSize())
+    if type.getByteSize() > 0
     then endBitOffset = Ints::add(startBitOffset, Ints::mul(type.getByteSize(), 8))
     else endBitOffset = Ints::unknown()
   )
@@ -298,7 +298,7 @@ class AllNonLocalMemory extends TAllNonLocalMemory, MemoryLocation {
 
   final override string toStringInternal() { result = "{AllNonLocal}" }
 
-  final override VirtualVariable getVirtualVariable() { result = TAllAliasedMemory(irFunc, false) }
+  final override AliasedVirtualVariable getVirtualVariable() { result.getIRFunction() = irFunc }
 
   final override Language::LanguageType getType() {
     result = any(IRUnknownType type).getCanonicalLanguageType()
@@ -311,6 +311,14 @@ class AllNonLocalMemory extends TAllNonLocalMemory, MemoryLocation {
   final override string getUniqueId() { result = "{AllNonLocal}" }
 
   final override predicate isMayAccess() { isMayAccess = true }
+
+  override predicate canDefineReadOnly() {
+    // A "must" access that defines all non-local memory appears only on the `InitializeNonLocal`
+    // instruction, which provides the initial definition for all memory outside of the current
+    // function's stack frame. This memory includes string literals and other read-only globals, so
+    // we allow such an access to be the definition for a use of a read-only location.
+    not isMayAccess()
+  }
 }
 
 /**
@@ -341,16 +349,6 @@ class AllAliasedMemory extends TAllAliasedMemory, MemoryLocation {
 
 class AliasedVirtualVariable extends AllAliasedMemory, VirtualVariable {
   AliasedVirtualVariable() { not isMayAccess() }
-
-  override predicate canDefineReadOnly() {
-    // A must-def of all aliased memory is only used in two places:
-    // 1. In the prologue of the function, to provide a definition for all memory defined before the
-    //    function was called. In this case, it needs to provide a definition even for read-only
-    //    non-local variables.
-    // 2. As the result of a `Chi` instruction. These don't participate in overlap analysis, so it's
-    //    OK if we let this predicate hold in that case.
-    any()
-  }
 }
 
 /**
@@ -405,10 +403,16 @@ private Overlap getExtentOverlap(MemoryLocation def, MemoryLocation use) {
       use instanceof AllNonLocalMemory and
       result instanceof MustExactlyOverlap
       or
-      // AllNonLocalMemory may partially overlap any location within the same virtual variable,
-      // except a local variable.
-      result instanceof MayPartiallyOverlap and
-      not use.isAlwaysAllocatedOnStack()
+      not use instanceof AllNonLocalMemory and
+      not use.isAlwaysAllocatedOnStack() and
+      if use instanceof VariableMemoryLocation
+      then
+        // AllNonLocalMemory totally overlaps any non-local variable.
+        result instanceof MustTotallyOverlap
+      else
+        // AllNonLocalMemory may partially overlap any other location within the same virtual
+        // variable, except a stack variable.
+        result instanceof MayPartiallyOverlap
     )
     or
     def.getVirtualVariable() = use.getVirtualVariable() and

cpp/ql/src/semmle/code/cpp/ir/implementation/aliased_ssa/internal/SSAConstruction.qll

@@ -759,7 +759,21 @@ module DefUse {
       then defLocation = useLocation
       else (
         definitionHasPhiNode(defLocation, block) and
-        defLocation = useLocation.getVirtualVariable()
+        defLocation = useLocation.getVirtualVariable() and
+        // Handle the unusual case where a virtual variable does not overlap one of its member
+        // locations. For example, a definition of the virtual variable representing all aliased
+        // memory does not overlap a use of a string literal, because the contents of a string
+        // literal can never be redefined. The string literal's location could still be a member of
+        // the `AliasedVirtualVariable` due to something like:
+        // ```
+        // char s[10];
+        // strcpy(s, p);
+        // const char* p = b ? "SomeLiteral" : s;
+        // return p[3];
+        // ```
+        // In the above example, `p[3]` may access either the string literal or the local variable
+        // `s`, so both of those locations must be members of the `AliasedVirtualVariable`.
+        exists(Alias::getOverlap(defLocation, useLocation))
       )
     )
     or

cpp/ql/src/semmle/code/cpp/ir/implementation/raw/gvn/ValueNumbering.qll

@@ -52,6 +52,11 @@ newtype TValueNumber =
   ) {
     inheritanceConversionValueNumber(_, irFunc, opcode, baseClass, derivedClass, operand)
   } or
+  TLoadTotalOverlapValueNumber(
+    IRFunction irFunc, IRType type, ValueNumber memOperand, ValueNumber operand
+  ) {
+    loadTotalOverlapValueNumber(_, irFunc, type, memOperand, operand)
+  } or
   TUniqueValueNumber(IRFunction irFunc, Instruction instr) { uniqueValueNumber(instr, irFunc) }
 
 /**
@@ -101,12 +106,18 @@ class ValueNumber extends TValueNumber {
  * The use of `p.x` on line 3 is linked to the definition of `p` on line 1 as well, but is not
  * congruent to that definition because `p.x` accesses only a subset of the memory defined by `p`.
  */
-private class CongruentCopyInstruction extends CopyInstruction {
+class CongruentCopyInstruction extends CopyInstruction {
   CongruentCopyInstruction() {
     this.getSourceValueOperand().getDefinitionOverlap() instanceof MustExactlyOverlap
   }
 }
 
+class LoadTotalOverlapInstruction extends LoadInstruction {
+  LoadTotalOverlapInstruction() {
+    this.getSourceValueOperand().getDefinitionOverlap() instanceof MustTotallyOverlap
+  }
+}
+
 /**
  * Holds if this library knows how to assign a value number to the specified instruction, other than
  * a `unique` value number that is never shared by multiple instructions.
@@ -131,6 +142,8 @@ private predicate numberableInstruction(Instruction instr) {
   instr instanceof PointerArithmeticInstruction
   or
   instr instanceof CongruentCopyInstruction
+  or
+  instr instanceof LoadTotalOverlapInstruction
 }
 
 private predicate variableAddressValueNumber(
@@ -205,6 +218,7 @@ private predicate unaryValueNumber(
   instr.getEnclosingIRFunction() = irFunc and
   not instr instanceof InheritanceConversionInstruction and
   not instr instanceof CopyInstruction and
+  not instr instanceof FieldAddressInstruction and
   instr.getOpcode() = opcode and
   instr.getResultIRType() = type and
   valueNumber(instr.getUnary()) = operand
@@ -221,6 +235,16 @@ private predicate inheritanceConversionValueNumber(
   valueNumber(instr.getUnary()) = operand
 }
 
+private predicate loadTotalOverlapValueNumber(
+  LoadTotalOverlapInstruction instr, IRFunction irFunc, IRType type, ValueNumber memOperand,
+  ValueNumber operand
+) {
+  instr.getEnclosingIRFunction() = irFunc and
+  instr.getResultIRType() = type and
+  valueNumber(instr.getAnOperand().(MemoryOperand).getAnyDef()) = memOperand and
+  valueNumberOfOperand(instr.getAnOperand().(AddressOperand)) = operand
+}
+
 /**
  * Holds if `instr` should be assigned a unique value number because this library does not know how
  * to determine if two instances of that instruction are equivalent.
@@ -313,6 +337,11 @@ private ValueNumber nonUniqueValueNumber(Instruction instr) {
           TPointerArithmeticValueNumber(irFunc, opcode, type, elementSize, leftOperand, rightOperand)
       )
       or
+      exists(IRType type, ValueNumber memOperand, ValueNumber operand |
+        loadTotalOverlapValueNumber(instr, irFunc, type, memOperand, operand) and
+        result = TLoadTotalOverlapValueNumber(irFunc, type, memOperand, operand)
+      )
+      or
       // The value number of a copy is just the value number of its source value.
       result = valueNumber(instr.(CongruentCopyInstruction).getSourceValue())
     )

cpp/ql/src/semmle/code/cpp/ir/implementation/raw/internal/InstructionTag.qll

@@ -12,12 +12,10 @@ newtype TInstructionTag =
   ZeroPadStringElementIndexTag() or
   ZeroPadStringElementAddressTag() or
   ZeroPadStringStoreTag() or
-  AssignOperationLoadTag() or
   AssignOperationConvertLeftTag() or
   AssignOperationOpTag() or
   AssignOperationConvertResultTag() or
   AssignmentStoreTag() or
-  CrementLoadTag() or
   CrementConstantTag() or
   CrementOpTag() or
   CrementStoreTag() or
@@ -28,6 +26,7 @@ newtype TInstructionTag =
   UnmodeledDefinitionTag() or
   UnmodeledUseTag() or
   AliasedDefinitionTag() or
+  InitializeNonLocalTag() or
   AliasedUseTag() or
   SwitchBranchTag() or
   CallTargetTag() or
@@ -94,8 +93,6 @@ string getInstructionTagId(TInstructionTag tag) {
   or
   tag = ZeroPadStringStoreTag() and result = "ZeroPadStore"
   or
-  tag = AssignOperationLoadTag() and result = "AssignOpLoad"
-  or
   tag = AssignOperationConvertLeftTag() and result = "AssignOpConvLeft"
   or
   tag = AssignOperationOpTag() and result = "AssignOpOp"
@@ -104,8 +101,6 @@ string getInstructionTagId(TInstructionTag tag) {
   or
   tag = AssignmentStoreTag() and result = "AssignStore"
   or
-  tag = CrementLoadTag() and result = "CrementLoad"
-  or
   tag = CrementConstantTag() and result = "CrementConst"
   or
   tag = CrementOpTag() and result = "CrementOp"
@@ -126,6 +121,8 @@ string getInstructionTagId(TInstructionTag tag) {
   or
   tag = AliasedDefinitionTag() and result = "AliasedDef"
   or
+  tag = InitializeNonLocalTag() and result = "InitNonLocal"
+  or
   tag = AliasedUseTag() and result = "AliasedUse"
   or
   tag = SwitchBranchTag() and result = "SwitchBranch"

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

@@ -208,7 +208,7 @@ private predicate usedAsCondition(Expr expr) {
  * AST as an lvalue-to-rvalue conversion, but the IR represents both a function
  * lvalue and a function pointer prvalue the same.
  */
-predicate ignoreLoad(Expr expr) {
+private predicate ignoreLoad(Expr expr) {
   expr.hasLValueToRValueConversion() and
   (
     expr instanceof ThisExpr or
@@ -220,6 +220,34 @@ predicate ignoreLoad(Expr expr) {
   )
 }
 
+/**
+ * Holds if `expr` should have a load on it because it will be loaded as part
+ * of the translation of its parent. We want to associate this load with `expr`
+ * itself rather than its parent since in practical applications like data flow
+ * we maintain that the value of the `x` in `x++` should be what's loaded from
+ * `x`.
+ */
+private predicate needsLoadForParentExpr(Expr expr) {
+  exists(CrementOperation crement | expr = crement.getOperand().getFullyConverted())
+  or
+  exists(AssignOperation ao | expr = ao.getLValue().getFullyConverted())
+}
+
+/**
+ * Holds if `expr` should have a `TranslatedLoad` on it.
+ */
+predicate hasTranslatedLoad(Expr expr) {
+  (
+    expr.hasLValueToRValueConversion()
+    or
+    needsLoadForParentExpr(expr)
+  ) and
+  not ignoreExpr(expr) and
+  not isNativeCondition(expr) and
+  not isFlexibleCondition(expr) and
+  not ignoreLoad(expr)
+}
+
 newtype TTranslatedElement =
   // An expression that is not being consumed as a condition
   TTranslatedValueExpr(Expr expr) {
@@ -229,21 +257,12 @@ newtype TTranslatedElement =
   } or
   // A separate element to handle the lvalue-to-rvalue conversion step of an
   // expression.
-  TTranslatedLoad(Expr expr) {
-    not ignoreExpr(expr) and
-    not isNativeCondition(expr) and
-    not isFlexibleCondition(expr) and
-    expr.hasLValueToRValueConversion() and
-    not ignoreLoad(expr)
-  } or
+  TTranslatedLoad(Expr expr) { hasTranslatedLoad(expr) } or
+  // For expressions that would not otherwise generate an instruction.
   TTranslatedResultCopy(Expr expr) {
     not ignoreExpr(expr) and
     exprNeedsCopyIfNotLoaded(expr) and
-    // Doesn't have a TTranslatedLoad
-    not (
-      expr.hasLValueToRValueConversion() and
-      not ignoreLoad(expr)
-    )
+    not hasTranslatedLoad(expr)
   } or
   // An expression most naturally translated as control flow.
   TTranslatedNativeCondition(Expr expr) {

cpp/ql/src/semmle/code/cpp/ir/implementation/raw/internal/TranslatedExpr.qll

@@ -63,9 +63,12 @@ abstract class TranslatedExpr extends TranslatedElement {
    * Holds if the result of this `TranslatedExpr` is a glvalue.
    */
   predicate isResultGLValue() {
-    // This implementation is overridden in `TranslatedCoreExpr` to mark them
-    // as glvalues if they have loads on them. It's not overridden in
-    // `TranslatedResultCopy` since result copies never have loads.
+    // This implementation is overridden in `TranslatedCoreExpr` to mark them as
+    // glvalues if they have loads on them. It's also overridden in
+    // `TranslatedLoad` to always mark loads as glvalues since a
+    // `TranslatedLoad` may have been created as a result of
+    // `needsLoadForParentExpr`. It's not overridden in `TranslatedResultCopy`
+    // since result copies never have loads.
     expr.isGLValueCategory()
   }
 
@@ -103,18 +106,13 @@ abstract class TranslatedCoreExpr extends TranslatedExpr {
     or
     // If this TranslatedExpr doesn't produce the result, then it must represent
     // a glvalue that is then loaded by a TranslatedLoad.
-    hasLoad()
-  }
-
-  final predicate hasLoad() {
-    expr.hasLValueToRValueConversion() and
-    not ignoreLoad(expr)
+    hasTranslatedLoad(expr)
   }
 
   final override predicate producesExprResult() {
     // If there's no load, then this is the only TranslatedExpr for this
     // expression.
-    not hasLoad() and
+    not hasTranslatedLoad(expr) and
     // If there's a result copy, then this expression's result is the copy.
     not exprNeedsCopyIfNotLoaded(expr)
   }
@@ -270,6 +268,8 @@ class TranslatedLoad extends TranslatedExpr, TTranslatedLoad {
     resultType = getResultType()
   }
 
+  override predicate isResultGLValue() { none() }
+
   override Instruction getInstructionSuccessor(InstructionTag tag, EdgeKind kind) {
     tag = LoadTag() and
     result = getParent().getChildSuccessor(this) and
@@ -298,7 +298,7 @@ class TranslatedLoad extends TranslatedExpr, TTranslatedLoad {
     any()
   }
 
-  private TranslatedCoreExpr getOperand() { result.getExpr() = expr }
+  TranslatedCoreExpr getOperand() { result.getExpr() = expr }
 }
 
 /**
@@ -387,7 +387,7 @@ private int getElementSize(Type type) {
 abstract class TranslatedCrementOperation extends TranslatedNonConstantExpr {
   override CrementOperation expr;
 
-  final override TranslatedElement getChild(int id) { id = 0 and result = getOperand() }
+  final override TranslatedElement getChild(int id) { id = 0 and result = getLoadedOperand() }
 
   final override string getInstructionConstantValue(InstructionTag tag) {
     tag = CrementConstantTag() and
@@ -416,10 +416,6 @@ abstract class TranslatedCrementOperation extends TranslatedNonConstantExpr {
   }
 
   final override predicate hasInstruction(Opcode opcode, InstructionTag tag, CppType resultType) {
-    tag = CrementLoadTag() and
-    opcode instanceof Opcode::Load and
-    resultType = getTypeForPRValue(expr.getType())
-    or
     tag = CrementConstantTag() and
     opcode instanceof Opcode::Constant and
     resultType = getConstantType()
@@ -434,19 +430,10 @@ abstract class TranslatedCrementOperation extends TranslatedNonConstantExpr {
   }
 
   final override Instruction getInstructionOperand(InstructionTag tag, OperandTag operandTag) {
-    tag = CrementLoadTag() and
-    (
-      operandTag instanceof AddressOperandTag and
-      result = getOperand().getResult()
-      or
-      operandTag instanceof LoadOperandTag and
-      result = getEnclosingFunction().getUnmodeledDefinitionInstruction()
-    )
-    or
     tag = CrementOpTag() and
     (
       operandTag instanceof LeftOperandTag and
-      result = getInstruction(CrementLoadTag())
+      result = getLoadedOperand().getResult()
       or
       operandTag instanceof RightOperandTag and
       result = getInstruction(CrementConstantTag())
@@ -455,21 +442,20 @@ abstract class TranslatedCrementOperation extends TranslatedNonConstantExpr {
     tag = CrementStoreTag() and
     (
       operandTag instanceof AddressOperandTag and
-      result = getOperand().getResult()
+      result = getUnloadedOperand().getResult()
       or
       operandTag instanceof StoreValueOperandTag and
       result = getInstruction(CrementOpTag())
     )
   }
 
-  final override Instruction getFirstInstruction() { result = getOperand().getFirstInstruction() }
+  final override Instruction getFirstInstruction() {
+    result = getLoadedOperand().getFirstInstruction()
+  }
 
   final override Instruction getInstructionSuccessor(InstructionTag tag, EdgeKind kind) {
     kind instanceof GotoEdge and
     (
-      tag = CrementLoadTag() and
-      result = getInstruction(CrementConstantTag())
-      or
       tag = CrementConstantTag() and
       result = getInstruction(CrementOpTag())
       or
@@ -482,7 +468,7 @@ abstract class TranslatedCrementOperation extends TranslatedNonConstantExpr {
   }
 
   final override Instruction getChildSuccessor(TranslatedElement child) {
-    child = getOperand() and result = getInstruction(CrementLoadTag())
+    child = getLoadedOperand() and result = getInstruction(CrementConstantTag())
   }
 
   final override int getInstructionElementSize(InstructionTag tag) {
@@ -494,10 +480,20 @@ abstract class TranslatedCrementOperation extends TranslatedNonConstantExpr {
     result = getElementSize(expr.getType())
   }
 
-  final TranslatedExpr getOperand() {
+  /**
+   * Gets the `TranslatedLoad` on the `e` in this `e++`, which is the element
+   * that holds the value to be cremented. It's guaranteed that there's a load
+   * on `e` because of the `needsLoadForParentExpr` predicate.
+   */
+  final TranslatedLoad getLoadedOperand() {
     result = getTranslatedExpr(expr.getOperand().getFullyConverted())
   }
 
+  /**
+   * Gets the address to which the result of this crement will be stored.
+   */
+  final TranslatedExpr getUnloadedOperand() { result = getLoadedOperand().getOperand() }
+
   final Opcode getOpcode() {
     exists(Type resultType |
       resultType = expr.getUnspecifiedType() and
@@ -534,17 +530,14 @@ class TranslatedPrefixCrementOperation extends TranslatedCrementOperation {
     else
       // This is C++, where the result is an lvalue for the operand, and that
       // lvalue is not being loaded as part of this expression.
-      result = getOperand().getResult()
+      result = getUnloadedOperand().getResult()
   }
 }
 
 class TranslatedPostfixCrementOperation extends TranslatedCrementOperation {
   override PostfixCrementOperation expr;
 
-  override Instruction getResult() {
-    // The result is a prvalue copy of the original value
-    result = getInstruction(CrementLoadTag())
-  }
+  override Instruction getResult() { result = getLoadedOperand().getResult() }
 }
 
 /**
@@ -1231,8 +1224,8 @@ class TranslatedBinaryOperation extends TranslatedSingleInstructionExpr {
   }
 }
 
-abstract class TranslatedAssignment extends TranslatedNonConstantExpr {
-  override Assignment expr;
+class TranslatedAssignExpr extends TranslatedNonConstantExpr {
+  override AssignExpr expr;
 
   final override TranslatedElement getChild(int id) {
     id = 0 and result = getLeftOperand()
@@ -1252,7 +1245,7 @@ abstract class TranslatedAssignment extends TranslatedNonConstantExpr {
       // value assigned to the left operand. If this is C++, then the result is
       // an lvalue, but that lvalue is being loaded as part of this expression.
       // EDG doesn't mark this as a load.
-      result = getStoredValue()
+      result = getRightOperand().getResult()
     else
       // This is C++, where the result is an lvalue for the left operand,
       // and that lvalue is not being loaded as part of this expression.
@@ -1268,10 +1261,6 @@ abstract class TranslatedAssignment extends TranslatedNonConstantExpr {
   final TranslatedExpr getRightOperand() {
     result = getTranslatedExpr(expr.getRValue().getFullyConverted())
   }
-}
-
-class TranslatedAssignExpr extends TranslatedAssignment {
-  TranslatedAssignExpr() { expr instanceof AssignExpr }
 
   override Instruction getInstructionSuccessor(InstructionTag tag, EdgeKind kind) {
     tag = AssignmentStoreTag() and
@@ -1304,23 +1293,57 @@ class TranslatedAssignExpr extends TranslatedAssignment {
       result = getRightOperand().getResult()
     )
   }
-
-  override Instruction getStoredValue() { result = getRightOperand().getResult() }
 }
 
-class TranslatedAssignOperation extends TranslatedAssignment {
+class TranslatedAssignOperation extends TranslatedNonConstantExpr {
   override AssignOperation expr;
 
+  final override TranslatedElement getChild(int id) {
+    id = 0 and result = getLoadedLeftOperand()
+    or
+    id = 1 and result = getRightOperand()
+  }
+
+  final override Instruction getFirstInstruction() {
+    // Evaluation is right-to-left
+    result = getRightOperand().getFirstInstruction()
+  }
+
+  final override Instruction getResult() {
+    if expr.isPRValueCategory()
+    then
+      // If this is C, then the result of an assignment is a prvalue for the new
+      // value assigned to the left operand. If this is C++, then the result is
+      // an lvalue, but that lvalue is being loaded as part of this expression.
+      // EDG doesn't mark this as a load.
+      result = getStoredValue()
+    else
+      // This is C++, where the result is an lvalue for the left operand,
+      // and that lvalue is not being loaded as part of this expression.
+      result = getUnloadedLeftOperand().getResult()
+  }
+
+  final TranslatedExpr getUnloadedLeftOperand() { result = getLoadedLeftOperand().getOperand() }
+
+  /**
+   * Gets the `TranslatedLoad` on the `e` in this `e += ...` which is the
+   * element that holds the value to be cremented. It's guaranteed that there's
+   * a load on `e` because of the `needsLoadForParentExpr` predicate.
+   */
+  final TranslatedLoad getLoadedLeftOperand() {
+    result = getTranslatedExpr(expr.getLValue().getFullyConverted())
+  }
+
+  /**
+   * Gets the address to which the result of this operation will be stored.
+   */
+  final TranslatedExpr getRightOperand() {
+    result = getTranslatedExpr(expr.getRValue().getFullyConverted())
+  }
+
   override Instruction getInstructionSuccessor(InstructionTag tag, EdgeKind kind) {
     kind instanceof GotoEdge and
     (
-      (
-        tag = AssignOperationLoadTag() and
-        if leftOperandNeedsConversion()
-        then result = getInstruction(AssignOperationConvertLeftTag())
-        else result = getInstruction(AssignOperationOpTag())
-      )
-      or
       tag = AssignOperationConvertLeftTag() and
       result = getInstruction(AssignOperationOpTag())
       or
@@ -1342,13 +1365,15 @@ class TranslatedAssignOperation extends TranslatedAssignment {
   override Instruction getChildSuccessor(TranslatedElement child) {
     // Operands are evaluated right-to-left.
     child = getRightOperand() and
-    result = getLeftOperand().getFirstInstruction()
+    result = getLoadedLeftOperand().getFirstInstruction()
     or
-    child = getLeftOperand() and
-    result = getInstruction(AssignOperationLoadTag())
+    child = getLoadedLeftOperand() and
+    if leftOperandNeedsConversion()
+    then result = getInstruction(AssignOperationConvertLeftTag())
+    else result = getInstruction(AssignOperationOpTag())
   }
 
-  override Instruction getStoredValue() {
+  private Instruction getStoredValue() {
     if leftOperandNeedsConversion()
     then result = getInstruction(AssignOperationConvertResultTag())
     else result = getInstruction(AssignOperationOpTag())
@@ -1368,7 +1393,7 @@ class TranslatedAssignOperation extends TranslatedAssignment {
       // anyway. If we really want to model this case perfectly, we'll need the
       // extractor to tell us what the promoted type of the left operand would
       // be.
-      result = getLeftOperand().getExpr().getType()
+      result = getLoadedLeftOperand().getExpr().getType()
     else
       // The right operand has already been converted to the type of the op.
       result = getRightOperand().getExpr().getType()
@@ -1376,7 +1401,7 @@ class TranslatedAssignOperation extends TranslatedAssignment {
 
   private predicate leftOperandNeedsConversion() {
     getConvertedLeftOperandType().getUnspecifiedType() !=
-      getLeftOperand().getExpr().getUnspecifiedType()
+      getLoadedLeftOperand().getExpr().getUnspecifiedType()
   }
 
   private Opcode getOpcode() {
@@ -1406,10 +1431,6 @@ class TranslatedAssignOperation extends TranslatedAssignment {
   }
 
   override predicate hasInstruction(Opcode opcode, InstructionTag tag, CppType resultType) {
-    tag = AssignOperationLoadTag() and
-    opcode instanceof Opcode::Load and
-    resultType = getTypeForPRValue(getLeftOperand().getExpr().getType())
-    or
     tag = AssignOperationOpTag() and
     opcode = getOpcode() and
     resultType = getTypeForPRValue(getConvertedLeftOperandType())
@@ -1425,7 +1446,7 @@ class TranslatedAssignOperation extends TranslatedAssignment {
       resultType = getTypeForPRValue(getConvertedLeftOperandType())
       or
       tag = AssignOperationConvertResultTag() and
-      resultType = getTypeForPRValue(getLeftOperand().getExpr().getType())
+      resultType = getTypeForPRValue(getLoadedLeftOperand().getExpr().getType())
     )
   }
 
@@ -1439,19 +1460,10 @@ class TranslatedAssignOperation extends TranslatedAssignment {
   }
 
   override Instruction getInstructionOperand(InstructionTag tag, OperandTag operandTag) {
-    tag = AssignOperationLoadTag() and
-    (
-      operandTag instanceof AddressOperandTag and
-      result = getLeftOperand().getResult()
-      or
-      operandTag instanceof LoadOperandTag and
-      result = getEnclosingFunction().getUnmodeledDefinitionInstruction()
-    )
-    or
     leftOperandNeedsConversion() and
     tag = AssignOperationConvertLeftTag() and
     operandTag instanceof UnaryOperandTag and
-    result = getInstruction(AssignOperationLoadTag())
+    result = getLoadedLeftOperand().getResult()
     or
     tag = AssignOperationOpTag() and
     (
@@ -1459,7 +1471,7 @@ class TranslatedAssignOperation extends TranslatedAssignment {
         operandTag instanceof LeftOperandTag and
         if leftOperandNeedsConversion()
         then result = getInstruction(AssignOperationConvertLeftTag())
-        else result = getInstruction(AssignOperationLoadTag())
+        else result = getLoadedLeftOperand().getResult()
       )
       or
       operandTag instanceof RightOperandTag and
@@ -1474,7 +1486,7 @@ class TranslatedAssignOperation extends TranslatedAssignment {
     tag = AssignmentStoreTag() and
     (
       operandTag instanceof AddressOperandTag and
-      result = getLeftOperand().getResult()
+      result = getUnloadedLeftOperand().getResult()
       or
       operandTag instanceof StoreValueOperandTag and
       result = getStoredValue()
@@ -2457,6 +2469,9 @@ predicate exprNeedsCopyIfNotLoaded(Expr expr) {
     expr instanceof PrefixCrementOperation and
     not expr.isPRValueCategory() // is C++
     or
+    // Because the load is on the `e` in `e++`.
+    expr instanceof PostfixCrementOperation
+    or
     expr instanceof PointerDereferenceExpr
     or
     expr instanceof AddressOfExpr

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

@@ -71,6 +71,9 @@ class TranslatedFunction extends TranslatedElement, TTranslatedFunction {
       result = getInstruction(AliasedDefinitionTag())
       or
       tag = AliasedDefinitionTag() and
+      result = getInstruction(InitializeNonLocalTag())
+      or
+      tag = InitializeNonLocalTag() and
       result = getInstruction(UnmodeledDefinitionTag())
       or
       (
@@ -144,6 +147,10 @@ class TranslatedFunction extends TranslatedElement, TTranslatedFunction {
       opcode instanceof Opcode::AliasedDefinition and
       resultType = getUnknownType()
       or
+      tag = InitializeNonLocalTag() and
+      opcode instanceof Opcode::InitializeNonLocal and
+      resultType = getUnknownType()
+      or
       tag = InitializeThisTag() and
       opcode instanceof Opcode::InitializeThis and
       resultType = getTypeForGLValue(getThisType())

cpp/ql/src/semmle/code/cpp/ir/implementation/unaliased_ssa/gvn/ValueNumbering.qll

@@ -52,6 +52,11 @@ newtype TValueNumber =
   ) {
     inheritanceConversionValueNumber(_, irFunc, opcode, baseClass, derivedClass, operand)
   } or
+  TLoadTotalOverlapValueNumber(
+    IRFunction irFunc, IRType type, ValueNumber memOperand, ValueNumber operand
+  ) {
+    loadTotalOverlapValueNumber(_, irFunc, type, memOperand, operand)
+  } or
   TUniqueValueNumber(IRFunction irFunc, Instruction instr) { uniqueValueNumber(instr, irFunc) }
 
 /**
@@ -101,12 +106,18 @@ class ValueNumber extends TValueNumber {
  * The use of `p.x` on line 3 is linked to the definition of `p` on line 1 as well, but is not
  * congruent to that definition because `p.x` accesses only a subset of the memory defined by `p`.
  */
-private class CongruentCopyInstruction extends CopyInstruction {
+class CongruentCopyInstruction extends CopyInstruction {
   CongruentCopyInstruction() {
     this.getSourceValueOperand().getDefinitionOverlap() instanceof MustExactlyOverlap
   }
 }
 
+class LoadTotalOverlapInstruction extends LoadInstruction {
+  LoadTotalOverlapInstruction() {
+    this.getSourceValueOperand().getDefinitionOverlap() instanceof MustTotallyOverlap
+  }
+}
+
 /**
  * Holds if this library knows how to assign a value number to the specified instruction, other than
  * a `unique` value number that is never shared by multiple instructions.
@@ -131,6 +142,8 @@ private predicate numberableInstruction(Instruction instr) {
   instr instanceof PointerArithmeticInstruction
   or
   instr instanceof CongruentCopyInstruction
+  or
+  instr instanceof LoadTotalOverlapInstruction
 }
 
 private predicate variableAddressValueNumber(
@@ -205,6 +218,7 @@ private predicate unaryValueNumber(
   instr.getEnclosingIRFunction() = irFunc and
   not instr instanceof InheritanceConversionInstruction and
   not instr instanceof CopyInstruction and
+  not instr instanceof FieldAddressInstruction and
   instr.getOpcode() = opcode and
   instr.getResultIRType() = type and
   valueNumber(instr.getUnary()) = operand
@@ -221,6 +235,16 @@ private predicate inheritanceConversionValueNumber(
   valueNumber(instr.getUnary()) = operand
 }
 
+private predicate loadTotalOverlapValueNumber(
+  LoadTotalOverlapInstruction instr, IRFunction irFunc, IRType type, ValueNumber memOperand,
+  ValueNumber operand
+) {
+  instr.getEnclosingIRFunction() = irFunc and
+  instr.getResultIRType() = type and
+  valueNumber(instr.getAnOperand().(MemoryOperand).getAnyDef()) = memOperand and
+  valueNumberOfOperand(instr.getAnOperand().(AddressOperand)) = operand
+}
+
 /**
  * Holds if `instr` should be assigned a unique value number because this library does not know how
  * to determine if two instances of that instruction are equivalent.
@@ -313,6 +337,11 @@ private ValueNumber nonUniqueValueNumber(Instruction instr) {
           TPointerArithmeticValueNumber(irFunc, opcode, type, elementSize, leftOperand, rightOperand)
       )
       or
+      exists(IRType type, ValueNumber memOperand, ValueNumber operand |
+        loadTotalOverlapValueNumber(instr, irFunc, type, memOperand, operand) and
+        result = TLoadTotalOverlapValueNumber(irFunc, type, memOperand, operand)
+      )
+      or
       // The value number of a copy is just the value number of its source value.
       result = valueNumber(instr.(CongruentCopyInstruction).getSourceValue())
     )

cpp/ql/src/semmle/code/cpp/ir/implementation/unaliased_ssa/internal/SSAConstruction.qll

@@ -759,7 +759,21 @@ module DefUse {
       then defLocation = useLocation
       else (
         definitionHasPhiNode(defLocation, block) and
-        defLocation = useLocation.getVirtualVariable()
+        defLocation = useLocation.getVirtualVariable() and
+        // Handle the unusual case where a virtual variable does not overlap one of its member
+        // locations. For example, a definition of the virtual variable representing all aliased
+        // memory does not overlap a use of a string literal, because the contents of a string
+        // literal can never be redefined. The string literal's location could still be a member of
+        // the `AliasedVirtualVariable` due to something like:
+        // ```
+        // char s[10];
+        // strcpy(s, p);
+        // const char* p = b ? "SomeLiteral" : s;
+        // return p[3];
+        // ```
+        // In the above example, `p[3]` may access either the string literal or the local variable
+        // `s`, so both of those locations must be members of the `AliasedVirtualVariable`.
+        exists(Alias::getOverlap(defLocation, useLocation))
       )
     )
     or

cpp/ql/src/semmle/code/cpp/rangeanalysis/RangeAnalysis.qll

@@ -187,6 +187,13 @@ private predicate boundFlowStepSsa(
     guard.controls(op2.getUse().getBlock(), testIsTrue) and
     reason = TCondReason(guard)
   )
+  or
+  exists(IRGuardCondition guard, boolean testIsTrue, SafeCastInstruction cast |
+    valueNumberOfOperand(op2) = valueNumber(cast.getUnary()) and
+    guard = boundFlowCond(valueNumber(cast), op1, delta, upper, testIsTrue) and
+    guard.controls(op2.getUse().getBlock(), testIsTrue) and
+    reason = TCondReason(guard)
+  )
 }
 
 /**
@@ -259,7 +266,7 @@ private predicate safeCast(IntegralType fromtyp, IntegralType totyp) {
 
 private class SafeCastInstruction extends ConvertInstruction {
   SafeCastInstruction() {
-    safeCast(getResultType(), getUnary().getResultType())
+    safeCast(getUnary().getResultType(), getResultType())
     or
     getResultType() instanceof PointerType and
     getUnary().getResultType() instanceof PointerType

cpp/ql/src/semmle/code/cpp/valuenumbering/GlobalValueNumbering.qll

@@ -1,608 +1 @@
-/**
- * Provides an implementation of Global Value Numbering.
- * See https://en.wikipedia.org/wiki/Global_value_numbering
- *
- * The predicate `globalValueNumber` converts an expression into a `GVN`,
- * which is an abstract type representing the value of the expression. If
- * two expressions have the same `GVN` then they compute the same value.
- * For example:
- *
- * ```
- * void f(int x, int y) {
- *   g(x+y, x+y);
- * }
- * ```
- *
- * In this example, both arguments in the call to `g` compute the same value,
- * so both arguments have the same `GVN`. In other words, we can find
- * this call with the following query:
- *
- * ```
- * from FunctionCall call, GVN v
- * where v = globalValueNumber(call.getArgument(0))
- *   and v = globalValueNumber(call.getArgument(1))
- * select call
- * ```
- *
- * The analysis is conservative, so two expressions might have different
- * `GVN`s even though the actually always compute the same value. The most
- * common reason for this is that the analysis cannot prove that there
- * are no side-effects that might cause the computed value to change.
- */
-
-/*
- * Note to developers: the correctness of this module depends on the
- * definitions of GVN, globalValueNumber, and analyzableExpr being kept in
- * sync with each other. If you change this module then make sure that the
- * change is symmetric across all three.
- */
-
-import cpp
-private import semmle.code.cpp.controlflow.SSA
-
-/**
- * Holds if the result is a control flow node that might change the
- * value of any global variable. This is used in the implementation
- * of `GVN_OtherVariable`, because we need to be quite conservative when
- * we assign a value number to a global variable. For example:
- *
- * ```
- * x = g+1;
- * dosomething();
- * y = g+1;
- * ```
- *
- * It is not safe to assign the same value number to both instances
- * of `g+1` in this example, because the call to `dosomething` might
- * change the value of `g`.
- */
-private ControlFlowNode nodeWithPossibleSideEffect() {
-  result instanceof Call
-  or
-  // If the lhs of an assignment is not analyzable by SSA, then
-  // we need to treat the assignment as having a possible side-effect.
-  result instanceof Assignment and not result instanceof SsaDefinition
-  or
-  result instanceof CrementOperation and not result instanceof SsaDefinition
-  or
-  exists(LocalVariable v |
-    result = v.getInitializer().getExpr() and not result instanceof SsaDefinition
-  )
-  or
-  result instanceof AsmStmt
-}
-
-/**
- * Gets the entry node of the control flow graph of which `node` is a
- * member.
- */
-cached
-private ControlFlowNode getControlFlowEntry(ControlFlowNode node) {
-  result = node.getControlFlowScope().getEntryPoint() and
-  result.getASuccessor*() = node
-}
-
-/**
- * Holds if there is a control flow edge from `src` to `dst` or
- * if `dst` is an expression with a possible side-effect. The idea
- * is to treat side effects as entry points in the control flow
- * graph so that we can use the dominator tree to find the most recent
- * side-effect.
- */
-private predicate sideEffectCFG(ControlFlowNode src, ControlFlowNode dst) {
-  src.getASuccessor() = dst
-  or
-  // Add an edge from the entry point to any node that might have a side
-  // effect.
-  dst = nodeWithPossibleSideEffect() and
-  src = getControlFlowEntry(dst)
-}
-
-/**
- * Holds if `dominator` is the immediate dominator of `node` in
- * the side-effect CFG.
- */
-private predicate iDomEffect(ControlFlowNode dominator, ControlFlowNode node) =
-  idominance(functionEntry/1, sideEffectCFG/2)(_, dominator, node)
-
-/**
- * Gets the most recent side effect. To be more precise, `result` is a
- * dominator of `node` and no side-effects can occur between `result` and
- * `node`.
- *
- * `sideEffectCFG` has an edge from the function entry to every node with a
- * side-effect. This means that every node with a side-effect has the
- * function entry as its immediate dominator. So if node `x` dominates node
- * `y` then there can be no side effects between `x` and `y` unless `x` is
- * the function entry. So the optimal choice for `result` has the function
- * entry as its immediate dominator.
- *
- * Example:
- *
- * ```
- * 000:  int f(int a, int b, int *p) {
- * 001:    int r = 0;
- * 002:    if (a) {
- * 003:      if (b) {
- * 004:        sideEffect1();
- * 005:      }
- * 006:    } else {
- * 007:      sideEffect2();
- * 008:    }
- * 009:    if (a) {
- * 010:      r++; // Not a side-effect, because r is an SSA variable.
- * 011:    }
- * 012:    if (b) {
- * 013:      r++; // Not a side-effect, because r is an SSA variable.
- * 014:    }
- * 015:    return *p;
- * 016:  }
- * ```
- *
- * Suppose we want to find the most recent side-effect for the dereference
- * of `p` on line 015. The `sideEffectCFG` has an edge from the function
- * entry (line 000) to the side effects at lines 004 and 007. Therefore,
- * the immediate dominator tree looks like this:
- *
- * 000 - 001 - 002 - 003
- *     - 004
- *     - 007
- *     - 009 - 010
- *           - 012 - 013
- *                 - 015
- *
- * The immediate dominator path to line 015 is 000 - 009 - 012 - 015.
- * Therefore, the most recent side effect for line 015 is line 009.
- */
-cached
-private ControlFlowNode mostRecentSideEffect(ControlFlowNode node) {
-  exists(ControlFlowNode entry |
-    functionEntry(entry) and
-    iDomEffect(entry, result) and
-    iDomEffect*(result, node)
-  )
-}
-
-/** Used to represent the "global value number" of an expression. */
-cached
-private newtype GVNBase =
-  GVN_IntConst(int val, Type t) { mk_IntConst(val, t, _) } or
-  GVN_FloatConst(float val, Type t) { mk_FloatConst(val, t, _) } or
-  // If the local variable does not have a defining value, then
-  // we use the SsaDefinition as its global value number.
-  GVN_UndefinedStackVariable(StackVariable x, SsaDefinition def) {
-    mk_UndefinedStackVariable(x, def, _)
-  } or
-  // Variables with no SSA information. As a crude (but safe)
-  // approximation, we use `mostRecentSideEffect` to compute a definition
-  // location for the variable. This ensures that two instances of the same
-  // global variable will only get the same value number if they are
-  // guaranteed to have the same value.
-  GVN_OtherVariable(Variable x, ControlFlowNode dominator) { mk_OtherVariable(x, dominator, _) } or
-  GVN_FieldAccess(GVN s, Field f) {
-    mk_DotFieldAccess(s, f, _) or
-    mk_PointerFieldAccess_with_deref(s, f, _) or
-    mk_ImplicitThisFieldAccess_with_deref(s, f, _)
-  } or
-  // Dereference a pointer. The value might have changed since the last
-  // time the pointer was dereferenced, so we need to include a definition
-  // location. As a crude (but safe) approximation, we use
-  // `mostRecentSideEffect` to compute a definition location.
-  GVN_Deref(GVN p, ControlFlowNode dominator) {
-    mk_Deref(p, dominator, _) or
-    mk_PointerFieldAccess(p, _, dominator, _) or
-    mk_ImplicitThisFieldAccess_with_qualifier(p, _, dominator, _)
-  } or
-  GVN_ThisExpr(Function fcn) {
-    mk_ThisExpr(fcn, _) or
-    mk_ImplicitThisFieldAccess(fcn, _, _, _)
-  } or
-  GVN_Conversion(Type t, GVN child) { mk_Conversion(t, child, _) } or
-  GVN_BinaryOp(GVN lhs, GVN rhs, string opname) { mk_BinaryOp(lhs, rhs, opname, _) } or
-  GVN_UnaryOp(GVN child, string opname) { mk_UnaryOp(child, opname, _) } or
-  GVN_ArrayAccess(GVN x, GVN i, ControlFlowNode dominator) { mk_ArrayAccess(x, i, dominator, _) } or
-  // Any expression that is not handled by the cases above is
-  // given a unique number based on the expression itself.
-  GVN_Unanalyzable(Expr e) { not analyzableExpr(e) }
-
-/**
- * A Global Value Number. A GVN is an abstract representation of the value
- * computed by an expression. The relationship between `Expr` and `GVN` is
- * many-to-one: every `Expr` has exactly one `GVN`, but multiple
- * expressions can have the same `GVN`. If two expressions have the same
- * `GVN`, it means that they compute the same value at run time. The `GVN`
- * is an opaque value, so you cannot deduce what the run-time value of an
- * expression will be from its `GVN`. The only use for the `GVN` of an
- * expression is to find other expressions that compute the same value.
- * Use the predicate `globalValueNumber` to get the `GVN` for an `Expr`.
- *
- * Note: `GVN` has `toString` and `getLocation` methods, so that it can be
- * displayed in a results list. These work by picking an arbitrary
- * expression with this `GVN` and using its `toString` and `getLocation`
- * methods.
- */
-class GVN extends GVNBase {
-  GVN() { this instanceof GVNBase }
-
-  /** Gets an expression that has this GVN. */
-  Expr getAnExpr() { this = globalValueNumber(result) }
-
-  /** Gets the kind of the GVN. This can be useful for debugging. */
-  string getKind() {
-    if this instanceof GVN_IntConst
-    then result = "IntConst"
-    else
-      if this instanceof GVN_FloatConst
-      then result = "FloatConst"
-      else
-        if this instanceof GVN_UndefinedStackVariable
-        then result = "UndefinedStackVariable"
-        else
-          if this instanceof GVN_OtherVariable
-          then result = "OtherVariable"
-          else
-            if this instanceof GVN_FieldAccess
-            then result = "FieldAccess"
-            else
-              if this instanceof GVN_Deref
-              then result = "Deref"
-              else
-                if this instanceof GVN_ThisExpr
-                then result = "ThisExpr"
-                else
-                  if this instanceof GVN_Conversion
-                  then result = "Conversion"
-                  else
-                    if this instanceof GVN_BinaryOp
-                    then result = "BinaryOp"
-                    else
-                      if this instanceof GVN_UnaryOp
-                      then result = "UnaryOp"
-                      else
-                        if this instanceof GVN_ArrayAccess
-                        then result = "ArrayAccess"
-                        else
-                          if this instanceof GVN_Unanalyzable
-                          then result = "Unanalyzable"
-                          else result = "error"
-  }
-
-  /**
-   * Gets an example of an expression with this GVN.
-   * This is useful for things like implementing toString().
-   */
-  private Expr exampleExpr() {
-    // Pick the expression with the minimum source location string. This is
-    // just an arbitrary way to pick an expression with this `GVN`.
-    result = min(Expr e | this = globalValueNumber(e) | e order by e.getLocation().toString())
-  }
-
-  /** Gets a textual representation of this element. */
-  string toString() { result = exampleExpr().toString() }
-
-  /** Gets the primary location of this element. */
-  Location getLocation() { result = exampleExpr().getLocation() }
-}
-
-private predicate analyzableIntConst(Expr e) {
-  strictcount(e.getValue().toInt()) = 1 and
-  strictcount(e.getUnspecifiedType()) = 1
-}
-
-private predicate mk_IntConst(int val, Type t, Expr e) {
-  analyzableIntConst(e) and
-  val = e.getValue().toInt() and
-  t = e.getUnspecifiedType()
-}
-
-private predicate analyzableFloatConst(Expr e) {
-  strictcount(e.getValue().toFloat()) = 1 and
-  strictcount(e.getUnspecifiedType()) = 1 and
-  not analyzableIntConst(e)
-}
-
-private predicate mk_FloatConst(float val, Type t, Expr e) {
-  analyzableFloatConst(e) and
-  val = e.getValue().toFloat() and
-  t = e.getUnspecifiedType()
-}
-
-private predicate analyzableStackVariable(VariableAccess access) {
-  strictcount(SsaDefinition def | def.getAUse(_) = access | def) = 1 and
-  strictcount(SsaDefinition def, Variable v | def.getAUse(v) = access | v) = 1 and
-  count(SsaDefinition def, Variable v |
-    def.getAUse(v) = access
-  |
-    def.getDefiningValue(v).getFullyConverted()
-  ) <= 1 and
-  not analyzableConst(access)
-}
-
-// Note: this predicate only has a result if the access has no
-// defining value. If there is a defining value, then there is no
-// need to generate a fresh `GVN` for the access because `globalValueNumber`
-// will follow the chain and use the GVN of the defining value.
-private predicate mk_UndefinedStackVariable(
-  StackVariable x, SsaDefinition def, VariableAccess access
-) {
-  analyzableStackVariable(access) and
-  access = def.getAUse(x) and
-  not exists(def.getDefiningValue(x))
-}
-
-private predicate analyzableDotFieldAccess(DotFieldAccess access) {
-  strictcount(access.getTarget()) = 1 and
-  strictcount(access.getQualifier().getFullyConverted()) = 1 and
-  not analyzableConst(access)
-}
-
-private predicate mk_DotFieldAccess(GVN qualifier, Field target, DotFieldAccess access) {
-  analyzableDotFieldAccess(access) and
-  target = access.getTarget() and
-  qualifier = globalValueNumber(access.getQualifier().getFullyConverted())
-}
-
-private predicate analyzablePointerFieldAccess(PointerFieldAccess access) {
-  strictcount(mostRecentSideEffect(access)) = 1 and
-  strictcount(access.getTarget()) = 1 and
-  strictcount(access.getQualifier().getFullyConverted()) = 1 and
-  not analyzableConst(access)
-}
-
-private predicate mk_PointerFieldAccess(
-  GVN qualifier, Field target, ControlFlowNode dominator, PointerFieldAccess access
-) {
-  analyzablePointerFieldAccess(access) and
-  dominator = mostRecentSideEffect(access) and
-  target = access.getTarget() and
-  qualifier = globalValueNumber(access.getQualifier().getFullyConverted())
-}
-
-/**
- * `obj->field` is equivalent to `(*obj).field`, so we need to wrap an
- * extra `GVN_Deref` around the qualifier.
- */
-private predicate mk_PointerFieldAccess_with_deref(
-  GVN new_qualifier, Field target, PointerFieldAccess access
-) {
-  exists(GVN qualifier, ControlFlowNode dominator |
-    mk_PointerFieldAccess(qualifier, target, dominator, access) and
-    new_qualifier = GVN_Deref(qualifier, dominator)
-  )
-}
-
-private predicate analyzableImplicitThisFieldAccess(ImplicitThisFieldAccess access) {
-  strictcount(mostRecentSideEffect(access)) = 1 and
-  strictcount(access.getTarget()) = 1 and
-  strictcount(access.getEnclosingFunction()) = 1 and
-  not analyzableConst(access)
-}
-
-private predicate mk_ImplicitThisFieldAccess(
-  Function fcn, Field target, ControlFlowNode dominator, ImplicitThisFieldAccess access
-) {
-  analyzableImplicitThisFieldAccess(access) and
-  dominator = mostRecentSideEffect(access) and
-  target = access.getTarget() and
-  fcn = access.getEnclosingFunction()
-}
-
-private predicate mk_ImplicitThisFieldAccess_with_qualifier(
-  GVN qualifier, Field target, ControlFlowNode dominator, ImplicitThisFieldAccess access
-) {
-  exists(Function fcn |
-    mk_ImplicitThisFieldAccess(fcn, target, dominator, access) and
-    qualifier = GVN_ThisExpr(fcn)
-  )
-}
-
-private predicate mk_ImplicitThisFieldAccess_with_deref(
-  GVN new_qualifier, Field target, ImplicitThisFieldAccess access
-) {
-  exists(GVN qualifier, ControlFlowNode dominator |
-    mk_ImplicitThisFieldAccess_with_qualifier(qualifier, target, dominator, access) and
-    new_qualifier = GVN_Deref(qualifier, dominator)
-  )
-}
-
-/**
- * Holds if `access` is an access of a variable that does
- * not have SSA information. (For example, because the variable
- * is global.)
- */
-private predicate analyzableOtherVariable(VariableAccess access) {
-  not access instanceof FieldAccess and
-  not exists(SsaDefinition def | access = def.getAUse(_)) and
-  strictcount(access.getTarget()) = 1 and
-  strictcount(mostRecentSideEffect(access)) = 1 and
-  not analyzableConst(access)
-}
-
-private predicate mk_OtherVariable(Variable x, ControlFlowNode dominator, VariableAccess access) {
-  analyzableOtherVariable(access) and
-  x = access.getTarget() and
-  dominator = mostRecentSideEffect(access)
-}
-
-private predicate analyzableConversion(Conversion conv) {
-  strictcount(conv.getUnspecifiedType()) = 1 and
-  strictcount(conv.getExpr()) = 1 and
-  not analyzableConst(conv)
-}
-
-private predicate mk_Conversion(Type t, GVN child, Conversion conv) {
-  analyzableConversion(conv) and
-  t = conv.getUnspecifiedType() and
-  child = globalValueNumber(conv.getExpr())
-}
-
-private predicate analyzableBinaryOp(BinaryOperation op) {
-  op.isPure() and
-  strictcount(op.getLeftOperand().getFullyConverted()) = 1 and
-  strictcount(op.getRightOperand().getFullyConverted()) = 1 and
-  strictcount(op.getOperator()) = 1 and
-  not analyzableConst(op)
-}
-
-private predicate mk_BinaryOp(GVN lhs, GVN rhs, string opname, BinaryOperation op) {
-  analyzableBinaryOp(op) and
-  lhs = globalValueNumber(op.getLeftOperand().getFullyConverted()) and
-  rhs = globalValueNumber(op.getRightOperand().getFullyConverted()) and
-  opname = op.getOperator()
-}
-
-private predicate analyzableUnaryOp(UnaryOperation op) {
-  not op instanceof PointerDereferenceExpr and
-  op.isPure() and
-  strictcount(op.getOperand().getFullyConverted()) = 1 and
-  strictcount(op.getOperator()) = 1 and
-  not analyzableConst(op)
-}
-
-private predicate mk_UnaryOp(GVN child, string opname, UnaryOperation op) {
-  analyzableUnaryOp(op) and
-  child = globalValueNumber(op.getOperand().getFullyConverted()) and
-  opname = op.getOperator()
-}
-
-private predicate analyzableThisExpr(ThisExpr thisExpr) {
-  strictcount(thisExpr.getEnclosingFunction()) = 1 and
-  not analyzableConst(thisExpr)
-}
-
-private predicate mk_ThisExpr(Function fcn, ThisExpr thisExpr) {
-  analyzableThisExpr(thisExpr) and
-  fcn = thisExpr.getEnclosingFunction()
-}
-
-private predicate analyzableArrayAccess(ArrayExpr ae) {
-  strictcount(ae.getArrayBase().getFullyConverted()) = 1 and
-  strictcount(ae.getArrayOffset().getFullyConverted()) = 1 and
-  strictcount(mostRecentSideEffect(ae)) = 1 and
-  not analyzableConst(ae)
-}
-
-private predicate mk_ArrayAccess(GVN base, GVN offset, ControlFlowNode dominator, ArrayExpr ae) {
-  analyzableArrayAccess(ae) and
-  base = globalValueNumber(ae.getArrayBase().getFullyConverted()) and
-  offset = globalValueNumber(ae.getArrayOffset().getFullyConverted()) and
-  dominator = mostRecentSideEffect(ae)
-}
-
-private predicate analyzablePointerDereferenceExpr(PointerDereferenceExpr deref) {
-  strictcount(deref.getOperand().getFullyConverted()) = 1 and
-  strictcount(mostRecentSideEffect(deref)) = 1 and
-  not analyzableConst(deref)
-}
-
-private predicate mk_Deref(GVN p, ControlFlowNode dominator, PointerDereferenceExpr deref) {
-  analyzablePointerDereferenceExpr(deref) and
-  p = globalValueNumber(deref.getOperand().getFullyConverted()) and
-  dominator = mostRecentSideEffect(deref)
-}
-
-/** Gets the global value number of expression `e`. */
-cached
-GVN globalValueNumber(Expr e) {
-  exists(int val, Type t |
-    mk_IntConst(val, t, e) and
-    result = GVN_IntConst(val, t)
-  )
-  or
-  exists(float val, Type t |
-    mk_FloatConst(val, t, e) and
-    result = GVN_FloatConst(val, t)
-  )
-  or
-  // Local variable with a defining value.
-  exists(StackVariable x, SsaDefinition def |
-    analyzableStackVariable(e) and
-    e = def.getAUse(x) and
-    result = globalValueNumber(def.getDefiningValue(x).getFullyConverted())
-  )
-  or
-  // Local variable without a defining value.
-  exists(StackVariable x, SsaDefinition def |
-    mk_UndefinedStackVariable(x, def, e) and
-    result = GVN_UndefinedStackVariable(x, def)
-  )
-  or
-  // Variable with no SSA information.
-  exists(Variable x, ControlFlowNode dominator |
-    mk_OtherVariable(x, dominator, e) and
-    result = GVN_OtherVariable(x, dominator)
-  )
-  or
-  exists(GVN qualifier, Field target |
-    mk_DotFieldAccess(qualifier, target, e) and
-    result = GVN_FieldAccess(qualifier, target)
-  )
-  or
-  exists(GVN qualifier, Field target |
-    mk_PointerFieldAccess_with_deref(qualifier, target, e) and
-    result = GVN_FieldAccess(qualifier, target)
-  )
-  or
-  exists(GVN qualifier, Field target |
-    mk_ImplicitThisFieldAccess_with_deref(qualifier, target, e) and
-    result = GVN_FieldAccess(qualifier, target)
-  )
-  or
-  exists(Function fcn |
-    mk_ThisExpr(fcn, e) and
-    result = GVN_ThisExpr(fcn)
-  )
-  or
-  exists(Type t, GVN child |
-    mk_Conversion(t, child, e) and
-    result = GVN_Conversion(t, child)
-  )
-  or
-  exists(GVN lhs, GVN rhs, string opname |
-    mk_BinaryOp(lhs, rhs, opname, e) and
-    result = GVN_BinaryOp(lhs, rhs, opname)
-  )
-  or
-  exists(GVN child, string opname |
-    mk_UnaryOp(child, opname, e) and
-    result = GVN_UnaryOp(child, opname)
-  )
-  or
-  exists(GVN x, GVN i, ControlFlowNode dominator |
-    mk_ArrayAccess(x, i, dominator, e) and
-    result = GVN_ArrayAccess(x, i, dominator)
-  )
-  or
-  exists(GVN p, ControlFlowNode dominator |
-    mk_Deref(p, dominator, e) and
-    result = GVN_Deref(p, dominator)
-  )
-  or
-  not analyzableExpr(e) and result = GVN_Unanalyzable(e)
-}
-
-private predicate analyzableConst(Expr e) {
-  analyzableIntConst(e) or
-  analyzableFloatConst(e)
-}
-
-/**
- * Holds if the expression is explicitly handled by `globalValueNumber`.
- * Unanalyzable expressions still need to be given a global value number,
- * but it will be a unique number that is not shared with any other
- * expression.
- */
-private predicate analyzableExpr(Expr e) {
-  analyzableConst(e) or
-  analyzableStackVariable(e) or
-  analyzableDotFieldAccess(e) or
-  analyzablePointerFieldAccess(e) or
-  analyzableImplicitThisFieldAccess(e) or
-  analyzableOtherVariable(e) or
-  analyzableConversion(e) or
-  analyzableBinaryOp(e) or
-  analyzableUnaryOp(e) or
-  analyzableThisExpr(e) or
-  analyzableArrayAccess(e) or
-  analyzablePointerDereferenceExpr(e)
-}
+import GlobalValueNumberingImpl

cpp/ql/src/semmle/code/cpp/valuenumbering/GlobalValueNumberingImpl.qll

@@ -0,0 +1,608 @@
+/**
+ * Provides an implementation of Global Value Numbering.
+ * See https://en.wikipedia.org/wiki/Global_value_numbering
+ *
+ * The predicate `globalValueNumber` converts an expression into a `GVN`,
+ * which is an abstract type representing the value of the expression. If
+ * two expressions have the same `GVN` then they compute the same value.
+ * For example:
+ *
+ * ```
+ * void f(int x, int y) {
+ *   g(x+y, x+y);
+ * }
+ * ```
+ *
+ * In this example, both arguments in the call to `g` compute the same value,
+ * so both arguments have the same `GVN`. In other words, we can find
+ * this call with the following query:
+ *
+ * ```
+ * from FunctionCall call, GVN v
+ * where v = globalValueNumber(call.getArgument(0))
+ *   and v = globalValueNumber(call.getArgument(1))
+ * select call
+ * ```
+ *
+ * The analysis is conservative, so two expressions might have different
+ * `GVN`s even though the actually always compute the same value. The most
+ * common reason for this is that the analysis cannot prove that there
+ * are no side-effects that might cause the computed value to change.
+ */
+
+/*
+ * Note to developers: the correctness of this module depends on the
+ * definitions of GVN, globalValueNumber, and analyzableExpr being kept in
+ * sync with each other. If you change this module then make sure that the
+ * change is symmetric across all three.
+ */
+
+import cpp
+private import semmle.code.cpp.controlflow.SSA
+
+/**
+ * Holds if the result is a control flow node that might change the
+ * value of any global variable. This is used in the implementation
+ * of `GVN_OtherVariable`, because we need to be quite conservative when
+ * we assign a value number to a global variable. For example:
+ *
+ * ```
+ * x = g+1;
+ * dosomething();
+ * y = g+1;
+ * ```
+ *
+ * It is not safe to assign the same value number to both instances
+ * of `g+1` in this example, because the call to `dosomething` might
+ * change the value of `g`.
+ */
+private ControlFlowNode nodeWithPossibleSideEffect() {
+  result instanceof Call
+  or
+  // If the lhs of an assignment is not analyzable by SSA, then
+  // we need to treat the assignment as having a possible side-effect.
+  result instanceof Assignment and not result instanceof SsaDefinition
+  or
+  result instanceof CrementOperation and not result instanceof SsaDefinition
+  or
+  exists(LocalVariable v |
+    result = v.getInitializer().getExpr() and not result instanceof SsaDefinition
+  )
+  or
+  result instanceof AsmStmt
+}
+
+/**
+ * Gets the entry node of the control flow graph of which `node` is a
+ * member.
+ */
+cached
+private ControlFlowNode getControlFlowEntry(ControlFlowNode node) {
+  result = node.getControlFlowScope().getEntryPoint() and
+  result.getASuccessor*() = node
+}
+
+/**
+ * Holds if there is a control flow edge from `src` to `dst` or
+ * if `dst` is an expression with a possible side-effect. The idea
+ * is to treat side effects as entry points in the control flow
+ * graph so that we can use the dominator tree to find the most recent
+ * side-effect.
+ */
+private predicate sideEffectCFG(ControlFlowNode src, ControlFlowNode dst) {
+  src.getASuccessor() = dst
+  or
+  // Add an edge from the entry point to any node that might have a side
+  // effect.
+  dst = nodeWithPossibleSideEffect() and
+  src = getControlFlowEntry(dst)
+}
+
+/**
+ * Holds if `dominator` is the immediate dominator of `node` in
+ * the side-effect CFG.
+ */
+private predicate iDomEffect(ControlFlowNode dominator, ControlFlowNode node) =
+  idominance(functionEntry/1, sideEffectCFG/2)(_, dominator, node)
+
+/**
+ * Gets the most recent side effect. To be more precise, `result` is a
+ * dominator of `node` and no side-effects can occur between `result` and
+ * `node`.
+ *
+ * `sideEffectCFG` has an edge from the function entry to every node with a
+ * side-effect. This means that every node with a side-effect has the
+ * function entry as its immediate dominator. So if node `x` dominates node
+ * `y` then there can be no side effects between `x` and `y` unless `x` is
+ * the function entry. So the optimal choice for `result` has the function
+ * entry as its immediate dominator.
+ *
+ * Example:
+ *
+ * ```
+ * 000:  int f(int a, int b, int *p) {
+ * 001:    int r = 0;
+ * 002:    if (a) {
+ * 003:      if (b) {
+ * 004:        sideEffect1();
+ * 005:      }
+ * 006:    } else {
+ * 007:      sideEffect2();
+ * 008:    }
+ * 009:    if (a) {
+ * 010:      r++; // Not a side-effect, because r is an SSA variable.
+ * 011:    }
+ * 012:    if (b) {
+ * 013:      r++; // Not a side-effect, because r is an SSA variable.
+ * 014:    }
+ * 015:    return *p;
+ * 016:  }
+ * ```
+ *
+ * Suppose we want to find the most recent side-effect for the dereference
+ * of `p` on line 015. The `sideEffectCFG` has an edge from the function
+ * entry (line 000) to the side effects at lines 004 and 007. Therefore,
+ * the immediate dominator tree looks like this:
+ *
+ * 000 - 001 - 002 - 003
+ *     - 004
+ *     - 007
+ *     - 009 - 010
+ *           - 012 - 013
+ *                 - 015
+ *
+ * The immediate dominator path to line 015 is 000 - 009 - 012 - 015.
+ * Therefore, the most recent side effect for line 015 is line 009.
+ */
+cached
+private ControlFlowNode mostRecentSideEffect(ControlFlowNode node) {
+  exists(ControlFlowNode entry |
+    functionEntry(entry) and
+    iDomEffect(entry, result) and
+    iDomEffect*(result, node)
+  )
+}
+
+/** Used to represent the "global value number" of an expression. */
+cached
+private newtype GVNBase =
+  GVN_IntConst(int val, Type t) { mk_IntConst(val, t, _) } or
+  GVN_FloatConst(float val, Type t) { mk_FloatConst(val, t, _) } or
+  // If the local variable does not have a defining value, then
+  // we use the SsaDefinition as its global value number.
+  GVN_UndefinedStackVariable(StackVariable x, SsaDefinition def) {
+    mk_UndefinedStackVariable(x, def, _)
+  } or
+  // Variables with no SSA information. As a crude (but safe)
+  // approximation, we use `mostRecentSideEffect` to compute a definition
+  // location for the variable. This ensures that two instances of the same
+  // global variable will only get the same value number if they are
+  // guaranteed to have the same value.
+  GVN_OtherVariable(Variable x, ControlFlowNode dominator) { mk_OtherVariable(x, dominator, _) } or
+  GVN_FieldAccess(GVN s, Field f) {
+    mk_DotFieldAccess(s, f, _) or
+    mk_PointerFieldAccess_with_deref(s, f, _) or
+    mk_ImplicitThisFieldAccess_with_deref(s, f, _)
+  } or
+  // Dereference a pointer. The value might have changed since the last
+  // time the pointer was dereferenced, so we need to include a definition
+  // location. As a crude (but safe) approximation, we use
+  // `mostRecentSideEffect` to compute a definition location.
+  GVN_Deref(GVN p, ControlFlowNode dominator) {
+    mk_Deref(p, dominator, _) or
+    mk_PointerFieldAccess(p, _, dominator, _) or
+    mk_ImplicitThisFieldAccess_with_qualifier(p, _, dominator, _)
+  } or
+  GVN_ThisExpr(Function fcn) {
+    mk_ThisExpr(fcn, _) or
+    mk_ImplicitThisFieldAccess(fcn, _, _, _)
+  } or
+  GVN_Conversion(Type t, GVN child) { mk_Conversion(t, child, _) } or
+  GVN_BinaryOp(GVN lhs, GVN rhs, string opname) { mk_BinaryOp(lhs, rhs, opname, _) } or
+  GVN_UnaryOp(GVN child, string opname) { mk_UnaryOp(child, opname, _) } or
+  GVN_ArrayAccess(GVN x, GVN i, ControlFlowNode dominator) { mk_ArrayAccess(x, i, dominator, _) } or
+  // Any expression that is not handled by the cases above is
+  // given a unique number based on the expression itself.
+  GVN_Unanalyzable(Expr e) { not analyzableExpr(e) }
+
+/**
+ * A Global Value Number. A GVN is an abstract representation of the value
+ * computed by an expression. The relationship between `Expr` and `GVN` is
+ * many-to-one: every `Expr` has exactly one `GVN`, but multiple
+ * expressions can have the same `GVN`. If two expressions have the same
+ * `GVN`, it means that they compute the same value at run time. The `GVN`
+ * is an opaque value, so you cannot deduce what the run-time value of an
+ * expression will be from its `GVN`. The only use for the `GVN` of an
+ * expression is to find other expressions that compute the same value.
+ * Use the predicate `globalValueNumber` to get the `GVN` for an `Expr`.
+ *
+ * Note: `GVN` has `toString` and `getLocation` methods, so that it can be
+ * displayed in a results list. These work by picking an arbitrary
+ * expression with this `GVN` and using its `toString` and `getLocation`
+ * methods.
+ */
+class GVN extends GVNBase {
+  GVN() { this instanceof GVNBase }
+
+  /** Gets an expression that has this GVN. */
+  Expr getAnExpr() { this = globalValueNumber(result) }
+
+  /** Gets the kind of the GVN. This can be useful for debugging. */
+  string getKind() {
+    if this instanceof GVN_IntConst
+    then result = "IntConst"
+    else
+      if this instanceof GVN_FloatConst
+      then result = "FloatConst"
+      else
+        if this instanceof GVN_UndefinedStackVariable
+        then result = "UndefinedStackVariable"
+        else
+          if this instanceof GVN_OtherVariable
+          then result = "OtherVariable"
+          else
+            if this instanceof GVN_FieldAccess
+            then result = "FieldAccess"
+            else
+              if this instanceof GVN_Deref
+              then result = "Deref"
+              else
+                if this instanceof GVN_ThisExpr
+                then result = "ThisExpr"
+                else
+                  if this instanceof GVN_Conversion
+                  then result = "Conversion"
+                  else
+                    if this instanceof GVN_BinaryOp
+                    then result = "BinaryOp"
+                    else
+                      if this instanceof GVN_UnaryOp
+                      then result = "UnaryOp"
+                      else
+                        if this instanceof GVN_ArrayAccess
+                        then result = "ArrayAccess"
+                        else
+                          if this instanceof GVN_Unanalyzable
+                          then result = "Unanalyzable"
+                          else result = "error"
+  }
+
+  /**
+   * Gets an example of an expression with this GVN.
+   * This is useful for things like implementing toString().
+   */
+  private Expr exampleExpr() {
+    // Pick the expression with the minimum source location string. This is
+    // just an arbitrary way to pick an expression with this `GVN`.
+    result = min(Expr e | this = globalValueNumber(e) | e order by e.getLocation().toString())
+  }
+
+  /** Gets a textual representation of this element. */
+  string toString() { result = exampleExpr().toString() }
+
+  /** Gets the primary location of this element. */
+  Location getLocation() { result = exampleExpr().getLocation() }
+}
+
+private predicate analyzableIntConst(Expr e) {
+  strictcount(e.getValue().toInt()) = 1 and
+  strictcount(e.getUnspecifiedType()) = 1
+}
+
+private predicate mk_IntConst(int val, Type t, Expr e) {
+  analyzableIntConst(e) and
+  val = e.getValue().toInt() and
+  t = e.getUnspecifiedType()
+}
+
+private predicate analyzableFloatConst(Expr e) {
+  strictcount(e.getValue().toFloat()) = 1 and
+  strictcount(e.getUnspecifiedType()) = 1 and
+  not analyzableIntConst(e)
+}
+
+private predicate mk_FloatConst(float val, Type t, Expr e) {
+  analyzableFloatConst(e) and
+  val = e.getValue().toFloat() and
+  t = e.getUnspecifiedType()
+}
+
+private predicate analyzableStackVariable(VariableAccess access) {
+  strictcount(SsaDefinition def | def.getAUse(_) = access | def) = 1 and
+  strictcount(SsaDefinition def, Variable v | def.getAUse(v) = access | v) = 1 and
+  count(SsaDefinition def, Variable v |
+    def.getAUse(v) = access
+  |
+    def.getDefiningValue(v).getFullyConverted()
+  ) <= 1 and
+  not analyzableConst(access)
+}
+
+// Note: this predicate only has a result if the access has no
+// defining value. If there is a defining value, then there is no
+// need to generate a fresh `GVN` for the access because `globalValueNumber`
+// will follow the chain and use the GVN of the defining value.
+private predicate mk_UndefinedStackVariable(
+  StackVariable x, SsaDefinition def, VariableAccess access
+) {
+  analyzableStackVariable(access) and
+  access = def.getAUse(x) and
+  not exists(def.getDefiningValue(x))
+}
+
+private predicate analyzableDotFieldAccess(DotFieldAccess access) {
+  strictcount(access.getTarget()) = 1 and
+  strictcount(access.getQualifier().getFullyConverted()) = 1 and
+  not analyzableConst(access)
+}
+
+private predicate mk_DotFieldAccess(GVN qualifier, Field target, DotFieldAccess access) {
+  analyzableDotFieldAccess(access) and
+  target = access.getTarget() and
+  qualifier = globalValueNumber(access.getQualifier().getFullyConverted())
+}
+
+private predicate analyzablePointerFieldAccess(PointerFieldAccess access) {
+  strictcount(mostRecentSideEffect(access)) = 1 and
+  strictcount(access.getTarget()) = 1 and
+  strictcount(access.getQualifier().getFullyConverted()) = 1 and
+  not analyzableConst(access)
+}
+
+private predicate mk_PointerFieldAccess(
+  GVN qualifier, Field target, ControlFlowNode dominator, PointerFieldAccess access
+) {
+  analyzablePointerFieldAccess(access) and
+  dominator = mostRecentSideEffect(access) and
+  target = access.getTarget() and
+  qualifier = globalValueNumber(access.getQualifier().getFullyConverted())
+}
+
+/**
+ * `obj->field` is equivalent to `(*obj).field`, so we need to wrap an
+ * extra `GVN_Deref` around the qualifier.
+ */
+private predicate mk_PointerFieldAccess_with_deref(
+  GVN new_qualifier, Field target, PointerFieldAccess access
+) {
+  exists(GVN qualifier, ControlFlowNode dominator |
+    mk_PointerFieldAccess(qualifier, target, dominator, access) and
+    new_qualifier = GVN_Deref(qualifier, dominator)
+  )
+}
+
+private predicate analyzableImplicitThisFieldAccess(ImplicitThisFieldAccess access) {
+  strictcount(mostRecentSideEffect(access)) = 1 and
+  strictcount(access.getTarget()) = 1 and
+  strictcount(access.getEnclosingFunction()) = 1 and
+  not analyzableConst(access)
+}
+
+private predicate mk_ImplicitThisFieldAccess(
+  Function fcn, Field target, ControlFlowNode dominator, ImplicitThisFieldAccess access
+) {
+  analyzableImplicitThisFieldAccess(access) and
+  dominator = mostRecentSideEffect(access) and
+  target = access.getTarget() and
+  fcn = access.getEnclosingFunction()
+}
+
+private predicate mk_ImplicitThisFieldAccess_with_qualifier(
+  GVN qualifier, Field target, ControlFlowNode dominator, ImplicitThisFieldAccess access
+) {
+  exists(Function fcn |
+    mk_ImplicitThisFieldAccess(fcn, target, dominator, access) and
+    qualifier = GVN_ThisExpr(fcn)
+  )
+}
+
+private predicate mk_ImplicitThisFieldAccess_with_deref(
+  GVN new_qualifier, Field target, ImplicitThisFieldAccess access
+) {
+  exists(GVN qualifier, ControlFlowNode dominator |
+    mk_ImplicitThisFieldAccess_with_qualifier(qualifier, target, dominator, access) and
+    new_qualifier = GVN_Deref(qualifier, dominator)
+  )
+}
+
+/**
+ * Holds if `access` is an access of a variable that does
+ * not have SSA information. (For example, because the variable
+ * is global.)
+ */
+private predicate analyzableOtherVariable(VariableAccess access) {
+  not access instanceof FieldAccess and
+  not exists(SsaDefinition def | access = def.getAUse(_)) and
+  strictcount(access.getTarget()) = 1 and
+  strictcount(mostRecentSideEffect(access)) = 1 and
+  not analyzableConst(access)
+}
+
+private predicate mk_OtherVariable(Variable x, ControlFlowNode dominator, VariableAccess access) {
+  analyzableOtherVariable(access) and
+  x = access.getTarget() and
+  dominator = mostRecentSideEffect(access)
+}
+
+private predicate analyzableConversion(Conversion conv) {
+  strictcount(conv.getUnspecifiedType()) = 1 and
+  strictcount(conv.getExpr()) = 1 and
+  not analyzableConst(conv)
+}
+
+private predicate mk_Conversion(Type t, GVN child, Conversion conv) {
+  analyzableConversion(conv) and
+  t = conv.getUnspecifiedType() and
+  child = globalValueNumber(conv.getExpr())
+}
+
+private predicate analyzableBinaryOp(BinaryOperation op) {
+  op.isPure() and
+  strictcount(op.getLeftOperand().getFullyConverted()) = 1 and
+  strictcount(op.getRightOperand().getFullyConverted()) = 1 and
+  strictcount(op.getOperator()) = 1 and
+  not analyzableConst(op)
+}
+
+private predicate mk_BinaryOp(GVN lhs, GVN rhs, string opname, BinaryOperation op) {
+  analyzableBinaryOp(op) and
+  lhs = globalValueNumber(op.getLeftOperand().getFullyConverted()) and
+  rhs = globalValueNumber(op.getRightOperand().getFullyConverted()) and
+  opname = op.getOperator()
+}
+
+private predicate analyzableUnaryOp(UnaryOperation op) {
+  not op instanceof PointerDereferenceExpr and
+  op.isPure() and
+  strictcount(op.getOperand().getFullyConverted()) = 1 and
+  strictcount(op.getOperator()) = 1 and
+  not analyzableConst(op)
+}
+
+private predicate mk_UnaryOp(GVN child, string opname, UnaryOperation op) {
+  analyzableUnaryOp(op) and
+  child = globalValueNumber(op.getOperand().getFullyConverted()) and
+  opname = op.getOperator()
+}
+
+private predicate analyzableThisExpr(ThisExpr thisExpr) {
+  strictcount(thisExpr.getEnclosingFunction()) = 1 and
+  not analyzableConst(thisExpr)
+}
+
+private predicate mk_ThisExpr(Function fcn, ThisExpr thisExpr) {
+  analyzableThisExpr(thisExpr) and
+  fcn = thisExpr.getEnclosingFunction()
+}
+
+private predicate analyzableArrayAccess(ArrayExpr ae) {
+  strictcount(ae.getArrayBase().getFullyConverted()) = 1 and
+  strictcount(ae.getArrayOffset().getFullyConverted()) = 1 and
+  strictcount(mostRecentSideEffect(ae)) = 1 and
+  not analyzableConst(ae)
+}
+
+private predicate mk_ArrayAccess(GVN base, GVN offset, ControlFlowNode dominator, ArrayExpr ae) {
+  analyzableArrayAccess(ae) and
+  base = globalValueNumber(ae.getArrayBase().getFullyConverted()) and
+  offset = globalValueNumber(ae.getArrayOffset().getFullyConverted()) and
+  dominator = mostRecentSideEffect(ae)
+}
+
+private predicate analyzablePointerDereferenceExpr(PointerDereferenceExpr deref) {
+  strictcount(deref.getOperand().getFullyConverted()) = 1 and
+  strictcount(mostRecentSideEffect(deref)) = 1 and
+  not analyzableConst(deref)
+}
+
+private predicate mk_Deref(GVN p, ControlFlowNode dominator, PointerDereferenceExpr deref) {
+  analyzablePointerDereferenceExpr(deref) and
+  p = globalValueNumber(deref.getOperand().getFullyConverted()) and
+  dominator = mostRecentSideEffect(deref)
+}
+
+/** Gets the global value number of expression `e`. */
+cached
+GVN globalValueNumber(Expr e) {
+  exists(int val, Type t |
+    mk_IntConst(val, t, e) and
+    result = GVN_IntConst(val, t)
+  )
+  or
+  exists(float val, Type t |
+    mk_FloatConst(val, t, e) and
+    result = GVN_FloatConst(val, t)
+  )
+  or
+  // Local variable with a defining value.
+  exists(StackVariable x, SsaDefinition def |
+    analyzableStackVariable(e) and
+    e = def.getAUse(x) and
+    result = globalValueNumber(def.getDefiningValue(x).getFullyConverted())
+  )
+  or
+  // Local variable without a defining value.
+  exists(StackVariable x, SsaDefinition def |
+    mk_UndefinedStackVariable(x, def, e) and
+    result = GVN_UndefinedStackVariable(x, def)
+  )
+  or
+  // Variable with no SSA information.
+  exists(Variable x, ControlFlowNode dominator |
+    mk_OtherVariable(x, dominator, e) and
+    result = GVN_OtherVariable(x, dominator)
+  )
+  or
+  exists(GVN qualifier, Field target |
+    mk_DotFieldAccess(qualifier, target, e) and
+    result = GVN_FieldAccess(qualifier, target)
+  )
+  or
+  exists(GVN qualifier, Field target |
+    mk_PointerFieldAccess_with_deref(qualifier, target, e) and
+    result = GVN_FieldAccess(qualifier, target)
+  )
+  or
+  exists(GVN qualifier, Field target |
+    mk_ImplicitThisFieldAccess_with_deref(qualifier, target, e) and
+    result = GVN_FieldAccess(qualifier, target)
+  )
+  or
+  exists(Function fcn |
+    mk_ThisExpr(fcn, e) and
+    result = GVN_ThisExpr(fcn)
+  )
+  or
+  exists(Type t, GVN child |
+    mk_Conversion(t, child, e) and
+    result = GVN_Conversion(t, child)
+  )
+  or
+  exists(GVN lhs, GVN rhs, string opname |
+    mk_BinaryOp(lhs, rhs, opname, e) and
+    result = GVN_BinaryOp(lhs, rhs, opname)
+  )
+  or
+  exists(GVN child, string opname |
+    mk_UnaryOp(child, opname, e) and
+    result = GVN_UnaryOp(child, opname)
+  )
+  or
+  exists(GVN x, GVN i, ControlFlowNode dominator |
+    mk_ArrayAccess(x, i, dominator, e) and
+    result = GVN_ArrayAccess(x, i, dominator)
+  )
+  or
+  exists(GVN p, ControlFlowNode dominator |
+    mk_Deref(p, dominator, e) and
+    result = GVN_Deref(p, dominator)
+  )
+  or
+  not analyzableExpr(e) and result = GVN_Unanalyzable(e)
+}
+
+private predicate analyzableConst(Expr e) {
+  analyzableIntConst(e) or
+  analyzableFloatConst(e)
+}
+
+/**
+ * Holds if the expression is explicitly handled by `globalValueNumber`.
+ * Unanalyzable expressions still need to be given a global value number,
+ * but it will be a unique number that is not shared with any other
+ * expression.
+ */
+private predicate analyzableExpr(Expr e) {
+  analyzableConst(e) or
+  analyzableStackVariable(e) or
+  analyzableDotFieldAccess(e) or
+  analyzablePointerFieldAccess(e) or
+  analyzableImplicitThisFieldAccess(e) or
+  analyzableOtherVariable(e) or
+  analyzableConversion(e) or
+  analyzableBinaryOp(e) or
+  analyzableUnaryOp(e) or
+  analyzableThisExpr(e) or
+  analyzableArrayAccess(e) or
+  analyzablePointerDereferenceExpr(e)
+}

cpp/ql/test/library-tests/dataflow/DefaultTaintTracking/defaulttainttracking.cpp

@@ -79,6 +79,15 @@ void test_dynamic_cast() {
     dynamic_cast<D3*>(b2)->f(getenv("VAR")); // tainted [FALSE POSITIVE]
 }
 
+namespace std {
+  template< class T >
+  T&& move( T&& t ) noexcept;
+}
+
+void test_std_move() {
+  sink(std::move(getenv("VAR")));
+}
+
 void flow_to_outparam(char ** ret, char *arg) {
     *ret = arg; 
 }

cpp/ql/test/library-tests/dataflow/DefaultTaintTracking/tainted.expected

@@ -93,13 +93,21 @@
 | defaulttainttracking.cpp:79:30:79:35 | call to getenv | defaulttainttracking.cpp:79:30:79:35 | call to getenv |
 | defaulttainttracking.cpp:79:30:79:35 | call to getenv | defaulttainttracking.cpp:79:30:79:42 | (const char *)... |
 | defaulttainttracking.cpp:79:30:79:35 | call to getenv | test_diff.cpp:1:11:1:20 | p#0 |
-| defaulttainttracking.cpp:88:27:88:32 | call to getenv | defaulttainttracking.cpp:9:11:9:20 | p#0 |
-| defaulttainttracking.cpp:88:27:88:32 | call to getenv | defaulttainttracking.cpp:82:42:82:44 | arg |
-| defaulttainttracking.cpp:88:27:88:32 | call to getenv | defaulttainttracking.cpp:83:12:83:14 | arg |
-| defaulttainttracking.cpp:88:27:88:32 | call to getenv | defaulttainttracking.cpp:88:27:88:32 | call to getenv |
-| defaulttainttracking.cpp:88:27:88:32 | call to getenv | defaulttainttracking.cpp:89:10:89:11 | (const char *)... |
-| defaulttainttracking.cpp:88:27:88:32 | call to getenv | defaulttainttracking.cpp:89:10:89:11 | p2 |
-| defaulttainttracking.cpp:88:27:88:32 | call to getenv | test_diff.cpp:1:11:1:20 | p#0 |
+| defaulttainttracking.cpp:88:18:88:23 | call to getenv | defaulttainttracking.cpp:9:11:9:20 | p#0 |
+| defaulttainttracking.cpp:88:18:88:23 | call to getenv | defaulttainttracking.cpp:84:17:84:17 | t |
+| defaulttainttracking.cpp:88:18:88:23 | call to getenv | defaulttainttracking.cpp:88:8:88:16 | call to move |
+| defaulttainttracking.cpp:88:18:88:23 | call to getenv | defaulttainttracking.cpp:88:8:88:32 | (const char *)... |
+| defaulttainttracking.cpp:88:18:88:23 | call to getenv | defaulttainttracking.cpp:88:8:88:32 | (reference dereference) |
+| defaulttainttracking.cpp:88:18:88:23 | call to getenv | defaulttainttracking.cpp:88:18:88:23 | call to getenv |
+| defaulttainttracking.cpp:88:18:88:23 | call to getenv | defaulttainttracking.cpp:88:18:88:30 | (reference to) |
+| defaulttainttracking.cpp:88:18:88:23 | call to getenv | test_diff.cpp:1:11:1:20 | p#0 |
+| defaulttainttracking.cpp:97:27:97:32 | call to getenv | defaulttainttracking.cpp:9:11:9:20 | p#0 |
+| defaulttainttracking.cpp:97:27:97:32 | call to getenv | defaulttainttracking.cpp:91:42:91:44 | arg |
+| defaulttainttracking.cpp:97:27:97:32 | call to getenv | defaulttainttracking.cpp:92:12:92:14 | arg |
+| defaulttainttracking.cpp:97:27:97:32 | call to getenv | defaulttainttracking.cpp:97:27:97:32 | call to getenv |
+| defaulttainttracking.cpp:97:27:97:32 | call to getenv | defaulttainttracking.cpp:98:10:98:11 | (const char *)... |
+| defaulttainttracking.cpp:97:27:97:32 | call to getenv | defaulttainttracking.cpp:98:10:98:11 | p2 |
+| defaulttainttracking.cpp:97:27:97:32 | call to getenv | test_diff.cpp:1:11:1:20 | p#0 |
 | test_diff.cpp:92:10:92:13 | argv | defaulttainttracking.cpp:9:11:9:20 | p#0 |
 | test_diff.cpp:92:10:92:13 | argv | test_diff.cpp:1:11:1:20 | p#0 |
 | test_diff.cpp:92:10:92:13 | argv | test_diff.cpp:92:10:92:13 | argv |

cpp/ql/test/library-tests/dataflow/DefaultTaintTracking/test_diff.expected

@@ -9,13 +9,19 @@
 | defaulttainttracking.cpp:22:20:22:25 | call to getenv | defaulttainttracking.cpp:24:8:24:10 | array to pointer conversion | IR only |
 | defaulttainttracking.cpp:38:25:38:30 | call to getenv | defaulttainttracking.cpp:39:51:39:61 | env_pointer | AST only |
 | defaulttainttracking.cpp:64:10:64:15 | call to getenv | defaulttainttracking.cpp:52:24:52:24 | p | IR only |
-| defaulttainttracking.cpp:88:27:88:32 | call to getenv | defaulttainttracking.cpp:9:11:9:20 | p#0 | IR only |
-| defaulttainttracking.cpp:88:27:88:32 | call to getenv | defaulttainttracking.cpp:82:31:82:33 | ret | AST only |
-| defaulttainttracking.cpp:88:27:88:32 | call to getenv | defaulttainttracking.cpp:83:5:83:8 | * ... | AST only |
-| defaulttainttracking.cpp:88:27:88:32 | call to getenv | defaulttainttracking.cpp:83:6:83:8 | ret | AST only |
-| defaulttainttracking.cpp:88:27:88:32 | call to getenv | defaulttainttracking.cpp:89:10:89:11 | (const char *)... | IR only |
-| defaulttainttracking.cpp:88:27:88:32 | call to getenv | defaulttainttracking.cpp:89:10:89:11 | p2 | IR only |
-| defaulttainttracking.cpp:88:27:88:32 | call to getenv | test_diff.cpp:1:11:1:20 | p#0 | IR only |
+| defaulttainttracking.cpp:88:18:88:23 | call to getenv | defaulttainttracking.cpp:9:11:9:20 | p#0 | IR only |
+| defaulttainttracking.cpp:88:18:88:23 | call to getenv | defaulttainttracking.cpp:88:8:88:16 | call to move | IR only |
+| defaulttainttracking.cpp:88:18:88:23 | call to getenv | defaulttainttracking.cpp:88:8:88:32 | (const char *)... | IR only |
+| defaulttainttracking.cpp:88:18:88:23 | call to getenv | defaulttainttracking.cpp:88:8:88:32 | (reference dereference) | IR only |
+| defaulttainttracking.cpp:88:18:88:23 | call to getenv | defaulttainttracking.cpp:88:18:88:30 | (reference to) | IR only |
+| defaulttainttracking.cpp:88:18:88:23 | call to getenv | test_diff.cpp:1:11:1:20 | p#0 | IR only |
+| defaulttainttracking.cpp:97:27:97:32 | call to getenv | defaulttainttracking.cpp:9:11:9:20 | p#0 | IR only |
+| defaulttainttracking.cpp:97:27:97:32 | call to getenv | defaulttainttracking.cpp:91:31:91:33 | ret | AST only |
+| defaulttainttracking.cpp:97:27:97:32 | call to getenv | defaulttainttracking.cpp:92:5:92:8 | * ... | AST only |
+| defaulttainttracking.cpp:97:27:97:32 | call to getenv | defaulttainttracking.cpp:92:6:92:8 | ret | AST only |
+| defaulttainttracking.cpp:97:27:97:32 | call to getenv | defaulttainttracking.cpp:98:10:98:11 | (const char *)... | IR only |
+| defaulttainttracking.cpp:97:27:97:32 | call to getenv | defaulttainttracking.cpp:98:10:98:11 | p2 | IR only |
+| defaulttainttracking.cpp:97:27:97:32 | call to getenv | test_diff.cpp:1:11:1:20 | p#0 | IR only |
 | test_diff.cpp:104:12:104:15 | argv | test_diff.cpp:104:11:104:20 | (...) | IR only |
 | test_diff.cpp:108:10:108:13 | argv | test_diff.cpp:36:24:36:24 | p | AST only |
 | test_diff.cpp:111:10:111:13 | argv | defaulttainttracking.cpp:9:11:9:20 | p#0 | AST only |

cpp/ql/test/library-tests/dataflow/crement/crements.cpp

@@ -0,0 +1,16 @@
+void test_crement() {
+  int x1 = 0;
+  ++x1;
+
+  int x2 = 0;
+  x2++;
+
+  int x3 = 0;
+  x3 -= 1; // flow
+
+  int x4 = 0;
+  x4 |= 1; // flow
+
+  int x5 = 0;
+  x5 = x5 - 1; // flow (to RHS)
+}

cpp/ql/test/library-tests/dataflow/crement/from0.expected

@@ -0,0 +1,5 @@
+| crements.cpp:3:5:3:6 | x1 |
+| crements.cpp:6:3:6:4 | x2 |
+| crements.cpp:9:3:9:4 | x3 |
+| crements.cpp:12:3:12:4 | x4 |
+| crements.cpp:15:8:15:9 | x5 |

cpp/ql/test/library-tests/dataflow/crement/from0.ql

@@ -0,0 +1,14 @@
+import cpp
+import semmle.code.cpp.ir.dataflow.DataFlow
+
+class Cfg extends DataFlow::Configuration {
+  Cfg() { this = "from0::Cfg" }
+
+  override predicate isSource(DataFlow::Node source) { source.asExpr().getValue() = "0" }
+
+  override predicate isSink(DataFlow::Node sink) { sink.asExpr() instanceof VariableAccess }
+}
+
+from Cfg cfg, Expr sink
+where cfg.hasFlowToExpr(sink)
+select sink

cpp/ql/test/library-tests/dataflow/dataflow-tests/BarrierGuard.cpp

@@ -48,7 +48,7 @@ struct XY {
 void bg_stackstruct(XY s1, XY s2) {
   s1.x = source();
   if (guarded(s1.x)) {
-    sink(s1.x); // no flow
+    sink(s1.x); // no flow [FALSE POSITIVE in AST]
   } else if (guarded(s1.y)) {
     sink(s1.x); // flow
   } else if (guarded(s2.y)) {

cpp/ql/test/library-tests/dataflow/dataflow-tests/test.expected

@@ -3,6 +3,7 @@
 | BarrierGuard.cpp:25:10:25:15 | source | BarrierGuard.cpp:21:17:21:22 | source |
 | BarrierGuard.cpp:31:10:31:15 | source | BarrierGuard.cpp:29:16:29:21 | source |
 | BarrierGuard.cpp:33:10:33:15 | source | BarrierGuard.cpp:29:16:29:21 | source |
+| BarrierGuard.cpp:51:13:51:13 | x | BarrierGuard.cpp:49:10:49:15 | call to source |
 | BarrierGuard.cpp:53:13:53:13 | x | BarrierGuard.cpp:49:10:49:15 | call to source |
 | BarrierGuard.cpp:55:13:55:13 | x | BarrierGuard.cpp:49:10:49:15 | call to source |
 | BarrierGuard.cpp:62:14:62:14 | x | BarrierGuard.cpp:60:11:60:16 | call to source |

cpp/ql/test/library-tests/dataflow/dataflow-tests/test_diff.expected

@@ -1,3 +1,4 @@
+| BarrierGuard.cpp:49:10:49:15 | BarrierGuard.cpp:51:13:51:13 | AST only |
 | BarrierGuard.cpp:60:11:60:16 | BarrierGuard.cpp:62:14:62:14 | AST only |
 | clang.cpp:12:9:12:20 | clang.cpp:22:8:22:20 | AST only |
 | clang.cpp:28:27:28:32 | clang.cpp:30:27:30:34 | AST only |

cpp/ql/test/library-tests/dataflow/security-taint/tainted_diff.expected

@@ -0,0 +1,10 @@
+| test.cpp:49:23:49:28 | call to getenv | test.cpp:50:15:50:24 | envStr_ptr | AST only |
+| test.cpp:49:23:49:28 | call to getenv | test.cpp:50:28:50:40 | & ... | AST only |
+| test.cpp:49:23:49:28 | call to getenv | test.cpp:50:29:50:40 | envStrGlobal | AST only |
+| test.cpp:49:23:49:28 | call to getenv | test.cpp:52:2:52:12 | * ... | AST only |
+| test.cpp:49:23:49:28 | call to getenv | test.cpp:52:3:52:12 | envStr_ptr | AST only |
+| test.cpp:68:28:68:33 | call to getenv | test.cpp:11:20:11:21 | s1 | AST only |
+| test.cpp:68:28:68:33 | call to getenv | test.cpp:67:7:67:13 | copying | AST only |
+| test.cpp:68:28:68:33 | call to getenv | test.cpp:69:10:69:13 | copy | AST only |
+| test.cpp:68:28:68:33 | call to getenv | test.cpp:70:12:70:15 | copy | AST only |
+| test.cpp:68:28:68:33 | call to getenv | test.cpp:71:12:71:15 | copy | AST only |

cpp/ql/test/library-tests/dataflow/security-taint/tainted_diff.ql

@@ -0,0 +1,16 @@
+import semmle.code.cpp.security.TaintTracking as AST
+import semmle.code.cpp.ir.dataflow.DefaultTaintTracking as IR
+import cpp
+
+from Expr source, Element tainted, string side
+where
+  AST::taintedIncludingGlobalVars(source, tainted, _) and
+  not IR::taintedIncludingGlobalVars(source, tainted, _) and
+  not tainted.getLocation().getFile().getExtension() = "h" and
+  side = "AST only"
+  or
+  IR::taintedIncludingGlobalVars(source, tainted, _) and
+  not AST::taintedIncludingGlobalVars(source, tainted, _) and
+  not tainted.getLocation().getFile().getExtension() = "h" and
+  side = "IR only"
+select source, tainted, side

cpp/ql/test/library-tests/dataflow/security-taint/tainted_ir.expected

@@ -0,0 +1,42 @@
+| test.cpp:23:23:23:28 | call to getenv | test.cpp:8:24:8:25 | s1 |  |
+| test.cpp:23:23:23:28 | call to getenv | test.cpp:23:14:23:19 | envStr |  |
+| test.cpp:23:23:23:28 | call to getenv | test.cpp:23:23:23:28 | call to getenv |  |
+| test.cpp:23:23:23:28 | call to getenv | test.cpp:23:23:23:40 | (const char *)... |  |
+| test.cpp:23:23:23:28 | call to getenv | test.cpp:25:6:25:29 | ! ... |  |
+| test.cpp:23:23:23:28 | call to getenv | test.cpp:25:7:25:12 | call to strcmp |  |
+| test.cpp:23:23:23:28 | call to getenv | test.cpp:25:7:25:29 | (bool)... |  |
+| test.cpp:23:23:23:28 | call to getenv | test.cpp:25:14:25:19 | envStr |  |
+| test.cpp:23:23:23:28 | call to getenv | test.cpp:29:6:29:28 | ! ... |  |
+| test.cpp:23:23:23:28 | call to getenv | test.cpp:29:7:29:12 | call to strcmp |  |
+| test.cpp:23:23:23:28 | call to getenv | test.cpp:29:7:29:28 | (bool)... |  |
+| test.cpp:23:23:23:28 | call to getenv | test.cpp:29:14:29:19 | envStr |  |
+| test.cpp:38:23:38:28 | call to getenv | test.cpp:8:24:8:25 | s1 |  |
+| test.cpp:38:23:38:28 | call to getenv | test.cpp:38:14:38:19 | envStr |  |
+| test.cpp:38:23:38:28 | call to getenv | test.cpp:38:23:38:28 | call to getenv |  |
+| test.cpp:38:23:38:28 | call to getenv | test.cpp:38:23:38:40 | (const char *)... |  |
+| test.cpp:38:23:38:28 | call to getenv | test.cpp:40:14:40:19 | envStr |  |
+| test.cpp:49:23:49:28 | call to getenv | test.cpp:8:24:8:25 | s1 |  |
+| test.cpp:49:23:49:28 | call to getenv | test.cpp:45:13:45:24 | envStrGlobal |  |
+| test.cpp:49:23:49:28 | call to getenv | test.cpp:49:14:49:19 | envStr |  |
+| test.cpp:49:23:49:28 | call to getenv | test.cpp:49:23:49:28 | call to getenv |  |
+| test.cpp:49:23:49:28 | call to getenv | test.cpp:49:23:49:40 | (const char *)... |  |
+| test.cpp:49:23:49:28 | call to getenv | test.cpp:52:16:52:21 | envStr |  |
+| test.cpp:49:23:49:28 | call to getenv | test.cpp:54:6:54:35 | ! ... |  |
+| test.cpp:49:23:49:28 | call to getenv | test.cpp:54:7:54:12 | call to strcmp |  |
+| test.cpp:49:23:49:28 | call to getenv | test.cpp:54:7:54:35 | (bool)... |  |
+| test.cpp:49:23:49:28 | call to getenv | test.cpp:54:14:54:25 | envStrGlobal |  |
+| test.cpp:60:29:60:34 | call to getenv | test.cpp:10:27:10:27 | s |  |
+| test.cpp:60:29:60:34 | call to getenv | test.cpp:60:18:60:25 | userName |  |
+| test.cpp:60:29:60:34 | call to getenv | test.cpp:60:29:60:34 | call to getenv |  |
+| test.cpp:60:29:60:34 | call to getenv | test.cpp:60:29:60:47 | (const char *)... |  |
+| test.cpp:60:29:60:34 | call to getenv | test.cpp:64:25:64:32 | userName |  |
+| test.cpp:68:28:68:33 | call to getenv | test.cpp:11:36:11:37 | s2 |  |
+| test.cpp:68:28:68:33 | call to getenv | test.cpp:68:17:68:24 | userName |  |
+| test.cpp:68:28:68:33 | call to getenv | test.cpp:68:28:68:33 | call to getenv |  |
+| test.cpp:68:28:68:33 | call to getenv | test.cpp:68:28:68:46 | (const char *)... |  |
+| test.cpp:68:28:68:33 | call to getenv | test.cpp:70:5:70:10 | call to strcpy |  |
+| test.cpp:68:28:68:33 | call to getenv | test.cpp:70:18:70:25 | userName |  |
+| test.cpp:75:20:75:25 | call to getenv | test.cpp:15:22:15:25 | nptr |  |
+| test.cpp:75:20:75:25 | call to getenv | test.cpp:75:15:75:18 | call to atoi |  |
+| test.cpp:75:20:75:25 | call to getenv | test.cpp:75:20:75:25 | call to getenv |  |
+| test.cpp:75:20:75:25 | call to getenv | test.cpp:75:20:75:45 | (const char *)... |  |

cpp/ql/test/library-tests/dataflow/security-taint/tainted_ir.ql

@@ -0,0 +1,7 @@
+import semmle.code.cpp.ir.dataflow.DefaultTaintTracking
+
+from Expr source, Element tainted, string globalVar
+where
+  taintedIncludingGlobalVars(source, tainted, globalVar) and
+  not tainted.getLocation().getFile().getExtension() = "h"
+select source, tainted, globalVar

cpp/ql/test/library-tests/dataflow/taint-tests/test_diff.expected

@@ -34,7 +34,6 @@
 | taint.cpp:352:7:352:7 | taint.cpp:330:6:330:11 | AST only |
 | taint.cpp:372:7:372:7 | taint.cpp:365:24:365:29 | AST only |
 | taint.cpp:374:7:374:7 | taint.cpp:365:24:365:29 | AST only |
-| taint.cpp:382:7:382:7 | taint.cpp:377:23:377:28 | AST only |
 | taint.cpp:391:7:391:7 | taint.cpp:385:27:385:32 | AST only |
 | taint.cpp:423:7:423:7 | taint.cpp:422:14:422:19 | AST only |
 | taint.cpp:424:9:424:17 | taint.cpp:422:14:422:19 | AST only |

cpp/ql/test/library-tests/dataflow/taint-tests/test_ir.expected

@@ -17,5 +17,6 @@
 | taint.cpp:291:7:291:7 | y | taint.cpp:275:6:275:11 | call to source |
 | taint.cpp:337:7:337:7 | t | taint.cpp:330:6:330:11 | call to source |
 | taint.cpp:350:7:350:7 | t | taint.cpp:330:6:330:11 | call to source |
+| taint.cpp:382:7:382:7 | a | taint.cpp:377:23:377:28 | source |
 | taint.cpp:429:7:429:7 | b | taint.cpp:428:13:428:18 | call to source |
 | taint.cpp:430:9:430:14 | member | taint.cpp:428:13:428:18 | call to source |

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

@@ -5753,9 +5753,9 @@ ir.cpp:
 #  851|       0: [VariableDeclarationEntry] definition of d
 #  851|           Type = [Struct] PolymorphicDerived
 #  851|         init: [Initializer] initializer for d
-#  851|           expr: [ConstructorCall] call to PolymorphicDerived
-#  851|               Type = [VoidType] void
-#  851|               ValueCategory = prvalue
+#-----|           expr: [ConstructorCall] call to PolymorphicDerived
+#-----|               Type = [VoidType] void
+#-----|               ValueCategory = prvalue
 #  853|     2: [DeclStmt] declaration
 #  853|       0: [VariableDeclarationEntry] definition of pb
 #  853|           Type = [PointerType] PolymorphicBase *

cpp/ql/test/library-tests/ir/ssa/ssa.cpp

@@ -243,3 +243,22 @@ void ExplicitConstructorCalls() {
   Constructible c2 = Constructible(2);
   c2.g();
 }
+
+char *VoidStarIndirectParameters(char *src, int size) {
+  char *dst = new char[size];
+  *src = 'a';
+  memcpy(dst, src, size);
+  return dst;
+}
+
+char StringLiteralAliasing2(bool b) {
+  if (b) {
+    ExternalFunc();
+  }
+  else {
+    ExternalFunc();
+  }
+
+  const char* s = "Literal";
+  return s[2];
+}

cpp/ql/test/library-tests/locations/overloaded_operators/locations.expected

@@ -1,4 +1,4 @@
-| test.cpp:9:12:9:35 | call to MyInt |
+| test.cpp:9:5:9:36 | call to MyInt |
 | test.cpp:26:18:26:23 | call to MyInt |
 | test.cpp:42:15:42:15 | call to operator+ |
 | test.cpp:43:5:43:5 | call to operator+= |

cpp/ql/test/library-tests/permissive/calls.expected

@@ -1,2 +1 @@
-| non_permissive.cpp:6:3:6:3 | call to f | non_permissive.cpp:2:13:2:13 | f |
 | permissive.cpp:6:3:6:3 | call to f | permissive.cpp:2:13:2:13 | f |

cpp/ql/test/library-tests/rangeanalysis/rangeanalysis/RangeAnalysis.expected

@@ -59,3 +59,8 @@
 | test.cpp:183:10:183:10 | Load: i | test.cpp:175:23:175:23 | InitializeParameter: x | -1 | true | CompareLT: ... < ... | test.cpp:182:9:182:13 | test.cpp:182:9:182:13 |
 | test.cpp:185:10:185:10 | Load: i | test.cpp:175:23:175:23 | InitializeParameter: x | 0 | true | CompareLT: ... < ... | test.cpp:176:7:176:11 | test.cpp:176:7:176:11 |
 | test.cpp:187:10:187:10 | Store: i | test.cpp:175:23:175:23 | InitializeParameter: x | 0 | false | CompareLT: ... < ... | test.cpp:182:9:182:13 | test.cpp:182:9:182:13 |
+| test.cpp:194:8:194:8 | Load: l | test.cpp:191:16:191:16 | InitializeParameter: i | 0 | false | NoReason | file://:0:0:0:0 | file://:0:0:0:0 |
+| test.cpp:194:8:194:8 | Load: l | test.cpp:191:16:191:16 | InitializeParameter: i | 0 | true | NoReason | file://:0:0:0:0 | file://:0:0:0:0 |
+| test.cpp:200:10:200:10 | Load: i | test.cpp:198:25:198:25 | InitializeParameter: l | -1 | true | CompareLT: ... < ... | test.cpp:199:7:199:11 | test.cpp:199:7:199:11 |
+| test.cpp:203:11:203:11 | Load: i | test.cpp:198:25:198:25 | InitializeParameter: l | -3 | true | CompareLT: ... < ... | test.cpp:202:7:202:15 | test.cpp:202:7:202:15 |
+| test.cpp:209:10:209:10 | Load: x | test.cpp:207:24:207:24 | InitializeParameter: y | -3 | true | CompareLT: ... < ... | test.cpp:208:7:208:15 | test.cpp:208:7:208:15 |

cpp/ql/test/library-tests/rangeanalysis/rangeanalysis/test.cpp

@@ -186,3 +186,26 @@ int test15(int i, int x) {
   }
   return i;
 }
+
+// safe integer type conversion
+int test16(int i) {
+  long l;
+  l = i;
+  sink(l);
+}
+
+// implicit integer casts
+void test17(int i, long l) {
+  if (i < l) {
+    sink(i);
+  }
+  if (i < l - 2) {
+    sink (i);
+  }
+}
+
+void test18(int x, int y) {
+  if (x < y - 2) {
+    sink(x);
+  }
+}

cpp/ql/test/library-tests/rangeanalysis/signanalysis/SignAnalysis.expected

@@ -52,9 +52,9 @@
 | test.c:18:10:18:14 | Load: count | positive |
 | test.c:18:10:18:14 | Store: count | positive |
 | test.c:23:10:23:10 | Phi: p | positive |
+| test.c:24:5:24:9 | Load: count | positive |
 | test.c:24:5:24:11 | Add: ... ++ | positive strictlyPositive |
 | test.c:24:5:24:11 | Constant: ... ++ | positive strictlyPositive |
-| test.c:24:5:24:11 | Load: ... ++ | positive |
 | test.c:24:5:24:11 | Store: ... ++ | positive strictlyPositive |
 | test.c:25:5:25:22 | Store: ... = ... | positive |
 | test.c:25:13:25:17 | Load: count | positive strictlyPositive |
@@ -70,8 +70,8 @@
 | test.c:33:26:33:26 | Load: i | positive |
 | test.c:33:26:33:28 | Add: ... + ... | positive strictlyPositive |
 | test.c:33:28:33:28 | Constant: 1 | positive strictlyPositive |
+| test.c:34:5:34:9 | Load: total | positive |
 | test.c:34:5:34:14 | Add: ... += ... | positive |
-| test.c:34:5:34:14 | Load: ... += ... | positive |
 | test.c:34:5:34:14 | Store: ... += ... | positive |
 | test.c:34:14:34:14 | Load: i | positive |
 | test.c:36:10:36:14 | Load: total | positive |
@@ -82,12 +82,12 @@
 | test.c:42:15:42:15 | Phi: i | positive |
 | test.c:42:15:42:15 | Phi: i | positive |
 | test.c:42:19:42:19 | Constant: 2 | positive strictlyPositive |
+| test.c:42:22:42:22 | Load: i | positive |
 | test.c:42:22:42:24 | Add: ... ++ | positive strictlyPositive |
 | test.c:42:22:42:24 | Constant: ... ++ | positive strictlyPositive |
-| test.c:42:22:42:24 | Load: ... ++ | positive |
 | test.c:42:22:42:24 | Store: ... ++ | positive strictlyPositive |
+| test.c:43:5:43:9 | Load: total | positive |
 | test.c:43:5:43:14 | Add: ... += ... | positive |
-| test.c:43:5:43:14 | Load: ... += ... | positive |
 | test.c:43:5:43:14 | Store: ... += ... | positive |
 | test.c:43:14:43:14 | Load: i | positive |
 | test.c:45:10:45:14 | Load: total | positive |
@@ -104,8 +104,8 @@
 | test.c:51:28:51:28 | Load: i | positive |
 | test.c:51:28:51:30 | Add: ... + ... | positive strictlyPositive |
 | test.c:51:30:51:30 | Constant: 1 | positive strictlyPositive |
+| test.c:52:5:52:9 | Load: total | positive |
 | test.c:52:5:52:14 | Add: ... += ... | positive |
-| test.c:52:5:52:14 | Load: ... += ... | positive |
 | test.c:52:5:52:14 | Store: ... += ... | positive |
 | test.c:52:14:52:14 | Load: i | positive |
 | test.c:54:10:54:14 | Load: total | positive |
@@ -147,8 +147,8 @@
 | test.c:124:36:124:36 | Constant: (unsigned long long)... | positive strictlyPositive |
 | test.c:126:31:126:43 | Call: call to test12_helper | positive |
 | test.c:126:31:126:43 | Store: call to test12_helper | positive |
+| test.c:127:6:127:10 | Load: Start | positive |
 | test.c:127:6:127:24 | Add: ... += ... | positive strictlyPositive |
-| test.c:127:6:127:24 | Load: ... += ... | positive |
 | test.c:127:6:127:24 | Store: ... += ... | positive strictlyPositive |
 | test.c:127:15:127:20 | Load: Length | positive |
 | test.c:127:15:127:24 | Add: ... + ... | positive strictlyPositive |
@@ -252,8 +252,8 @@
 | test.c:205:13:205:15 | Mul: ... * ... | positive |
 | test.c:205:13:205:15 | Store: ... * ... | positive |
 | test.c:205:15:205:15 | Load: b | positive |
+| test.c:206:5:206:9 | Load: total | positive |
 | test.c:206:5:206:14 | Add: ... += ... | positive |
-| test.c:206:5:206:14 | Load: ... += ... | positive |
 | test.c:206:5:206:14 | Store: ... += ... | positive |
 | test.c:206:14:206:14 | Load: r | positive |
 | test.c:208:7:208:7 | Constant: 3 | positive strictlyPositive |
@@ -263,7 +263,7 @@
 | test.c:208:28:208:30 | Constant: - ... | negative strictlyNegative |
 | test.c:208:45:208:46 | Constant: 23 | positive strictlyPositive |
 | test.c:209:13:209:13 | Load: a | positive strictlyPositive |
-| test.c:210:5:210:14 | Load: ... += ... | positive |
+| test.c:210:5:210:9 | Load: total | positive |
 | test.c:212:7:212:7 | Constant: 3 | positive strictlyPositive |
 | test.c:212:17:212:17 | Load: a | positive strictlyPositive |
 | test.c:212:22:212:23 | Constant: 11 | positive strictlyPositive |
@@ -304,8 +304,8 @@
 | test.c:233:13:233:15 | Mul: ... * ... | positive |
 | test.c:233:13:233:15 | Store: ... * ... | positive |
 | test.c:233:15:233:15 | Load: b | positive |
+| test.c:234:5:234:9 | Load: total | positive |
 | test.c:234:5:234:14 | Add: ... += ... | positive |
-| test.c:234:5:234:14 | Load: ... += ... | positive |
 | test.c:234:5:234:14 | Store: ... += ... | positive |
 | test.c:234:14:234:14 | Load: r | positive |
 | test.c:236:7:236:7 | Phi: 0 | positive |
@@ -314,7 +314,7 @@
 | test.c:236:28:236:30 | Constant: - ... | negative strictlyNegative |
 | test.c:236:45:236:46 | Constant: 23 | positive strictlyPositive |
 | test.c:237:13:237:13 | Load: a | positive |
-| test.c:238:5:238:14 | Load: ... += ... | positive |
+| test.c:238:5:238:9 | Load: total | positive |
 | test.c:240:17:240:17 | Load: a | positive |
 | test.c:240:22:240:23 | Constant: 11 | positive strictlyPositive |
 | test.c:240:28:240:30 | Constant: - ... | negative strictlyNegative |
@@ -375,8 +375,8 @@
 | test.c:289:13:289:15 | Mul: ... * ... | negative |
 | test.c:289:13:289:15 | Store: ... * ... | negative |
 | test.c:289:15:289:15 | Load: b | positive |
+| test.c:290:5:290:9 | Load: total | negative |
 | test.c:290:5:290:14 | Add: ... += ... | negative |
-| test.c:290:5:290:14 | Load: ... += ... | negative |
 | test.c:290:5:290:14 | Store: ... += ... | negative |
 | test.c:290:14:290:14 | Load: r | negative |
 | test.c:292:7:292:9 | Constant: - ... | negative strictlyNegative |
@@ -384,7 +384,7 @@
 | test.c:292:29:292:31 | Constant: - ... | negative strictlyNegative |
 | test.c:292:46:292:47 | Constant: 23 | positive strictlyPositive |
 | test.c:293:13:293:13 | Load: a | negative |
-| test.c:294:5:294:14 | Load: ... += ... | negative |
+| test.c:294:5:294:9 | Load: total | negative |
 | test.c:296:7:296:9 | Constant: - ... | negative strictlyNegative |
 | test.c:296:29:296:31 | Constant: - ... | negative strictlyNegative |
 | test.c:297:13:297:13 | Load: a | negative |
@@ -421,8 +421,8 @@
 | test.c:317:13:317:15 | Mul: ... * ... | negative |
 | test.c:317:13:317:15 | Store: ... * ... | negative |
 | test.c:317:15:317:15 | Load: b | positive |
+| test.c:318:5:318:9 | Load: total | negative |
 | test.c:318:5:318:14 | Add: ... += ... | negative |
-| test.c:318:5:318:14 | Load: ... += ... | negative |
 | test.c:318:5:318:14 | Store: ... += ... | negative |
 | test.c:318:14:318:14 | Load: r | negative |
 | test.c:320:7:320:9 | Constant: - ... | negative strictlyNegative |
@@ -431,7 +431,7 @@
 | test.c:320:30:320:32 | Constant: - ... | negative strictlyNegative |
 | test.c:320:47:320:48 | Constant: 23 | positive strictlyPositive |
 | test.c:321:13:321:13 | Load: a | negative strictlyNegative |
-| test.c:322:5:322:14 | Load: ... += ... | negative |
+| test.c:322:5:322:9 | Load: total | negative |
 | test.c:324:7:324:9 | Constant: - ... | negative strictlyNegative |
 | test.c:324:24:324:25 | Constant: - ... | negative strictlyNegative |
 | test.c:324:30:324:32 | Constant: - ... | negative strictlyNegative |
@@ -454,9 +454,9 @@
 | test.c:342:10:342:10 | Load: i | positive |
 | test.c:342:10:342:10 | Phi: i | positive |
 | test.c:342:14:342:14 | Constant: 3 | positive strictlyPositive |
+| test.c:343:5:343:5 | Load: i | positive |
 | test.c:343:5:343:7 | Add: ... ++ | positive strictlyPositive |
 | test.c:343:5:343:7 | Constant: ... ++ | positive strictlyPositive |
-| test.c:343:5:343:7 | Load: ... ++ | positive |
 | test.c:343:5:343:7 | Store: ... ++ | positive strictlyPositive |
 | test.c:345:3:345:7 | Store: ... = ... | positive strictlyPositive |
 | test.c:345:7:345:7 | Load: i | positive strictlyPositive |
@@ -646,18 +646,18 @@
 | test.c:397:3:397:15 | Store: ... = ... | positive strictlyPositive |
 | test.c:397:9:397:11 | Add: ++ ... | positive strictlyPositive |
 | test.c:397:9:397:11 | Constant: ++ ... | positive strictlyPositive |
-| test.c:397:9:397:11 | Load: ++ ... | positive |
 | test.c:397:9:397:11 | Store: ++ ... | positive strictlyPositive |
 | test.c:397:9:397:14 | CopyValue: ... , ... | positive strictlyPositive |
+| test.c:397:11:397:11 | Load: y | positive |
 | test.c:397:14:397:14 | Load: y | positive strictlyPositive |
 | test.c:398:3:398:23 | Store: ... = ... | positive strictlyPositive |
+| test.c:398:9:398:9 | Load: y | positive strictlyPositive |
 | test.c:398:9:398:11 | Add: ... ++ | positive strictlyPositive |
 | test.c:398:9:398:11 | Constant: ... ++ | positive strictlyPositive |
-| test.c:398:9:398:11 | Load: ... ++ | positive strictlyPositive |
 | test.c:398:9:398:11 | Store: ... ++ | positive strictlyPositive |
 | test.c:398:9:398:22 | CopyValue: ... , ... | positive strictlyPositive |
+| test.c:398:14:398:14 | Load: y | positive strictlyPositive |
 | test.c:398:14:398:19 | Add: ... += ... | positive strictlyPositive |
-| test.c:398:14:398:19 | Load: ... += ... | positive strictlyPositive |
 | test.c:398:14:398:19 | Store: ... += ... | positive strictlyPositive |
 | test.c:398:19:398:19 | Constant: (unsigned int)... | positive strictlyPositive |
 | test.c:398:22:398:22 | Load: y | positive strictlyPositive |

cpp/ql/test/library-tests/valuenumbering/GlobalValueNumbering/GlobalValueNumbering.expected

@@ -30,3 +30,11 @@
 | test.cpp:105:11:105:12 | (Base *)... | 105:c11-c12 106:c14-c35 107:c11-c12 |
 | test.cpp:105:11:105:12 | pd | 105:c11-c12 106:c33-c34 |
 | test.cpp:105:15:105:15 | b | 105:c15-c15 107:c15-c15 109:c10-c10 |
+| test.cpp:125:11:125:12 | pa | 125:c11-c12 126:c11-c12 128:c3-c4 129:c11-c12 |
+| test.cpp:125:15:125:15 | x | 125:c15-c15 126:c15-c15 128:c7-c7 |
+| test.cpp:136:11:136:18 | global_a | 136:c11-c18 137:c11-c18 139:c3-c10 |
+| test.cpp:136:21:136:21 | x | 136:c21-c21 137:c21-c21 139:c13-c13 |
+| test.cpp:144:11:144:12 | pa | 144:c11-c12 145:c11-c12 147:c3-c4 149:c11-c12 |
+| test.cpp:145:15:145:15 | y | 145:c15-c15 147:c7-c7 |
+| test.cpp:153:11:153:18 | global_a | 153:c11-c18 154:c11-c18 156:c3-c10 |
+| test.cpp:153:21:153:21 | x | 153:c21-c21 154:c21-c21 |

cpp/ql/test/library-tests/valuenumbering/GlobalValueNumbering/test.cpp

@@ -42,7 +42,7 @@ int test03(int p0, int p1, int* p2) {
 
   x = p0 + p1 + global03;
   *p2 = 0; // Might change global03
-  x = p0 + p1 + global03; // Not the same value
+  x = p0 + p1 + global03; // BUG: Not the same value, but given the same value number (this is likely due to #2696)
   y = x;
 }
 
@@ -115,3 +115,45 @@ void test06() {
   "a";
   "c";
 }
+
+struct A {
+  int x;
+  int y;
+};
+
+void test_read_arg_same(A *pa, int n) {
+  int b = pa->x;
+  int c = pa->x;
+
+  pa->x = n;
+  int d = pa->x;
+}
+
+A* global_a;
+int global_n;
+
+void test_read_global_same() {
+  int b = global_a->x;
+  int c = global_a->x;
+
+  global_a->x = global_n;
+  int d = global_a->x;
+}
+
+void test_read_arg_different(A *pa) {
+  int b = pa->x;
+  int c = pa->y;
+
+  pa->y = global_n;
+
+  int d = pa->x;
+}
+
+void test_read_global_different(int n) {
+  int b = global_a->x;
+  int c = global_a->x;
+
+  global_a->y = n;
+
+  int d = global_a->x;
+}

cpp/ql/test/query-tests/Likely Bugs/RedundantNullCheckSimple/RedundantNullCheckSimple.cpp

@@ -17,7 +17,7 @@ void test_not_same_basic_block(int *p) {
 void test_indirect(int **p) {
   int x;
   x = **p;
-  if (*p == nullptr) { // BAD [NOT DETECTED]
+  if (*p == nullptr) { // BAD
     return;
   }
 }

cpp/ql/test/query-tests/Likely Bugs/RedundantNullCheckSimple/RedundantNullCheckSimple.expected

@@ -1,5 +1,6 @@
 | RedundantNullCheckSimple.cpp:4:7:4:7 | Load: p | This null check is redundant because the value is $@ in any case | RedundantNullCheckSimple.cpp:3:7:3:8 | Load: * ... | dereferenced here |
 | RedundantNullCheckSimple.cpp:13:8:13:8 | Load: p | This null check is redundant because the value is $@ in any case | RedundantNullCheckSimple.cpp:10:11:10:12 | Load: * ... | dereferenced here |
+| RedundantNullCheckSimple.cpp:20:7:20:8 | Load: * ... | This null check is redundant because the value is $@ in any case | RedundantNullCheckSimple.cpp:19:7:19:9 | Load: * ... | dereferenced here |
 | RedundantNullCheckSimple.cpp:48:12:48:12 | Load: p | This null check is redundant because the value is $@ in any case | RedundantNullCheckSimple.cpp:51:10:51:11 | Load: * ... | dereferenced here |
 | RedundantNullCheckSimple.cpp:79:7:79:9 | Load: * ... | This null check is redundant because the value is $@ in any case | RedundantNullCheckSimple.cpp:78:7:78:10 | Load: * ... | dereferenced here |
 | RedundantNullCheckSimple.cpp:93:13:93:13 | Load: p | This null check is redundant because the value is $@ in any case | RedundantNullCheckSimple.cpp:92:13:92:18 | Load: * ... | dereferenced here |

cpp/ql/test/query-tests/Security/CWE/CWE-807/semmle/TaintedCondition/test.cpp

@@ -35,7 +35,7 @@ void processRequest()
         adminPrivileges = 0; // OK, since it's a 0 and not a 1
      }
 
-     // BAD, but it requires pointer analysis to catch
+     // BAD (requires pointer analysis to catch)
      const char** userp = &currentUser;
      *userp = userName;
      if (!strcmp(currentUser, "admin")) {

cpp/ql/test/query-tests/definitions/definitions.expected

@@ -42,7 +42,7 @@
 | class.cpp:91:27:91:29 | num | class.cpp:87:17:87:19 | num | V |
 | class.cpp:100:24:100:34 | type mention | class.cpp:94:18:94:28 | string_type | T |
 | class.cpp:105:1:105:15 | type mention | class.cpp:97:7:97:21 | StringContainer | T |
-| class.cpp:106:9:106:23 | type mention | class.cpp:100:2:100:16 | StringContainer | M |
+| class.cpp:106:9:106:23 | type mention | class.cpp:97:7:97:21 | StringContainer | T |
 | class.cpp:106:25:106:27 | STR(x) | class.cpp:95:1:95:18 | #define STR(x) L ## x | X |
 | class.cpp:117:2:117:29 | type mention | class.cpp:109:7:109:34 | myClassWithConstructorParams | T |
 | class.cpp:117:37:117:37 | a | class.cpp:115:27:115:27 | a | V |

csharp/ql/src/Likely Bugs/DangerousNonShortCircuitLogic.ql

@@ -27,7 +27,8 @@ class DangerousExpression extends Expr {
       e instanceof MethodCall
       or
       e instanceof ArrayAccess
-    )
+    ) and
+    not exists(Expr e | this = e.getParent*() | e.(Call).getTarget().getAParameter().isOutOrRef())
   }
 }
 

csharp/ql/src/semmle/code/csharp/dataflow/internal/DataFlowImpl.qll

@@ -2041,12 +2041,12 @@ private class PathNodeMid extends PathNode, TPathNodeMid {
     result = getSuccMid()
     or
     // a final step to a sink via zero steps means we merge the last two steps to prevent trivial-looking edges
-    exists(PathNodeMid mid |
+    exists(PathNodeMid mid, PathNodeSink sink |
       mid = getSuccMid() and
-      mid.getNode() = result.getNode() and
+      mid.getNode() = sink.getNode() and
       mid.getAp() instanceof AccessPathNil and
-      result instanceof PathNodeSink and
-      result.getConfiguration() = unbind(mid.getConfiguration())
+      sink.getConfiguration() = unbind(mid.getConfiguration()) and
+      result = sink
     )
   }
 

csharp/ql/src/semmle/code/csharp/dataflow/internal/DataFlowImpl2.qll

@@ -2041,12 +2041,12 @@ private class PathNodeMid extends PathNode, TPathNodeMid {
     result = getSuccMid()
     or
     // a final step to a sink via zero steps means we merge the last two steps to prevent trivial-looking edges
-    exists(PathNodeMid mid |
+    exists(PathNodeMid mid, PathNodeSink sink |
       mid = getSuccMid() and
-      mid.getNode() = result.getNode() and
+      mid.getNode() = sink.getNode() and
       mid.getAp() instanceof AccessPathNil and
-      result instanceof PathNodeSink and
-      result.getConfiguration() = unbind(mid.getConfiguration())
+      sink.getConfiguration() = unbind(mid.getConfiguration()) and
+      result = sink
     )
   }
 

csharp/ql/src/semmle/code/csharp/dataflow/internal/DataFlowImpl3.qll

@@ -2041,12 +2041,12 @@ private class PathNodeMid extends PathNode, TPathNodeMid {
     result = getSuccMid()
     or
     // a final step to a sink via zero steps means we merge the last two steps to prevent trivial-looking edges
-    exists(PathNodeMid mid |
+    exists(PathNodeMid mid, PathNodeSink sink |
       mid = getSuccMid() and
-      mid.getNode() = result.getNode() and
+      mid.getNode() = sink.getNode() and
       mid.getAp() instanceof AccessPathNil and
-      result instanceof PathNodeSink and
-      result.getConfiguration() = unbind(mid.getConfiguration())
+      sink.getConfiguration() = unbind(mid.getConfiguration()) and
+      result = sink
     )
   }
 

csharp/ql/src/semmle/code/csharp/dataflow/internal/DataFlowImpl4.qll

@@ -2041,12 +2041,12 @@ private class PathNodeMid extends PathNode, TPathNodeMid {
     result = getSuccMid()
     or
     // a final step to a sink via zero steps means we merge the last two steps to prevent trivial-looking edges
-    exists(PathNodeMid mid |
+    exists(PathNodeMid mid, PathNodeSink sink |
       mid = getSuccMid() and
-      mid.getNode() = result.getNode() and
+      mid.getNode() = sink.getNode() and
       mid.getAp() instanceof AccessPathNil and
-      result instanceof PathNodeSink and
-      result.getConfiguration() = unbind(mid.getConfiguration())
+      sink.getConfiguration() = unbind(mid.getConfiguration()) and
+      result = sink
     )
   }
 

csharp/ql/src/semmle/code/csharp/dataflow/internal/DataFlowImpl5.qll

@@ -2041,12 +2041,12 @@ private class PathNodeMid extends PathNode, TPathNodeMid {
     result = getSuccMid()
     or
     // a final step to a sink via zero steps means we merge the last two steps to prevent trivial-looking edges
-    exists(PathNodeMid mid |
+    exists(PathNodeMid mid, PathNodeSink sink |
       mid = getSuccMid() and
-      mid.getNode() = result.getNode() and
+      mid.getNode() = sink.getNode() and
       mid.getAp() instanceof AccessPathNil and
-      result instanceof PathNodeSink and
-      result.getConfiguration() = unbind(mid.getConfiguration())
+      sink.getConfiguration() = unbind(mid.getConfiguration()) and
+      result = sink
     )
   }
 

csharp/ql/src/semmle/code/csharp/ir/implementation/Opcode.qll

@@ -63,6 +63,7 @@ private newtype TOpcode =
   TUnmodeledDefinition() or
   TUnmodeledUse() or
   TAliasedDefinition() or
+  TInitializeNonLocal() or
   TAliasedUse() or
   TPhi() or
   TBuiltIn() or
@@ -596,6 +597,14 @@ module Opcode {
     final override MemoryAccessKind getWriteMemoryAccess() { result instanceof EscapedMemoryAccess }
   }
 
+  class InitializeNonLocal extends Opcode, TInitializeNonLocal {
+    final override string toString() { result = "InitializeNonLocal" }
+
+    final override MemoryAccessKind getWriteMemoryAccess() {
+      result instanceof NonLocalMemoryAccess
+    }
+  }
+
   class AliasedUse extends Opcode, TAliasedUse {
     final override string toString() { result = "AliasedUse" }
 

csharp/ql/src/semmle/code/csharp/ir/implementation/raw/gvn/ValueNumbering.qll

@@ -52,6 +52,11 @@ newtype TValueNumber =
   ) {
     inheritanceConversionValueNumber(_, irFunc, opcode, baseClass, derivedClass, operand)
   } or
+  TLoadTotalOverlapValueNumber(
+    IRFunction irFunc, IRType type, ValueNumber memOperand, ValueNumber operand
+  ) {
+    loadTotalOverlapValueNumber(_, irFunc, type, memOperand, operand)
+  } or
   TUniqueValueNumber(IRFunction irFunc, Instruction instr) { uniqueValueNumber(instr, irFunc) }
 
 /**
@@ -101,12 +106,18 @@ class ValueNumber extends TValueNumber {
  * The use of `p.x` on line 3 is linked to the definition of `p` on line 1 as well, but is not
  * congruent to that definition because `p.x` accesses only a subset of the memory defined by `p`.
  */
-private class CongruentCopyInstruction extends CopyInstruction {
+class CongruentCopyInstruction extends CopyInstruction {
   CongruentCopyInstruction() {
     this.getSourceValueOperand().getDefinitionOverlap() instanceof MustExactlyOverlap
   }
 }
 
+class LoadTotalOverlapInstruction extends LoadInstruction {
+  LoadTotalOverlapInstruction() {
+    this.getSourceValueOperand().getDefinitionOverlap() instanceof MustTotallyOverlap
+  }
+}
+
 /**
  * Holds if this library knows how to assign a value number to the specified instruction, other than
  * a `unique` value number that is never shared by multiple instructions.
@@ -131,6 +142,8 @@ private predicate numberableInstruction(Instruction instr) {
   instr instanceof PointerArithmeticInstruction
   or
   instr instanceof CongruentCopyInstruction
+  or
+  instr instanceof LoadTotalOverlapInstruction
 }
 
 private predicate variableAddressValueNumber(
@@ -205,6 +218,7 @@ private predicate unaryValueNumber(
   instr.getEnclosingIRFunction() = irFunc and
   not instr instanceof InheritanceConversionInstruction and
   not instr instanceof CopyInstruction and
+  not instr instanceof FieldAddressInstruction and
   instr.getOpcode() = opcode and
   instr.getResultIRType() = type and
   valueNumber(instr.getUnary()) = operand
@@ -221,6 +235,16 @@ private predicate inheritanceConversionValueNumber(
   valueNumber(instr.getUnary()) = operand
 }
 
+private predicate loadTotalOverlapValueNumber(
+  LoadTotalOverlapInstruction instr, IRFunction irFunc, IRType type, ValueNumber memOperand,
+  ValueNumber operand
+) {
+  instr.getEnclosingIRFunction() = irFunc and
+  instr.getResultIRType() = type and
+  valueNumber(instr.getAnOperand().(MemoryOperand).getAnyDef()) = memOperand and
+  valueNumberOfOperand(instr.getAnOperand().(AddressOperand)) = operand
+}
+
 /**
  * Holds if `instr` should be assigned a unique value number because this library does not know how
  * to determine if two instances of that instruction are equivalent.
@@ -313,6 +337,11 @@ private ValueNumber nonUniqueValueNumber(Instruction instr) {
           TPointerArithmeticValueNumber(irFunc, opcode, type, elementSize, leftOperand, rightOperand)
       )
       or
+      exists(IRType type, ValueNumber memOperand, ValueNumber operand |
+        loadTotalOverlapValueNumber(instr, irFunc, type, memOperand, operand) and
+        result = TLoadTotalOverlapValueNumber(irFunc, type, memOperand, operand)
+      )
+      or
       // The value number of a copy is just the value number of its source value.
       result = valueNumber(instr.(CongruentCopyInstruction).getSourceValue())
     )

csharp/ql/src/semmle/code/csharp/ir/implementation/unaliased_ssa/gvn/ValueNumbering.qll

@@ -52,6 +52,11 @@ newtype TValueNumber =
   ) {
     inheritanceConversionValueNumber(_, irFunc, opcode, baseClass, derivedClass, operand)
   } or
+  TLoadTotalOverlapValueNumber(
+    IRFunction irFunc, IRType type, ValueNumber memOperand, ValueNumber operand
+  ) {
+    loadTotalOverlapValueNumber(_, irFunc, type, memOperand, operand)
+  } or
   TUniqueValueNumber(IRFunction irFunc, Instruction instr) { uniqueValueNumber(instr, irFunc) }
 
 /**
@@ -101,12 +106,18 @@ class ValueNumber extends TValueNumber {
  * The use of `p.x` on line 3 is linked to the definition of `p` on line 1 as well, but is not
  * congruent to that definition because `p.x` accesses only a subset of the memory defined by `p`.
  */
-private class CongruentCopyInstruction extends CopyInstruction {
+class CongruentCopyInstruction extends CopyInstruction {
   CongruentCopyInstruction() {
     this.getSourceValueOperand().getDefinitionOverlap() instanceof MustExactlyOverlap
   }
 }
 
+class LoadTotalOverlapInstruction extends LoadInstruction {
+  LoadTotalOverlapInstruction() {
+    this.getSourceValueOperand().getDefinitionOverlap() instanceof MustTotallyOverlap
+  }
+}
+
 /**
  * Holds if this library knows how to assign a value number to the specified instruction, other than
  * a `unique` value number that is never shared by multiple instructions.
@@ -131,6 +142,8 @@ private predicate numberableInstruction(Instruction instr) {
   instr instanceof PointerArithmeticInstruction
   or
   instr instanceof CongruentCopyInstruction
+  or
+  instr instanceof LoadTotalOverlapInstruction
 }
 
 private predicate variableAddressValueNumber(
@@ -205,6 +218,7 @@ private predicate unaryValueNumber(
   instr.getEnclosingIRFunction() = irFunc and
   not instr instanceof InheritanceConversionInstruction and
   not instr instanceof CopyInstruction and
+  not instr instanceof FieldAddressInstruction and
   instr.getOpcode() = opcode and
   instr.getResultIRType() = type and
   valueNumber(instr.getUnary()) = operand
@@ -221,6 +235,16 @@ private predicate inheritanceConversionValueNumber(
   valueNumber(instr.getUnary()) = operand
 }
 
+private predicate loadTotalOverlapValueNumber(
+  LoadTotalOverlapInstruction instr, IRFunction irFunc, IRType type, ValueNumber memOperand,
+  ValueNumber operand
+) {
+  instr.getEnclosingIRFunction() = irFunc and
+  instr.getResultIRType() = type and
+  valueNumber(instr.getAnOperand().(MemoryOperand).getAnyDef()) = memOperand and
+  valueNumberOfOperand(instr.getAnOperand().(AddressOperand)) = operand
+}
+
 /**
  * Holds if `instr` should be assigned a unique value number because this library does not know how
  * to determine if two instances of that instruction are equivalent.
@@ -313,6 +337,11 @@ private ValueNumber nonUniqueValueNumber(Instruction instr) {
           TPointerArithmeticValueNumber(irFunc, opcode, type, elementSize, leftOperand, rightOperand)
       )
       or
+      exists(IRType type, ValueNumber memOperand, ValueNumber operand |
+        loadTotalOverlapValueNumber(instr, irFunc, type, memOperand, operand) and
+        result = TLoadTotalOverlapValueNumber(irFunc, type, memOperand, operand)
+      )
+      or
       // The value number of a copy is just the value number of its source value.
       result = valueNumber(instr.(CongruentCopyInstruction).getSourceValue())
     )

csharp/ql/src/semmle/code/csharp/ir/implementation/unaliased_ssa/internal/SSAConstruction.qll

@@ -759,7 +759,21 @@ module DefUse {
       then defLocation = useLocation
       else (
         definitionHasPhiNode(defLocation, block) and
-        defLocation = useLocation.getVirtualVariable()
+        defLocation = useLocation.getVirtualVariable() and
+        // Handle the unusual case where a virtual variable does not overlap one of its member
+        // locations. For example, a definition of the virtual variable representing all aliased
+        // memory does not overlap a use of a string literal, because the contents of a string
+        // literal can never be redefined. The string literal's location could still be a member of
+        // the `AliasedVirtualVariable` due to something like:
+        // ```
+        // char s[10];
+        // strcpy(s, p);
+        // const char* p = b ? "SomeLiteral" : s;
+        // return p[3];
+        // ```
+        // In the above example, `p[3]` may access either the string literal or the local variable
+        // `s`, so both of those locations must be members of the `AliasedVirtualVariable`.
+        exists(Alias::getOverlap(defLocation, useLocation))
       )
     )
     or

csharp/ql/test/query-tests/Likely Bugs/DangerousNonShortCircuitLogic/DangerousNonShortCircuitLogic.cs

@@ -20,6 +20,9 @@ class Test
         var b = true;
         b &= c.Method(); // GOOD
         b |= c[0]; // GOOD
+
+        if (c == null | c.Method(out _)) ; // GOOD
+        if (c == null | (c.Method() | c.Method(out _))) ; // GOOD
     }
 
     class C
@@ -28,6 +31,7 @@ class Test
         public string Property { get; set; }
         public bool this[int i] { get { return false; } set { } }
         public bool Method() { return false; }
+        public bool Method(out int x) { x = 0; return false; }
     }
 }
 

java/ql/src/Security/CWE/CWE-113/NettyResponseSplitting.ql

@@ -29,5 +29,12 @@ private class InsecureDefaultHttpResponseClassInstantiation extends InsecureNett
   }
 }
 
+private class InsecureDefaultFullHttpResponseClassInstantiation extends InsecureNettyObjectCreation {
+  InsecureDefaultFullHttpResponseClassInstantiation() {
+    getConstructedType().hasQualifiedName("io.netty.handler.codec.http", "DefaultFullHttpResponse") and
+    getArgument(3).(CompileTimeConstantExpr).getBooleanValue() = false
+  }
+}
+
 from InsecureNettyObjectCreation new
 select new, "Response-splitting vulnerability due to header value verification being disabled."

java/ql/src/semmle/code/java/Expr.qll

@@ -167,6 +167,7 @@ class CompileTimeConstantExpr extends Expr {
   /**
    * Gets the string value of this expression, where possible.
    */
+  pragma[nomagic]
   string getStringValue() {
     result = this.(StringLiteral).getRepresentedString()
     or

java/ql/src/semmle/code/java/dataflow/internal/DataFlowImpl.qll

@@ -2041,12 +2041,12 @@ private class PathNodeMid extends PathNode, TPathNodeMid {
     result = getSuccMid()
     or
     // a final step to a sink via zero steps means we merge the last two steps to prevent trivial-looking edges
-    exists(PathNodeMid mid |
+    exists(PathNodeMid mid, PathNodeSink sink |
       mid = getSuccMid() and
-      mid.getNode() = result.getNode() and
+      mid.getNode() = sink.getNode() and
       mid.getAp() instanceof AccessPathNil and
-      result instanceof PathNodeSink and
-      result.getConfiguration() = unbind(mid.getConfiguration())
+      sink.getConfiguration() = unbind(mid.getConfiguration()) and
+      result = sink
     )
   }
 

java/ql/src/semmle/code/java/dataflow/internal/DataFlowImpl2.qll

@@ -2041,12 +2041,12 @@ private class PathNodeMid extends PathNode, TPathNodeMid {
     result = getSuccMid()
     or
     // a final step to a sink via zero steps means we merge the last two steps to prevent trivial-looking edges
-    exists(PathNodeMid mid |
+    exists(PathNodeMid mid, PathNodeSink sink |
       mid = getSuccMid() and
-      mid.getNode() = result.getNode() and
+      mid.getNode() = sink.getNode() and
       mid.getAp() instanceof AccessPathNil and
-      result instanceof PathNodeSink and
-      result.getConfiguration() = unbind(mid.getConfiguration())
+      sink.getConfiguration() = unbind(mid.getConfiguration()) and
+      result = sink
     )
   }
 

java/ql/src/semmle/code/java/dataflow/internal/DataFlowImpl3.qll

@@ -2041,12 +2041,12 @@ private class PathNodeMid extends PathNode, TPathNodeMid {
     result = getSuccMid()
     or
     // a final step to a sink via zero steps means we merge the last two steps to prevent trivial-looking edges
-    exists(PathNodeMid mid |
+    exists(PathNodeMid mid, PathNodeSink sink |
       mid = getSuccMid() and
-      mid.getNode() = result.getNode() and
+      mid.getNode() = sink.getNode() and
       mid.getAp() instanceof AccessPathNil and
-      result instanceof PathNodeSink and
-      result.getConfiguration() = unbind(mid.getConfiguration())
+      sink.getConfiguration() = unbind(mid.getConfiguration()) and
+      result = sink
     )
   }
 

java/ql/src/semmle/code/java/dataflow/internal/DataFlowImpl4.qll

@@ -2041,12 +2041,12 @@ private class PathNodeMid extends PathNode, TPathNodeMid {
     result = getSuccMid()
     or
     // a final step to a sink via zero steps means we merge the last two steps to prevent trivial-looking edges
-    exists(PathNodeMid mid |
+    exists(PathNodeMid mid, PathNodeSink sink |
       mid = getSuccMid() and
-      mid.getNode() = result.getNode() and
+      mid.getNode() = sink.getNode() and
       mid.getAp() instanceof AccessPathNil and
-      result instanceof PathNodeSink and
-      result.getConfiguration() = unbind(mid.getConfiguration())
+      sink.getConfiguration() = unbind(mid.getConfiguration()) and
+      result = sink
     )
   }
 

java/ql/src/semmle/code/java/dataflow/internal/DataFlowImpl5.qll

@@ -2041,12 +2041,12 @@ private class PathNodeMid extends PathNode, TPathNodeMid {
     result = getSuccMid()
     or
     // a final step to a sink via zero steps means we merge the last two steps to prevent trivial-looking edges
-    exists(PathNodeMid mid |
+    exists(PathNodeMid mid, PathNodeSink sink |
       mid = getSuccMid() and
-      mid.getNode() = result.getNode() and
+      mid.getNode() = sink.getNode() and
       mid.getAp() instanceof AccessPathNil and
-      result instanceof PathNodeSink and
-      result.getConfiguration() = unbind(mid.getConfiguration())
+      sink.getConfiguration() = unbind(mid.getConfiguration()) and
+      result = sink
     )
   }
 

java/ql/test/library-tests/dataflow/taint-ioutils/Test.java

@@ -0,0 +1,65 @@
+import java.io.FileInputStream;
+import java.io.InputStream;
+import java.io.InputStreamReader;
+import java.io.Reader;
+import java.io.StringWriter;
+import java.util.ArrayList;
+import java.util.List;
+import org.apache.commons.io.IOUtils;
+
+class Test {
+	public static void ioutils() {
+		InputStream inp = new FileInputStream("test"); // user input
+
+		InputStream buf = IOUtils.buffer(inp);
+		List<String> lines = IOUtils.readLines(inp, "UTF-8");
+		byte[] bytes = IOUtils.readFully(inp, 1000);
+		InputStream buf2 = IOUtils.toBufferedInputStream(inp);
+		Reader bufread = IOUtils.toBufferedReader(new InputStreamReader(inp));
+		byte[] bytes2 = IOUtils.toByteArray(inp, 1000);
+		char[] chars = IOUtils.toCharArray(inp, "UTF-8");
+		String s = IOUtils.toString(inp, "UTF-8");
+		InputStream is = IOUtils.toInputStream(s, "UTF-8");
+
+		StringWriter writer = new StringWriter();
+		writer.toString(); // not tainted
+		IOUtils.copy(inp, writer, "UTF-8");
+		writer.toString(); // tainted
+
+		writer = new StringWriter();
+		writer.toString(); // not tainted
+		IOUtils.copyLarge(bufread, writer);
+		writer.toString(); // tainted
+
+		byte x;
+		byte[] tgt = new byte[100];
+		x = tgt[0]; // not tainted
+		IOUtils.read(inp, tgt);
+		x = tgt[0]; // tainted
+
+		tgt = new byte[100];
+		x = tgt[0]; // not tainted
+		IOUtils.readFully(inp, tgt);
+		x = tgt[0]; // tainted
+
+		writer = new StringWriter();
+		writer.toString(); // not tainted
+		IOUtils.write(chars, writer);
+		writer.toString(); // tainted
+
+		writer = new StringWriter();
+		writer.toString(); // not tainted
+		IOUtils.writeChunked(chars, writer);
+		writer.toString(); // tainted
+
+		writer = new StringWriter();
+		writer.toString(); // not tainted
+		IOUtils.writeLines(lines, "\n", writer);
+		writer.toString(); // tainted
+
+		writer = new StringWriter();
+		writer.toString(); // not tainted
+		IOUtils.writeLines(new ArrayList<String>(), s, writer);
+		writer.toString(); // tainted
+	}
+}

java/ql/test/library-tests/dataflow/taint-ioutils/dataFlow.expected

@@ -0,0 +1,54 @@
+| Test.java:12:21:12:47 | new FileInputStream(...) |
+| Test.java:14:21:14:39 | buffer(...) |
+| Test.java:14:36:14:38 | inp |
+| Test.java:15:24:15:54 | readLines(...) |
+| Test.java:15:42:15:44 | inp |
+| Test.java:16:18:16:45 | readFully(...) |
+| Test.java:16:36:16:38 | inp |
+| Test.java:17:22:17:55 | toBufferedInputStream(...) |
+| Test.java:17:52:17:54 | inp |
+| Test.java:18:20:18:71 | toBufferedReader(...) |
+| Test.java:18:45:18:70 | new InputStreamReader(...) |
+| Test.java:18:67:18:69 | inp |
+| Test.java:19:19:19:48 | toByteArray(...) |
+| Test.java:19:39:19:41 | inp |
+| Test.java:20:18:20:50 | toCharArray(...) |
+| Test.java:20:38:20:40 | inp |
+| Test.java:21:14:21:43 | toString(...) |
+| Test.java:21:31:21:33 | inp |
+| Test.java:22:20:22:52 | toInputStream(...) |
+| Test.java:22:42:22:42 | s |
+| Test.java:26:16:26:18 | inp |
+| Test.java:26:21:26:26 | writer |
+| Test.java:27:3:27:8 | writer |
+| Test.java:27:3:27:19 | toString(...) |
+| Test.java:31:21:31:27 | bufread |
+| Test.java:31:30:31:35 | writer |
+| Test.java:32:3:32:8 | writer |
+| Test.java:32:3:32:19 | toString(...) |
+| Test.java:37:16:37:18 | inp |
+| Test.java:37:21:37:23 | tgt |
+| Test.java:38:3:38:12 | ...=... |
+| Test.java:38:7:38:9 | tgt |
+| Test.java:38:7:38:12 | ...[...] |
+| Test.java:42:21:42:23 | inp |
+| Test.java:42:26:42:28 | tgt |
+| Test.java:43:3:43:12 | ...=... |
+| Test.java:43:7:43:9 | tgt |
+| Test.java:43:7:43:12 | ...[...] |
+| Test.java:47:17:47:21 | chars |
+| Test.java:47:24:47:29 | writer |
+| Test.java:48:3:48:8 | writer |
+| Test.java:48:3:48:19 | toString(...) |
+| Test.java:52:24:52:28 | chars |
+| Test.java:52:31:52:36 | writer |
+| Test.java:53:3:53:8 | writer |
+| Test.java:53:3:53:19 | toString(...) |
+| Test.java:57:22:57:26 | lines |
+| Test.java:57:35:57:40 | writer |
+| Test.java:58:3:58:8 | writer |
+| Test.java:58:3:58:19 | toString(...) |
+| Test.java:62:47:62:47 | s |
+| Test.java:62:50:62:55 | writer |
+| Test.java:63:3:63:8 | writer |
+| Test.java:63:3:63:19 | toString(...) |

java/ql/test/library-tests/dataflow/taint-ioutils/dataFlow.ql

@@ -0,0 +1,15 @@
+import semmle.code.java.dataflow.DataFlow
+import semmle.code.java.dataflow.TaintTracking
+import semmle.code.java.dataflow.FlowSources
+
+class Conf extends TaintTracking::Configuration {
+  Conf() { this = "qltest:dataflow:ioutils" }
+
+  override predicate isSource(DataFlow::Node source) { source instanceof UserInput }
+
+  override predicate isSink(DataFlow::Node sink) { any() }
+}
+
+from UserInput u, DataFlow::Node e, Conf config
+where config.hasFlow(u, e) and e.getEnclosingCallable().hasName("ioutils")
+select e

java/ql/test/library-tests/dataflow/taint-ioutils/options

@@ -0,0 +1 @@
+//semmle-extractor-options: --javac-args -cp ${testdir}/../../../stubs/apache-commons-io-2.6

java/ql/test/library-tests/dataflow/taint/B.java

@@ -0,0 +1,159 @@
+import java.io.ByteArrayInputStream;
+import java.io.ByteArrayOutputStream;
+import java.io.IOException;
+import java.io.ObjectInputStream;
+import java.io.ObjectOutput;
+import java.io.ObjectOutputStream;
+import java.util.StringTokenizer;
+
+public class B {
+  public static String[] taint() { return new String[] { "tainted" }; }
+
+  public static void sink(Object o) { }
+
+  public static void maintest() {
+    String[] args = taint();
+    // tainted - access to main args
+    String[] aaaargs = args;
+    sink(aaaargs);
+    // tainted - access to tainted array
+    String s = args[0];
+    sink(s);
+    // tainted - concatenation of tainted string
+    String concat = "Look at me " + s + ", I'm tainted!";
+    sink(concat);
+    // tainted - parenthesised
+    String pars = (concat);
+    sink(pars);
+    // tainted method argument, implies tainted return value
+    String method = tainty(pars);
+    sink(method);
+    // tainted - complex
+    String complex = ("Look at me " + args[0]) + ", I'm tainted!";
+    sink(complex);
+    // tainted - data preserving constructors
+    String constructed = new String(complex);
+    sink(constructed);
+    // tainted - unsafe escape
+    String badEscape = constructed.replaceAll("(<script>)", "");
+    sink(badEscape);
+    // tainted - tokenized string
+    String token = new StringTokenizer(badEscape).nextToken();
+    sink(token);
+
+    // not tainted
+    String safe = notTainty(complex);
+    sink(safe);
+    String shouldBeFine = taintyOtherArg(safe, complex);
+    sink(shouldBeFine);
+    // non-whitelisted constructors don't pass taint
+    StringWrapper herring = new StringWrapper(complex);
+    sink(herring);
+
+    // tainted equality check with constant
+    boolean cond = "foo" == s;
+    sink(cond);
+    // tainted logic with tainted operand
+    boolean logic = cond && safe();
+    sink(logic);
+    // tainted condition
+    sink(concat.endsWith("I'm tainted"));
+    // tainted
+    logic = safe() || cond;
+    sink(logic);
+    // tainted, use of equals
+    logic = badEscape.equals("constant");
+    sink(logic);
+
+    // not tainted
+    boolean okay = s == shouldBeFine;
+    sink(okay);
+
+    // methods on string that pass on taint
+    String trimmed = s.trim();
+    sink(trimmed);
+    String[] split = s.split(" ");
+    sink(split);
+    String lower = s.toLowerCase();
+    sink(lower);
+    String upper = s.toUpperCase();
+    sink(upper);
+    byte[] bytes = s.getBytes("UTF-8");
+    sink(bytes);
+    String toString = s.toString();
+    sink(toString);
+    String subs = s.substring(1, 10);
+    sink(subs);
+    String repl = "some constant".replace(" ", s);
+    sink(repl);
+    String replAll = "some constant".replaceAll(" ", s);
+    sink(replAll);
+    String replFirst = "some constant".replaceFirst(" ", s);
+    sink(replFirst);
+
+    ByteArrayOutputStream baos = null;
+    ObjectOutput oos = null;
+    ByteArrayInputStream bais = null;
+    ObjectInputStream ois = null;
+    try
+    {
+      // serialization of tainted string
+      baos = new ByteArrayOutputStream();
+      oos = new ObjectOutputStream(baos);
+      oos.writeObject(s);
+      byte[] serializedData = baos.toByteArray(); // tainted
+      sink(serializedData);
+      // serialization of fixed string
+      baos = new ByteArrayOutputStream();
+      oos = new ObjectOutputStream(baos);
+      oos.writeObject("not tainted");
+      byte[] serializedData2 = baos.toByteArray(); // *not* tainted
+      sink(serializedData2);
+
+      // de-serialization of tainted string
+      bais = new ByteArrayInputStream(serializedData);
+      ois = new ObjectInputStream(bais);
+      String deserializedData = (String)ois.readObject(); // tainted
+      sink(deserializedData);
+    } catch (IOException e) {
+      // ignored in test code
+    } catch (ClassNotFoundException e) {
+      // ignored in test code
+    }
+
+    // tainted array initializers
+    String[] taintedArray = new String[] { s };
+    sink(taintedArray);
+    String[][] taintedArray2 = new String[][] { { s } };
+    sink(taintedArray2);
+    String[][][] taintedArray3 = new String[][][] { { { s } } };
+    sink(taintedArray3);
+
+    return;
+  }
+
+  public static String tainty(String arg) {
+    // tainted return value
+    return arg;
+  }
+
+  public static String taintyOtherArg(String safe, String tainted) {
+    return safe;
+  }
+
+  public static String notTainty(String arg) {
+    return "foo";
+  }
+
+  public static class StringWrapper {
+    public String wrapped;
+
+    public StringWrapper(String s) {
+      this.wrapped = s;
+    }
+  }
+
+  public static boolean safe() {
+    return true;
+  }
+}

java/ql/test/library-tests/dataflow/taint/MethodFlow.java

@@ -0,0 +1,38 @@
+public class MethodFlow {
+  public static String taint() { return "tainted"; }
+
+  public static void sink(String s) { }
+
+  public void test() {
+    String tainted = taint();
+    sink(tainted);
+    String tainted2 = notNull(taint());
+    sink(tainted2);
+    String tainted3 = wrapNotNull(taint());
+    sink(tainted3);
+
+    String safe = notNull("a constant");
+    sink(safe);
+
+    String diffString = returnDiffString(taint());
+    sink(diffString);
+  }
+
+  public <T> T notNull(T x) {
+    if (x == null) {
+      throw new NullPointerException();
+    }
+    return x;
+  }
+
+  public <T> T wrapNotNull(T x) {
+    T res = notNull(x);
+    sink("Logged: " + res);
+    return res;
+  }
+
+  public String returnDiffString(String x) {
+    sink("Received: " + x);
+    return "OK";
+  }
+}

java/ql/test/library-tests/dataflow/taint/StringBuilderTests.java

@@ -0,0 +1,66 @@
+public class StringBuilderTests {
+  public static String taint() { return "tainted"; }
+
+  public static void sink(String s) { }
+
+  static void stringBuilderBad() {
+    StringBuilder sb = new StringBuilder();
+    sb.append("from preferences select locale where user='");
+    sb.append(taint());
+    sb.append("'");
+    sink(sb.toString());
+  }
+
+  static void stringBuilderOkay() {
+    StringBuilder sb = new StringBuilder();
+    sb.append("from preferences select locale where user='");
+    sb.append("fred");
+    sb.append("'");
+    sink(sb.toString());
+  }
+
+  static void stringBufferBad() {
+    StringBuffer sb = new StringBuffer();
+    sb.append("from preferences select locale where user='");
+    sb.append(taint());
+    sb.append("'");
+    sink(sb.toString());
+  }
+
+  static void stringBuilderNoVarBad() {
+    sink(new StringBuilder()
+      .append("from preferences select locale where user='")
+      .append(taint())
+      .append("'").toString()
+    );
+  }
+
+  static void stringBuilderConstructorBad() {
+    StringBuilder sb = new StringBuilder(taint());
+    sb.append("from preferences select locale where user='");
+    sb.append("fred");
+    sb.append("'");
+    sink(sb.toString());
+  }
+
+  static void stringBuilderMultipleAppendsBad() {
+    StringBuilder sb = new StringBuilder();
+    sb.append("from preferences select locale where user='").append(taint());
+    sb.append("'");
+    sink(sb.toString());
+  }
+
+  static void stringBuilderReplaceBad() {
+    StringBuilder sb = new StringBuilder();
+    sb.append("from preferences select locale where user='placeholder'");
+    sb.replace(45, 57, taint());
+    sink(sb.toString());
+  }
+
+  static void stringBuilderInsertBad() {
+    StringBuilder sb = new StringBuilder();
+    sb.append("from preferences select locale where user=''");
+    sb.insert(45, taint());
+    sink(sb.toString());
+  }
+}

java/ql/test/library-tests/dataflow/taint/Varargs.java

@@ -0,0 +1,26 @@
+public class Varargs {
+  public String taint() { return "tainted"; }
+
+  public void sink(String s) { }
+
+  public void sources() {
+    f1(taint());
+    f2(taint(), taint());
+    f3(new String[] { taint(), "" });
+  }
+
+  public void f1(String... ss) {
+    String s = ss[0];
+    sink(s);
+  }
+
+  public void f2(String... ss) {
+    String s = ss[0];
+    sink(s);
+  }
+
+  public void f3(String... ss) {
+    String s = ss[0];
+    sink(s);
+  }
+}

java/ql/test/library-tests/dataflow/taint/test.expected

@@ -1,2 +1,47 @@
 | A.java:10:19:10:25 | taint(...) | A.java:15:10:15:11 | b2 |
 | A.java:20:19:20:25 | taint(...) | A.java:25:10:25:11 | b2 |
+| B.java:15:21:15:27 | taint(...) | B.java:18:10:18:16 | aaaargs |
+| B.java:15:21:15:27 | taint(...) | B.java:21:10:21:10 | s |
+| B.java:15:21:15:27 | taint(...) | B.java:24:10:24:15 | concat |
+| B.java:15:21:15:27 | taint(...) | B.java:27:10:27:13 | pars |
+| B.java:15:21:15:27 | taint(...) | B.java:30:10:30:15 | method |
+| B.java:15:21:15:27 | taint(...) | B.java:33:10:33:16 | complex |
+| B.java:15:21:15:27 | taint(...) | B.java:36:10:36:20 | constructed |
+| B.java:15:21:15:27 | taint(...) | B.java:39:10:39:18 | badEscape |
+| B.java:15:21:15:27 | taint(...) | B.java:42:10:42:14 | token |
+| B.java:15:21:15:27 | taint(...) | B.java:55:10:55:13 | cond |
+| B.java:15:21:15:27 | taint(...) | B.java:58:10:58:14 | logic |
+| B.java:15:21:15:27 | taint(...) | B.java:60:10:60:39 | endsWith(...) |
+| B.java:15:21:15:27 | taint(...) | B.java:63:10:63:14 | logic |
+| B.java:15:21:15:27 | taint(...) | B.java:66:10:66:14 | logic |
+| B.java:15:21:15:27 | taint(...) | B.java:74:10:74:16 | trimmed |
+| B.java:15:21:15:27 | taint(...) | B.java:76:10:76:14 | split |
+| B.java:15:21:15:27 | taint(...) | B.java:78:10:78:14 | lower |
+| B.java:15:21:15:27 | taint(...) | B.java:80:10:80:14 | upper |
+| B.java:15:21:15:27 | taint(...) | B.java:82:10:82:14 | bytes |
+| B.java:15:21:15:27 | taint(...) | B.java:84:10:84:17 | toString |
+| B.java:15:21:15:27 | taint(...) | B.java:86:10:86:13 | subs |
+| B.java:15:21:15:27 | taint(...) | B.java:88:10:88:13 | repl |
+| B.java:15:21:15:27 | taint(...) | B.java:90:10:90:16 | replAll |
+| B.java:15:21:15:27 | taint(...) | B.java:92:10:92:18 | replFirst |
+| B.java:15:21:15:27 | taint(...) | B.java:105:12:105:25 | serializedData |
+| B.java:15:21:15:27 | taint(...) | B.java:117:12:117:27 | deserializedData |
+| B.java:15:21:15:27 | taint(...) | B.java:126:10:126:21 | taintedArray |
+| B.java:15:21:15:27 | taint(...) | B.java:128:10:128:22 | taintedArray2 |
+| B.java:15:21:15:27 | taint(...) | B.java:130:10:130:22 | taintedArray3 |
+| MethodFlow.java:7:22:7:28 | taint(...) | MethodFlow.java:8:10:8:16 | tainted |
+| MethodFlow.java:9:31:9:37 | taint(...) | MethodFlow.java:10:10:10:17 | tainted2 |
+| MethodFlow.java:11:35:11:41 | taint(...) | MethodFlow.java:12:10:12:17 | tainted3 |
+| MethodFlow.java:11:35:11:41 | taint(...) | MethodFlow.java:30:10:30:25 | ... + ... |
+| MethodFlow.java:17:42:17:48 | taint(...) | MethodFlow.java:35:10:35:25 | ... + ... |
+| StringBuilderTests.java:9:15:9:21 | taint(...) | StringBuilderTests.java:11:10:11:22 | toString(...) |
+| StringBuilderTests.java:25:15:25:21 | taint(...) | StringBuilderTests.java:27:10:27:22 | toString(...) |
+| StringBuilderTests.java:33:15:33:21 | taint(...) | StringBuilderTests.java:31:10:34:29 | toString(...) |
+| StringBuilderTests.java:39:42:39:48 | taint(...) | StringBuilderTests.java:43:10:43:22 | toString(...) |
+| StringBuilderTests.java:48:69:48:75 | taint(...) | StringBuilderTests.java:50:10:50:22 | toString(...) |
+| StringBuilderTests.java:56:24:56:30 | taint(...) | StringBuilderTests.java:57:10:57:22 | toString(...) |
+| StringBuilderTests.java:63:19:63:25 | taint(...) | StringBuilderTests.java:64:10:64:22 | toString(...) |
+| Varargs.java:7:8:7:14 | taint(...) | Varargs.java:14:10:14:10 | s |
+| Varargs.java:8:8:8:14 | taint(...) | Varargs.java:19:10:19:10 | s |
+| Varargs.java:8:17:8:23 | taint(...) | Varargs.java:19:10:19:10 | s |
+| Varargs.java:9:23:9:29 | taint(...) | Varargs.java:24:10:24:10 | s |

java/ql/test/library-tests/dataflow/taintsources/A.java

@@ -0,0 +1,49 @@
+import java.io.FileInputStream;
+import java.io.IOException;
+import java.net.InetAddress;
+import java.net.MalformedURLException;
+import java.net.Socket;
+import java.net.URL;
+import java.sql.ResultSet;
+import java.sql.SQLException;
+import java.util.Properties;
+
+import javax.servlet.ServletException;
+import javax.servlet.http.HttpServlet;
+import javax.servlet.http.HttpServletRequest;
+import javax.servlet.http.HttpServletResponse;
+
+public class A {
+  public static void main(String[] args) {
+    String[] a = args; // user input
+    String s = args[0]; // user input
+  }
+
+  public static void userInput() throws SQLException, IOException, MalformedURLException {
+    System.getenv("test"); // user input
+    class TestServlet extends HttpServlet {
+      @Override
+      protected void doGet(HttpServletRequest req, HttpServletResponse resp)
+      throws ServletException, IOException {
+        req.getParameter("test"); // remote user input
+        req.getHeader("test"); // remote user input
+        req.getQueryString(); // remote user input
+        req.getCookies()[0].getValue(); // remote user input
+      }
+    }
+    new Properties().getProperty("test"); // user input
+    System.getProperty("test"); // user input
+    new Object() {
+      public void test(ResultSet rs) throws SQLException {
+        rs.getString(0); // user input
+      }
+    };
+    new URL("test").openConnection().getInputStream(); // remote user input
+    new Socket("test", 1234).getInputStream(); // remote user input
+    InetAddress.getByName("test").getHostName(); // remote user input
+
+    System.in.read(); // user input
+    new FileInputStream("test").read(); // user input
+  }
+
+}

java/ql/test/library-tests/dataflow/taintsources/RmiFlow.java

@@ -0,0 +1,8 @@
+package security.library.dataflow;
+
+import java.rmi.Remote;
+import java.rmi.RemoteException;
+
+public interface RmiFlow extends Remote {
+	String listDirectory(String path);
+}

java/ql/test/library-tests/dataflow/taintsources/RmiFlowImpl.java

@@ -0,0 +1,15 @@
+package security.library.dataflow;
+
+public class RmiFlowImpl implements RmiFlow {
+	public String listDirectory(String path) {
+		String command = "ls " + path;
+		Runtime.getRuntime().exec(command);
+		return "pretend there are some results here";
+	}
+
+	public String notRemotable(String path) {
+		String command = "ls " + path;
+		Runtime.getRuntime().exec(command);
+		return "pretend there are some results here";
+	}
+}

java/ql/test/library-tests/dataflow/taintsources/local.expected

@@ -0,0 +1,10 @@
+| A.java:17:27:17:39 | args | A.java:17:27:17:39 | args |
+| A.java:17:27:17:39 | args | A.java:18:18:18:21 | args |
+| A.java:17:27:17:39 | args | A.java:19:16:19:19 | args |
+| A.java:17:27:17:39 | args | A.java:19:16:19:22 | ...[...] |
+| A.java:23:5:23:25 | getenv(...) | A.java:23:5:23:25 | getenv(...) |
+| A.java:34:5:34:40 | getProperty(...) | A.java:34:5:34:40 | getProperty(...) |
+| A.java:35:5:35:30 | getProperty(...) | A.java:35:5:35:30 | getProperty(...) |
+| A.java:38:9:38:23 | getString(...) | A.java:38:9:38:23 | getString(...) |
+| A.java:45:5:45:13 | System.in | A.java:45:5:45:13 | System.in |
+| A.java:46:5:46:31 | new FileInputStream(...) | A.java:46:5:46:31 | new FileInputStream(...) |

java/ql/test/library-tests/dataflow/taintsources/local.ql

@@ -0,0 +1,18 @@
+import java
+import semmle.code.java.dataflow.TaintTracking
+import semmle.code.java.dataflow.FlowSources
+
+class Conf extends TaintTracking::Configuration {
+  Conf() { this = "remote taint conf" }
+
+  override predicate isSource(DataFlow::Node n) {
+    n instanceof UserInput and
+    not n instanceof RemoteFlowSource
+  }
+
+  override predicate isSink(DataFlow::Node n) { any() }
+}
+
+from DataFlow::Node src, DataFlow::Node sink, Conf conf
+where conf.hasFlow(src, sink)
+select src, sink

java/ql/test/library-tests/dataflow/taintsources/options

@@ -0,0 +1 @@
+//semmle-extractor-options: --javac-args -cp ${testdir}/../../../stubs/servlet-api-2.4

java/ql/test/library-tests/dataflow/taintsources/remote.expected

@@ -0,0 +1,11 @@
+| A.java:28:9:28:32 | getParameter(...) | A.java:28:9:28:32 | getParameter(...) |
+| A.java:29:9:29:29 | getHeader(...) | A.java:29:9:29:29 | getHeader(...) |
+| A.java:30:9:30:28 | getQueryString(...) | A.java:30:9:30:28 | getQueryString(...) |
+| A.java:31:9:31:38 | getValue(...) | A.java:31:9:31:38 | getValue(...) |
+| A.java:41:5:41:53 | getInputStream(...) | A.java:41:5:41:53 | getInputStream(...) |
+| A.java:42:5:42:45 | getInputStream(...) | A.java:42:5:42:45 | getInputStream(...) |
+| A.java:43:5:43:47 | getHostName(...) | A.java:43:5:43:47 | getHostName(...) |
+| RmiFlowImpl.java:4:30:4:40 | path | RmiFlowImpl.java:4:30:4:40 | path |
+| RmiFlowImpl.java:4:30:4:40 | path | RmiFlowImpl.java:5:20:5:31 | ... + ... |
+| RmiFlowImpl.java:4:30:4:40 | path | RmiFlowImpl.java:5:28:5:31 | path |
+| RmiFlowImpl.java:4:30:4:40 | path | RmiFlowImpl.java:6:29:6:35 | command |

java/ql/test/library-tests/dataflow/taintsources/remote.ql

@@ -0,0 +1,15 @@
+import java
+import semmle.code.java.dataflow.TaintTracking
+import semmle.code.java.dataflow.FlowSources
+
+class Conf extends TaintTracking::Configuration {
+  Conf() { this = "remote taint conf" }
+
+  override predicate isSource(DataFlow::Node n) { n instanceof RemoteFlowSource }
+
+  override predicate isSink(DataFlow::Node n) { any() }
+}
+
+from DataFlow::Node src, DataFlow::Node sink, Conf conf
+where conf.hasFlow(src, sink)
+select src, sink

java/ql/test/stubs/apache-commons-io-2.6/LICENSE.txt

@@ -0,0 +1,14 @@
+Licensed to the Apache Software Foundation (ASF) under one or more
+contributor license agreements.  See the NOTICE file distributed with
+this work for additional information regarding copyright ownership.
+The ASF licenses this file to You under the Apache License, Version 2.0
+(the "License"); you may not use this file except in compliance with
+the License.  You may obtain a copy of the License at
+
+     http://www.apache.org/licenses/LICENSE-2.0
+
+Unless required by applicable law or agreed to in writing, software
+distributed under the License is distributed on an "AS IS" BASIS,
+WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+See the License for the specific language governing permissions and
+limitations under the License.

java/ql/test/stubs/apache-commons-io-2.6/org/apache/commons/io/IOUtils.java

@@ -0,0 +1,22 @@
+package org.apache.commons.io;
+import java.io.*;
+import java.util.*;
+
+public class IOUtils {
+  public static BufferedInputStream buffer(InputStream inputStream) { return null; }
+  public static void copy(InputStream input, Writer output, String inputEncoding) throws IOException { }
+  public static long copyLarge(Reader input, Writer output) throws IOException { return 42; }
+  public static int read(InputStream input, byte[] buffer) throws IOException { return 42; }
+  public static void readFully(InputStream input, byte[] buffer) throws IOException { }
+  public static byte[] readFully(InputStream input, int length) throws IOException { return null; }
+  public static List<String> readLines(InputStream input, String encoding) throws IOException { return null; }
+  public static InputStream toBufferedInputStream(InputStream input) throws IOException { return null; }
+  public static BufferedReader toBufferedReader(Reader reader) { return null; }
+  public static byte[] toByteArray(InputStream input, int size) throws IOException { return null; }
+  public static char[] toCharArray(InputStream is, String encoding) throws IOException { return null; }
+  public static InputStream toInputStream(String input, String encoding) throws IOException { return null; }
+  public static String toString(InputStream input, String encoding) throws IOException { return null; }
+  public static void write(char[] data, Writer output) throws IOException { }
+  public static void writeChunked(char[] data, Writer output) throws IOException { }
+  public static void writeLines(Collection<?> lines, String lineEnding, Writer writer) throws IOException { }
+}