Fix MSC39-C after review

Author: mbaluda

c/cert/src/rules/MSC39-C/DoNotCallVaArgOnAVaListThatHasAnIndeterminateValue.ql

@@ -15,13 +15,22 @@ import codingstandards.c.cert
 import codingstandards.cpp.Macro
 import semmle.code.cpp.dataflow.DataFlow
 
+abstract class VaAccess extends Expr { }
+
 /**
  * The argument of a call to `va_arg`
  */
-class VaArgArg extends Expr {
+class VaArgArg extends VaAccess {
   VaArgArg() { this = any(MacroInvocation m | m.getMacroName() = ["va_arg"]).getExpr().getChild(0) }
 }
 
+/**
+ * The argument of a call to `va_end`
+ */
+class VaEndArg extends VaAccess {
+  VaEndArg() { this = any(MacroInvocation m | m.getMacroName() = ["va_end"]).getExpr().getChild(0) }
+}
+
 /**
  * Dataflow configuration for flow from a library function
  * to a call of function `asctime`
@@ -34,13 +43,13 @@ class VaArgConfig extends DataFlow::Configuration {
       any(VariableDeclarationEntry m | m.getType().hasName("va_list")).getVariable()
   }
 
-  override predicate isSink(DataFlow::Node sink) { sink.asExpr() instanceof VaArgArg }
+  override predicate isSink(DataFlow::Node sink) { sink.asExpr() instanceof VaAccess }
 }
 
 /**
  * Controlflow nodes preceeding a call to `va_arg`
  */
-ControlFlowNode preceedsFC(VaArgArg va_arg) {
+ControlFlowNode preceedsFC(VaAccess va_arg) {
   result = va_arg
   or
   exists(ControlFlowNode mid |
@@ -49,25 +58,25 @@ ControlFlowNode preceedsFC(VaArgArg va_arg) {
     // stop recursion on va_end on the same object
     not result =
       any(MacroInvocation m |
-        m.getMacroName() = ["va_end"] and
+        m.getMacroName() = ["va_start"] and
         m.getExpr().getChild(0).(VariableAccess).getTarget() = va_arg.(VariableAccess).getTarget()
       ).getExpr()
   )
 }
 
-predicate sameSource(VaArgArg va_arg1, VaArgArg va_arg2) {
+predicate sameSource(VaAccess e1, VaAccess e2) {
   exists(VaArgConfig config, DataFlow::Node source |
-    config.hasFlow(source, DataFlow::exprNode(va_arg1)) and
-    config.hasFlow(source, DataFlow::exprNode(va_arg2))
+    config.hasFlow(source, DataFlow::exprNode(e1)) and
+    config.hasFlow(source, DataFlow::exprNode(e2))
   )
 }
 
-from VaArgArg va_arg1, VaArgArg va_arg2, FunctionCall fc
+from VaAccess va_acc, VaArgArg va_arg, FunctionCall fc
 where
-  not isExcluded(va_arg1,
+  not isExcluded(va_acc,
     Contracts7Package::doNotCallVaArgOnAVaListThatHasAnIndeterminateValueQuery()) and
-  sameSource(va_arg1, va_arg2) and
-  fc = preceedsFC(va_arg1) and
-  fc.getTarget().calls*(va_arg2.getEnclosingFunction())
-select va_arg1, "The value of " + va_arg1.toString() + " is indeterminate after the $@.", fc,
+  sameSource(va_acc, va_arg) and
+  fc = preceedsFC(va_acc) and
+  fc.getTarget().calls*(va_arg.getEnclosingFunction())
+select va_acc, "The value of " + va_acc.toString() + " is indeterminate after the $@.", fc,
   fc.toString()

c/cert/test/rules/MSC39-C/DoNotCallVaArgOnAVaListThatHasAnIndeterminateValue.expected

@@ -1,2 +1,6 @@
 | test.c:23:32:23:33 | ap | The value of ap is indeterminate after the $@. | test.c:17:7:17:19 | call to contains_zero | call to contains_zero |
+| test.c:26:10:26:11 | ap | The value of ap is indeterminate after the $@. | test.c:17:7:17:19 | call to contains_zero | call to contains_zero |
+| test.c:39:12:39:13 | ap | The value of ap is indeterminate after the $@. | test.c:35:7:35:19 | call to contains_zero | call to contains_zero |
+| test.c:48:10:48:11 | ap | The value of ap is indeterminate after the $@. | test.c:35:7:35:19 | call to contains_zero | call to contains_zero |
 | test.c:65:34:65:35 | ap | The value of ap is indeterminate after the $@. | test.c:58:7:58:19 | call to contains_zero | call to contains_zero |
+| test.c:71:10:71:11 | ap | The value of ap is indeterminate after the $@. | test.c:58:7:58:19 | call to contains_zero | call to contains_zero |

c/cert/test/rules/MSC39-C/test.c

@@ -15,15 +15,15 @@ int f1a(size_t count, ...) {
   va_start(ap, count);
 
   if (contains_zero(count, ap)) {
-    va_end(ap);
+    va_start(ap, count);
     return 1;
   }
 
   for (size_t i = 0; i < count; ++i) {
     printf("%f ", 1.0 / va_arg(ap, double)); // NON_COMPLIANT
   }
 
-  va_end(ap);
+  va_end(ap); // NON_COMPLIANT
   return 0;
 }
 
@@ -36,7 +36,7 @@ int f1b(size_t count, ...) {
     printf("0 in arguments!\n");
     status = 1;
   } else {
-    va_end(ap);
+    va_end(ap); // NON_COMPLIANT
     va_start(ap, count);
     for (size_t i = 0; i < count; i++) {
       printf("%f ", 1.0 / va_arg(ap, double)); // COMPLIANT
@@ -45,7 +45,7 @@ int f1b(size_t count, ...) {
     status = 0;
   }
 
-  va_end(ap);
+  va_end(ap); // NON_COMPLIANT
   return status;
 }
 
@@ -59,7 +59,7 @@ int f1c(size_t count, ...) {
     printf("0 in arguments!\n");
     status = 1;
   } else {
-    va_end(ap1); // ending the wrong va_list object
+    va_end(ap1); // COMPLIANT
     va_start(ap1, count);
     for (size_t i = 0; i < count; i++) {
       printf("%f ", 1.0 / va_arg(ap, double)); // NON_COMPLIANT
@@ -68,7 +68,7 @@ int f1c(size_t count, ...) {
     status = 0;
   }
 
-  va_end(ap);
+  va_end(ap); // NON_COMPLIANT
   return status;
 }
 
@@ -80,7 +80,7 @@ int contains_zero_ok(size_t count, va_list *ap) {
       return 1;
     }
   }
-  va_end(ap1);
+  va_end(ap1); // COMPLIANT
   return 0;
 }
 
@@ -100,6 +100,6 @@ int print_reciprocals_ok(size_t count, ...) {
     status = 0;
   }
 
-  va_end(ap);
+  va_end(ap); // COMPLIANT
   return status;
 }