Merge pull request github#15559 from MathiasVP/fix-constness-type

C++: Don't strip specifiers in `Node.getType`

Author: MathiasVP

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

@@ -709,7 +709,7 @@ class FinalGlobalValue extends Node, TFinalGlobalValue {
   override DataFlowType getType() {
     exists(int indirectionIndex |
       indirectionIndex = globalUse.getIndirectionIndex() and
-      result = getTypeImpl(globalUse.getUnspecifiedType(), indirectionIndex - 1)
+      result = getTypeImpl(globalUse.getUnderlyingType(), indirectionIndex - 1)
     )
   }
 
@@ -740,7 +740,7 @@ class InitialGlobalValue extends Node, TInitialGlobalValue {
 
   override DataFlowType getType() {
     exists(DataFlowType type |
-      type = globalDef.getUnspecifiedType() and
+      type = globalDef.getUnderlyingType() and
       if this.isGLValue()
       then result = type
       else result = getTypeImpl(type, globalDef.getIndirectionIndex() - 1)
@@ -943,10 +943,13 @@ private Type getTypeImpl0(Type t, int indirectionIndex) {
   indirectionIndex > 0 and
   exists(Type stripped |
     stripped = stripPointer(t.stripTopLevelSpecifiers()) and
-    // We need to avoid the case where `stripPointer(t) = t` (which can happen on
-    // iterators that specify a `value_type` that is the iterator itself). Such a type
-    // would create an infinite loop otherwise. For these cases we simply don't produce
-    // a result for `getTypeImpl`.
+    // We need to avoid the case where `stripPointer(t) = t` (which can happen
+    // on iterators that specify a `value_type` that is the iterator itself).
+    // Such a type would create an infinite loop otherwise. For these cases we
+    // simply don't produce a result for `getTypeImpl`.
+    // To be on the safe side, we check whether the _unspecified_ type has
+    // changed since this also prevents an infinite loop when `stripped` and
+    // `t` only differ by const'ness or volatile'ness.
     stripped.getUnspecifiedType() != t.getUnspecifiedType() and
     result = getTypeImpl0(stripped, indirectionIndex - 1)
   )
@@ -996,12 +999,14 @@ private module RawIndirectNodes {
 
     override Declaration getEnclosingCallable() { result = this.getFunction() }
 
+    override predicate isGLValue() { this.getOperand().isGLValue() }
+
     override DataFlowType getType() {
       exists(int sub, DataFlowType type, boolean isGLValue |
         type = getOperandType(this.getOperand(), isGLValue) and
         if isGLValue = true then sub = 1 else sub = 0
       |
-        result = getTypeImpl(type.getUnspecifiedType(), indirectionIndex - sub)
+        result = getTypeImpl(type.getUnderlyingType(), indirectionIndex - sub)
       )
     }
 
@@ -1038,12 +1043,14 @@ private module RawIndirectNodes {
 
     override Declaration getEnclosingCallable() { result = this.getFunction() }
 
+    override predicate isGLValue() { this.getInstruction().isGLValue() }
+
     override DataFlowType getType() {
       exists(int sub, DataFlowType type, boolean isGLValue |
         type = getInstructionType(this.getInstruction(), isGLValue) and
         if isGLValue = true then sub = 1 else sub = 0
       |
-        result = getTypeImpl(type.getUnspecifiedType(), indirectionIndex - sub)
+        result = getTypeImpl(type.getUnderlyingType(), indirectionIndex - sub)
       )
     }
 
@@ -1136,7 +1143,7 @@ class FinalParameterNode extends Node, TFinalParameterNode {
 
   override Declaration getEnclosingCallable() { result = this.getFunction() }
 
-  override DataFlowType getType() { result = getTypeImpl(p.getUnspecifiedType(), indirectionIndex) }
+  override DataFlowType getType() { result = getTypeImpl(p.getUnderlyingType(), indirectionIndex) }
 
   final override Location getLocationImpl() {
     // Parameters can have multiple locations. When there's a unique location we use
@@ -1789,7 +1796,7 @@ class VariableNode extends Node, TVariableNode {
   }
 
   override DataFlowType getType() {
-    result = getTypeImpl(v.getUnspecifiedType(), indirectionIndex - 1)
+    result = getTypeImpl(v.getUnderlyingType(), indirectionIndex - 1)
   }
 
   final override Location getLocationImpl() {

cpp/ql/lib/semmle/code/cpp/ir/dataflow/internal/SsaInternals.qll

@@ -548,6 +548,11 @@ class GlobalUse extends UseImpl, TGlobalUse {
    */
   Type getUnspecifiedType() { result = global.getUnspecifiedType() }
 
+  /**
+   * Gets the type of this use, after typedefs have been resolved.
+   */
+  Type getUnderlyingType() { result = global.getUnderlyingType() }
+
   override predicate isCertain() { any() }
 
   override BaseSourceVariableInstruction getBase() { none() }
@@ -591,11 +596,16 @@ class GlobalDefImpl extends DefOrUseImpl, TGlobalDefImpl {
   int getIndirection() { result = indirectionIndex }
 
   /**
-   * Gets the type of this use after specifiers have been deeply stripped
-   * and typedefs have been resolved.
+   * Gets the type of this definition after specifiers have been deeply
+   * stripped and typedefs have been resolved.
    */
   Type getUnspecifiedType() { result = global.getUnspecifiedType() }
 
+  /**
+   * Gets the type of this definition, after typedefs have been resolved.
+   */
+  Type getUnderlyingType() { result = global.getUnderlyingType() }
+
   override string toString() { result = "Def of " + this.getSourceVariable() }
 
   override Location getLocation() { result = f.getLocation() }
@@ -1115,6 +1125,11 @@ class GlobalDef extends TGlobalDef, SsaDefOrUse {
    */
   DataFlowType getUnspecifiedType() { result = global.getUnspecifiedType() }
 
+  /**
+   * Gets the type of this definition, after typedefs have been resolved.
+   */
+  DataFlowType getUnderlyingType() { result = global.getUnderlyingType() }
+
   /** Gets the `IRFunction` whose body is evaluated after this definition. */
   IRFunction getIRFunction() { result = global.getIRFunction() }
 

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

@@ -1,3 +1,18 @@
 astTypeBugs
 irTypeBugs
+incorrectBaseType
+| clang.cpp:22:8:22:20 | *& ... | Expected 'Node.getType()' to be int, but it was int * |
+| clang.cpp:23:17:23:29 | *& ... | Expected 'Node.getType()' to be int, but it was int * |
+| flowOut.cpp:50:14:50:15 | *& ... | Expected 'Node.getType()' to be int, but it was int * |
+| flowOut.cpp:84:9:84:10 | *& ... | Expected 'Node.getType()' to be int, but it was int * |
+| flowOut.cpp:101:13:101:14 | *& ... | Expected 'Node.getType()' to be int, but it was int * |
+| self_parameter_flow.cpp:8:8:8:9 | *& ... | Expected 'Node.getType()' to be unsigned char, but it was unsigned char * |
+| test.cpp:67:28:67:37 | (reference dereference) | Expected 'Node.getType()' to be const int, but it was int * |
+| test.cpp:531:39:531:40 | *& ... | Expected 'Node.getType()' to be int, but it was const int * |
+| test.cpp:615:13:615:21 | *& ... | Expected 'Node.getType()' to be int, but it was void |
+| test.cpp:704:22:704:25 | *& ... | Expected 'Node.getType()' to be int, but it was int * |
+| test.cpp:715:24:715:25 | *& ... | Expected 'Node.getType()' to be unsigned char, but it was unsigned char * |
+| test.cpp:848:23:848:25 | (reference dereference) | Expected 'Node.getType()' to be int, but it was int * |
+| test.cpp:854:10:854:36 | * ... | Expected 'Node.getType()' to be const int, but it was int |
+| test.cpp:867:10:867:30 | * ... | Expected 'Node.getType()' to be const int, but it was int |
 failures

cpp/ql/test/library-tests/dataflow/dataflow-tests/type-bugs.ql

@@ -25,6 +25,17 @@ module IrTest {
       n != 1
     )
   }
+
+  query predicate incorrectBaseType(Node n, string msg) {
+    exists(PointerType pointerType, Type nodeType, Type baseType |
+      not n.isGLValue() and
+      pointerType = n.asIndirectExpr(1).getActualType() and
+      baseType = pointerType.getBaseType() and
+      nodeType = n.getType() and
+      nodeType != baseType and
+      msg = "Expected 'Node.getType()' to be " + baseType + ", but it was " + nodeType
+    )
+  }
 }
 
 import IrTest