Merge pull request #18629 from MathiasVP/fix-more-fps-in-buffer-overflow

C++: Fix more FPs in `cpp/overflow-buffer`

cpp/ql/lib/change-notes/2025-01-30-getOffsetInClass.md

@@ -0,0 +1,4 @@
+---
+category: feature
+---
+* A new predicate `getOffsetInClass` was added to the `Field` class, which computes the byte offset of a field relative to a given `Class`.

cpp/ql/lib/semmle/code/cpp/Field.qll

@@ -5,6 +5,30 @@
 import semmle.code.cpp.Variable
 import semmle.code.cpp.Enum
 
+private predicate hasAFieldWithOffset(Class c, Field f, int offset) {
+  // Base case: `f` is a field in `c`.
+  f = c.getAField() and
+  offset = f.getByteOffset() and
+  not f.getUnspecifiedType().(Class).hasDefinition()
+  or
+  // Otherwise, we find the struct that is a field of `c` which then has
+  // the field `f` as a member.
+  exists(Field g |
+    g = c.getAField() and
+    // Find the field with the largest offset that's less than or equal to
+    // offset. That's the struct we need to search recursively.
+    g =
+      max(Field cand, int candOffset |
+        cand = c.getAField() and
+        candOffset = cand.getByteOffset() and
+        offset >= candOffset
+      |
+        cand order by candOffset
+      ) and
+    hasAFieldWithOffset(g.getUnspecifiedType(), f, offset - g.getByteOffset())
+  )
+}
+
 /**
  * A C structure member or C++ non-static member variable. For example the
  * member variable `m` in the following code (but not `s`):
@@ -76,6 +100,27 @@ class Field extends MemberVariable {
         rank[result + 1](int index | cls.getCanonicalMember(index).(Field).isInitializable())
     )
   }
+
+  /**
+   * Gets the offset (in bytes) of this field starting at `c`.
+   *
+   * For example, consider:
+   * ```cpp
+   * struct S1 {
+   *   int a;
+   *   void* b;
+   * };
+   *
+   * struct S2 {
+   *   S1 s1;
+   *   char c;
+   * };
+   * ```
+   * If `f` represents the field `s1` and `c` represents the class `S2` then
+   * `f.getOffsetInClass(S2) = 0` holds. Likewise, if `f` represents the
+   * field `a`, then `f.getOffsetInClass(c) = 0` holds.
+   */
+  int getOffsetInClass(Class c) { hasAFieldWithOffset(c, this, result) }
 }
 
 /**

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

@@ -24,6 +24,74 @@ predicate memberMayBeVarSize(Class c, MemberVariable v) {
   exists(ArrayType t | t = v.getUnspecifiedType() | not t.getArraySize() > 1)
 }
 
+/**
+ * Given a chain of accesses of the form `x.f1.f2...fn` this
+ * predicate gives the type of `x`. Note that `x` may be an implicit
+ * `this` expression.
+ */
+private Class getRootType(FieldAccess fa) {
+  // If the object is accessed inside a member function then the root will
+  // be a(n implicit) `this`. And the root type will be the type of `this`.
+  exists(VariableAccess root |
+    root = fa.getQualifier*() and
+    result =
+      root.getQualifier()
+          .(ThisExpr)
+          .getUnspecifiedType()
+          .(PointerType)
+          .getBaseType()
+          .getUnspecifiedType()
+  )
+  or
+  // Otherwise, if this is not inside a member function there will not be
+  // a(n implicit) `this`. And the root type is the type of the outermost
+  // access.
+  exists(VariableAccess root |
+    root = fa.getQualifier+() and
+    not exists(root.getQualifier()) and
+    result = root.getUnspecifiedType()
+  )
+}
+
+/**
+ * Gets the size of the buffer access at `va`.
+ */
+private int getSize(VariableAccess va) {
+  exists(Variable v | va.getTarget() = v |
+    // If `v` is not a field then the size of the buffer is just
+    // the size of the type of `v`.
+    exists(Type t |
+      t = v.getUnspecifiedType() and
+      not v instanceof Field and
+      not t instanceof ReferenceType and
+      result = t.getSize()
+    )
+    or
+    exists(Class c |
+      // Otherwise, we find the "outermost" object and compute the size
+      // as the difference between the size of the type of the "outermost
+      // object" and the offset of the field relative to that type.
+      // For example, consider the following structs:
+      // ```
+      // struct S {
+      //   uint32_t x;
+      //   uint32_t y;
+      // };
+      // struct S2 {
+      //   S s;
+      //   uint32_t z;
+      // };
+      // ```
+      // Given an object `S2 s2` the size of the buffer `&s2.s.y`
+      // is the size of the base object type (i.e., `S2`) minutes the offset
+      // of `y` relative to the type `S2` (i.e., `4`). So the size of the
+      // buffer is `12 - 4 = 8`.
+      c = getRootType(va) and
+      result = c.getSize() - v.(Field).getOffsetInClass(c)
+    )
+  )
+}
+
 /**
  * Holds if `bufferExpr` is an allocation-like expression.
  *
@@ -54,37 +122,11 @@ private int isSource(Expr bufferExpr, Element why) {
   result = bufferExpr.(AllocationExpr).getSizeBytes() and
   why = bufferExpr
   or
-  exists(Type bufferType, Variable v |
+  exists(Variable v |
     v = why and
     // buffer is the address of a variable
     why = bufferExpr.(AddressOfExpr).getAddressable() and
-    bufferType = v.getUnspecifiedType() and
-    not bufferType instanceof ReferenceType and
-    not any(Union u).getAMemberVariable() = why
-  |
-    not v instanceof Field and
-    result = bufferType.getSize()
-    or
-    // If it's an address of a field (i.e., a non-static member variable)
-    // then it's okay to use that address to access the other member variables.
-    // For example, this is okay:
-    // ```
-    // struct S { uint8_t a, b, c; };
-    // S s;
-    // memset(&s.a, 0, sizeof(S) - offsetof(S, a));
-    exists(Field f |
-      v = f and
-      result = f.getDeclaringType().getSize() - f.getByteOffset()
-    )
-  )
-  or
-  exists(Union bufferType |
-    // buffer is the address of a union member; in this case, we
-    // take the size of the union itself rather the union member, since
-    // it's usually OK to access that amount (e.g. clearing with memset).
-    why = bufferExpr.(AddressOfExpr).getAddressable() and
-    bufferType.getAMemberVariable() = why and
-    result = bufferType.getSize()
+    result = getSize(bufferExpr.(AddressOfExpr).getOperand())
   )
 }