Address feedback

Author: knewbury01

cpp/common/src/codingstandards/cpp/lifetimes/CppObjects.qll

@@ -2,6 +2,7 @@ import cpp
 import codingstandards.cpp.lifetimes.StorageDuration
 import semmle.code.cpp.valuenumbering.HashCons
 import codingstandards.cpp.Clvalues
+import codingstandards.cpp.types.Pointers
 
 /**
  * A library for handling "Objects" in C++.
@@ -136,19 +137,39 @@ abstract class ObjectIdentityBase extends Element {
 }
 
 /**
- * Finds expressions `e.x` or `e[x]` for expression `e`, recursively. Does not resolve pointers.
+ * Finds expressions `e.x` or `e[x]` for expression `e`, recursively.
+ *
+ * Omits accesses to reference fields, as they are not subobjects.
  *
  * Note that this does not hold for `e->x` or `e[x]` where `e` is a pointer.
  */
-private Expr getASubobjectAccessOf(Expr e) {
-  result = e
-  or
-  result.(DotFieldAccess).getQualifier() = getASubobjectAccessOf(e)
+Expr getASubobjectAccessOf(Expr e) {
+  exists(Field f |
+    not f.getUnderlyingType() instanceof ReferenceType and
+    f.getAnAccess().(FieldAccess) = result.getAChild*()
+  ) and
+  (
+    result = e
+    or
+    result.(DotFieldAccess).getQualifier() = getASubobjectAccessOf(e)
+  )
   or
   result.(ArrayExpr).getArrayBase() = getASubobjectAccessOf(e) and
   not result.(ArrayExpr).getArrayBase().getUnspecifiedType() instanceof PointerType
 }
 
+/**
+ * gets an access where the pointee is the subobject
+ */
+Expr getASubobjectAccessOfPointee(Expr e) {
+  e.getParent() instanceof AddressOfExpr and
+  result = getASubobjectAccessOf(e.getParent())
+  or
+  // the accessed field is a pointer to a subobject
+  e.getParent().(PointerFieldAccess).getTarget().getUnspecifiedType() instanceof PointerType and
+  result = getASubobjectAccessOf(e.getParent())
+}
+
 /**
  * Find the object types that are embedded within the current type.
  *

cpp/misra/src/rules/RULE-6-8-4/MemberFunctionsRefqualified.ql

@@ -17,54 +17,66 @@ import cpp
 import codingstandards.cpp.misra
 import codingstandards.cpp.types.Compatible
 import codingstandards.cpp.Operator
+import codingstandards.cpp.types.Pointers
+import codingstandards.cpp.lifetimes.CppObjects
 
 class MembersReturningPointerOrRef extends MemberFunction {
-  MembersReturningPointerOrRef() {
-    this.getUnspecifiedType() instanceof PointerType or
-    this.getUnspecifiedType() instanceof ReferenceType
-  }
+  MembersReturningPointerOrRef() { this.getType() instanceof PointerLikeType }
 }
 
 abstract class MembersReturningObjectOrSubobject extends MembersReturningPointerOrRef {
   string toString() { result = "Members returning object or subobject" }
 }
 
+/**
+ * Members that have an explicit `this` access within their return statement.
+ */
 class MembersReturningObject extends MembersReturningObjectOrSubobject {
   MembersReturningObject() {
     exists(ReturnStmt r, ThisExpr t |
       r.getEnclosingFunction() = this and
       (
-        r.getAChild*() = t
+        //direct access only
+        r.getAChild() = t
         or
+        //or one level of indirection
         exists(PointerDereferenceExpr p |
-          p.getAChild*() = t and
-          r.getAChild*() = p
+          p.getAChild() = t and
+          r.getAChild() = p
         )
       ) and
       t.getActualType().stripType() = this.getDeclaringType()
     )
   }
 }
 
+/**
+ * Members that have an explicit subobject access within their return statement.
+ *
+ * Specifically, this captures when the return is a reference or pointer
+ * to a subobject.
+ */
 class MembersReturningSubObject extends MembersReturningObjectOrSubobject {
   MembersReturningSubObject() {
-    exists(ReturnStmt r, FieldSubObjectDeclaration field |
+    exists(ReturnStmt r, FieldAccess access, Expr e |
       r.getEnclosingFunction() = this and
+      //direct access only
+      r.getAChild() = e and
       (
-        r.getAChild*() = field.(Field).getAnAccess()
+        //pointer or reference to pointer subobject returned
+        e = getASubobjectAccessOfPointee(access) and
+        (e.getType() instanceof PointerType or e.getType() instanceof ReferenceType)
         or
-        exists(PointerDereferenceExpr p |
-          p.getAChild*() = field.(Field).getAnAccess() and
-          r.getAChild*() = p
-        )
-      ) and
-      field.(Field).getDeclaringType() = this.getDeclaringType()
+        //reference to subobject returned
+        (this.getType() instanceof ReferenceType or e.getType() instanceof ReferenceType) and
+        not access.getTarget().getType() instanceof PointerType
+      )
     )
   }
 }
 
 predicate relevantTypes(Type a, Type b) {
-  exists(MembersReturningObject f, MemberFunction overload |
+  exists(MembersReturningObjectOrSubobject f, MemberFunction overload |
     f.getAnOverload() = overload and
     exists(int i |
       f.getParameter(i).getType() = a and
@@ -77,11 +89,11 @@ class AppropriatelyQualified extends MembersReturningObjectOrSubobject {
   AppropriatelyQualified() {
     //non-const-lvalue-ref-qualified
     this.isLValueRefQualified() and
-    not this.hasSpecifier("const")
+    this.getType().(PointerLikeType).pointsToNonConst()
     or
     //const-lvalue-ref-qualified
     this.isLValueRefQualified() and
-    this.hasSpecifier("const") and
+    this.getType().(PointerLikeType).pointsToConst() and
     //and overload exists that is rvalue-ref-qualified
     exists(MemberFunction overload |
       this.getAnOverload() = overload and
@@ -95,16 +107,6 @@ class AppropriatelyQualified extends MembersReturningObjectOrSubobject {
   }
 }
 
-/**
- * Fields that are not reference type can be subobjects
- */
-class FieldSubObjectDeclaration extends Declaration {
-  FieldSubObjectDeclaration() {
-    not this.getADeclarationEntry().getType() instanceof ReferenceType and
-    this instanceof Field
-  }
-}
-
 class DefaultedAssignmentOperator extends AssignmentOperator {
   DefaultedAssignmentOperator() { this.isDefaulted() }
 }

cpp/misra/test/rules/RULE-6-8-4/MemberFunctionsRefqualified.expected

@@ -4,3 +4,17 @@
 | test.cpp:42:16:42:16 | Members returning object or subobject | Member function is not properly ref qualified. |
 | test.cpp:42:16:42:16 | Members returning object or subobject | Member function is not properly ref qualified. |
 | test.cpp:42:16:42:16 | Members returning object or subobject | Member function is not properly ref qualified. |
+| test.cpp:61:8:61:8 | Members returning object or subobject | Member function is not properly ref qualified. |
+| test.cpp:71:9:71:10 | Members returning object or subobject | Member function is not properly ref qualified. |
+| test.cpp:79:8:79:9 | Members returning object or subobject | Member function is not properly ref qualified. |
+| test.cpp:82:8:82:9 | Members returning object or subobject | Member function is not properly ref qualified. |
+| test.cpp:85:8:85:9 | Members returning object or subobject | Member function is not properly ref qualified. |
+| test.cpp:89:9:89:10 | Members returning object or subobject | Member function is not properly ref qualified. |
+| test.cpp:93:9:93:10 | Members returning object or subobject | Member function is not properly ref qualified. |
+| test.cpp:103:8:103:8 | Members returning object or subobject | Member function is not properly ref qualified. |
+| test.cpp:113:9:113:10 | Members returning object or subobject | Member function is not properly ref qualified. |
+| test.cpp:121:8:121:9 | Members returning object or subobject | Member function is not properly ref qualified. |
+| test.cpp:124:8:124:9 | Members returning object or subobject | Member function is not properly ref qualified. |
+| test.cpp:127:8:127:9 | Members returning object or subobject | Member function is not properly ref qualified. |
+| test.cpp:131:9:131:10 | Members returning object or subobject | Member function is not properly ref qualified. |
+| test.cpp:135:9:135:10 | Members returning object or subobject | Member function is not properly ref qualified. |

cpp/misra/test/rules/RULE-6-8-4/test.cpp

@@ -51,4 +51,88 @@ void f(int p, float p1) {
   t.f(p);
   t.f(p1); // instantiation that causes issue due to parameter list type meaning
            // there is no overload
-}
+}
+
+class B {
+  int i;
+  int *ip;
+  int **ip2;
+
+  int *f() {
+    return &i; // NON_COMPLIANT
+  }
+  int *f1() {
+    return ip; // COMPLIANT -- reads pointer
+  }
+  int *f2() {
+    return *ip2; // COMPLIANT -- reads pointer
+  }
+
+  int **f3() {
+    return &ip; // NON_COMPLIANT
+  }
+
+  int **f4() {
+    return ip2; // COMPLIANT -- copies pointer
+  }
+
+  int &f5() {
+    return i; // NON_COMPLIANT
+  }
+  int &f6() {
+    return *ip; // COMPLIANT[FALSE_POSITIVE] -- reads pointer
+  }
+  int &f7() {
+    return **ip2; // COMPLIANT[FALSE_POSITIVE]-- reads pointer
+  }
+
+  int *&f8() {
+    // return &p; // won't compile
+    return ip; // NON_COMPLIANT
+  }
+  int *&f9() {
+    return *ip2; // COMPLIANT[FALSE_POSITIVE] -- reads pointer
+  }
+};
+
+class D {
+  int i;
+  int *ip;
+  int **ip2;
+
+  int *f() {
+    return &(this->i); // NON_COMPLIANT
+  }
+  int *f1() {
+    return this->ip; // COMPLIANT -- reads pointer
+  }
+  int *f2() {
+    return *this->ip2; // COMPLIANT -- reads pointer
+  }
+
+  int **f3() {
+    return &this->ip; // NON_COMPLIANT
+  }
+
+  int **f4() {
+    return this->ip2; // COMPLIANT -- copies pointer
+  }
+
+  int &f5() {
+    return this->i; // NON_COMPLIANT
+  }
+  int &f6() {
+    return *this->ip; // COMPLIANT[FALSE_POSITIVE] -- reads pointer
+  }
+  int &f7() {
+    return **this->ip2; // COMPLIANT[FALSE_POSITIVE] -- reads pointer
+  }
+
+  int *&f8() {
+    // return &p; // won't compile
+    return this->ip; // NON_COMPLIANT
+  }
+  int *&f9() {
+    return *this->ip2; // COMPLIANT[FALSE_POSITIVE] -- reads pointer
+  }
+};