[CPP-370] First attempt at isAdditionalFlowStep().

cpp/ql/src/Likely Bugs/Format/NonConstantFormat.ql

@@ -45,8 +45,7 @@ predicate whitelistFunction(Function f, int arg) {
 predicate underscoreMacro(Expr e) {
   exists(MacroInvocation mi |
     mi.getMacroName() = "_" and
-    mi.getExpr() = e and
-    isConstMacro(e)
+    mi.getExpr() = e
   )
 }
 
@@ -56,19 +55,14 @@ predicate whitelisted(Expr e) {
     isConst(fc.getArgument(arg))
   )
   or
+  // we let the '_' macro through regardless of what it points at
   underscoreMacro(e)
 }
 
-predicate isConstMacro(Expr e) {
-  e instanceof StringLiteral
-  or
-  whitelisted(e)
-}
-
 predicate isConst(Expr e) {
-  isConstMacro(e)
+  e instanceof StringLiteral
   or
-  underscoreMacro(e)
+  whitelisted(e)
 }
 
 class ConstFlow extends DataFlow::Configuration {
@@ -79,6 +73,20 @@ class ConstFlow extends DataFlow::Configuration {
   override predicate isSink(DataFlow::Node sink) {
     exists(FormattingFunctionCall fc | sink.asExpr() = fc.getArgument(fc.getFormatParameterIndex()))
   }
+
+  override predicate isAdditionalFlowStep(DataFlow::Node source, DataFlow::Node sink) {
+  	none()
+  	or
+    // an element picked from an array of string literals is a string literal
+    exists(Variable v, int a |
+      a = sink.asExpr().(ArrayExpr).getArrayOffset().getValue().toInt() and
+      v = sink.asExpr().(ArrayExpr).getArrayBase().(VariableAccess).getTarget()
+    |
+    // we disallow parameters, since they may be bound to unsafe arguments
+    // at various call sites.
+      not v instanceof Parameter and source.asExpr() instanceof StringLiteral
+      )
+  }
 }
 
 from FormattingFunctionCall call, Expr formatString

cpp/ql/test/query-tests/Likely Bugs/Format/NonConstantFormat/NonConstantFormat.expected

@@ -3,7 +3,6 @@
 | NonConstantFormat.c:50:2:50:7 | call to printf | The format string argument to printf should be constant to prevent security issues and other potential errors. |
 | test.cpp:48:3:48:8 | call to printf | The format string argument to printf should be constant to prevent security issues and other potential errors. |
 | test.cpp:54:5:54:10 | call to printf | The format string argument to printf should be constant to prevent security issues and other potential errors. |
-| test.cpp:55:5:55:10 | call to printf | The format string argument to printf should be constant to prevent security issues and other potential errors. |
 | test.cpp:56:5:56:10 | call to printf | The format string argument to printf should be constant to prevent security issues and other potential errors. |
 | test.cpp:57:5:57:10 | call to printf | The format string argument to printf should be constant to prevent security issues and other potential errors. |
 | test.cpp:58:5:58:10 | call to printf | The format string argument to printf should be constant to prevent security issues and other potential errors. |
@@ -17,7 +16,4 @@
 | test.cpp:79:5:79:10 | call to printf | The format string argument to printf should be constant to prevent security issues and other potential errors. |
 | test.cpp:85:5:85:10 | call to printf | The format string argument to printf should be constant to prevent security issues and other potential errors. |
 | test.cpp:90:5:90:10 | call to printf | The format string argument to printf should be constant to prevent security issues and other potential errors. |
-| test.cpp:97:5:97:10 | call to printf | The format string argument to printf should be constant to prevent security issues and other potential errors. |
-| test.cpp:104:5:104:10 | call to printf | The format string argument to printf should be constant to prevent security issues and other potential errors. |
 | test.cpp:107:5:107:10 | call to printf | The format string argument to printf should be constant to prevent security issues and other potential errors. |
-| test.cpp:121:3:121:8 | call to printf | The format string argument to printf should be constant to prevent security issues and other potential errors. |

cpp/ql/test/query-tests/Likely Bugs/Format/NonConstantFormat/test.cpp

@@ -52,7 +52,7 @@ int main(int argc, char **argv) {
     char hello[] = "hello, World\n";
     hello[0] = 'H';
     printf(hello); // NOT OK
-    printf(_(hello)); // NOT OK
+    printf(_(hello)); // OK
     printf(gettext(hello)); // NOT OK
     printf(const_wash(hello)); // NOT OK
     printf((hello + 1) + 1); // NOT OK
@@ -94,14 +94,14 @@ int main(int argc, char **argv) {
     const char *hello = "Hello, World\n";
     const char **p = &hello;
     (*p)++;
-    printf(hello); // NOT OK
+    printf(hello); // NOT OK [NOT DETECTED]
   }
   {
     // Same as above block but through a C++ reference
     const char *hello = "Hello, World\n";
     const char *&p = hello;
     p++;
-    printf(hello); // NOT OK
+    printf(hello); // NOT OK [NOT DETECTED]
   }
   if (gettext_debug) {
     printf(new char[100]); // NOT OK
@@ -109,14 +109,10 @@ int main(int argc, char **argv) {
   {
     const char *hello = "Hello, World\n";
     const char *const *p = &hello; // harmless reference to const pointer
-    printf(hello); // OK [FALSE POSITIVE]
+    printf(hello); // OK
     hello++; // modification comes after use and so does no harm
   }
   printf(argc > 2 ? "More than one\n" : _("Only one\n")); // OK
 
-  // This false positive arises because we use const_wash in a problematic
-  // place at one call site, and then the error spreads to all call sites. It
-  // does not happen for "_" only because functions with the name "_" are
-  // special-cased and assumed correct in the query.
-  printf(const_wash("Hello, World\n")); // OK [FALSE POSITIVE]
+  printf(const_wash("Hello, World\n")); // OK
 }