Merge pull request #3786 from geoffw0/bufferwritecleanup

C++: Clean up BufferWrite.qll

cpp/ql/src/Likely Bugs/Likely Typos/UsingStrcpyAsBoolean.ql

@@ -15,6 +15,15 @@ import cpp
 import semmle.code.cpp.models.implementations.Strcpy
 import semmle.code.cpp.dataflow.DataFlow
 
+/**
+ * A string copy function that returns a string, rather than an error code (for
+ * example, `strcpy` returns a string, whereas `strcpy_s` returns an error
+ * code).
+ */
+class InterestingStrcpyFunction extends StrcpyFunction {
+  InterestingStrcpyFunction() { getType().getUnspecifiedType() instanceof PointerType }
+}
+
 predicate isBoolean(Expr e1) {
   exists(Type t1 |
     t1 = e1.getType() and
@@ -25,12 +34,12 @@ predicate isBoolean(Expr e1) {
 predicate isStringCopyCastedAsBoolean(FunctionCall func, Expr expr1, string msg) {
   DataFlow::localExprFlow(func, expr1) and
   isBoolean(expr1.getConversion*()) and
-  func.getTarget() instanceof StrcpyFunction and
+  func.getTarget() instanceof InterestingStrcpyFunction and
   msg = "Return value of " + func.getTarget().getName() + " used as a Boolean."
 }
 
 predicate isStringCopyUsedInLogicalOperationOrCondition(FunctionCall func, Expr expr1, string msg) {
-  func.getTarget() instanceof StrcpyFunction and
+  func.getTarget() instanceof InterestingStrcpyFunction and
   (
     (
       // it is being used in an equality or logical operation

cpp/ql/src/semmle/code/cpp/models/implementations/Gets.qll

@@ -1,3 +1,8 @@
+/**
+ * Provides implementation classes modelling `gets` and various similar
+ * functions. See `semmle.code.cpp.models.Models` for usage information.
+ */
+
 import semmle.code.cpp.models.interfaces.DataFlow
 import semmle.code.cpp.models.interfaces.Taint
 import semmle.code.cpp.models.interfaces.ArrayFunction

cpp/ql/src/semmle/code/cpp/models/implementations/Memcpy.qll

@@ -1,3 +1,8 @@
+/**
+ * Provides implementation classes modelling `memcpy` and various similar
+ * functions. See `semmle.code.cpp.models.Models` for usage information.
+ */
+
 import semmle.code.cpp.Function
 import semmle.code.cpp.models.interfaces.ArrayFunction
 import semmle.code.cpp.models.interfaces.DataFlow

cpp/ql/src/semmle/code/cpp/models/implementations/Memset.qll

@@ -1,3 +1,8 @@
+/**
+ * Provides implementation classes modelling `memset` and various similar
+ * functions. See `semmle.code.cpp.models.Models` for usage information.
+ */
+
 import semmle.code.cpp.Function
 import semmle.code.cpp.models.interfaces.ArrayFunction
 import semmle.code.cpp.models.interfaces.DataFlow

cpp/ql/src/semmle/code/cpp/models/implementations/Printf.qll

@@ -66,12 +66,25 @@ class Sprintf extends FormattingFunction {
   Sprintf() {
     this instanceof TopLevelFunction and
     (
-      hasGlobalOrStdName("sprintf") or
-      hasGlobalName("_sprintf_l") or
-      hasGlobalName("__swprintf_l") or
-      hasGlobalOrStdName("wsprintf") or
-      hasGlobalName("g_strdup_printf") or
-      hasGlobalName("g_sprintf") or
+      // sprintf(dst, format, args...)
+      hasGlobalOrStdName("sprintf")
+      or
+      //  _sprintf_l(dst, format, locale, args...)
+      hasGlobalName("_sprintf_l")
+      or
+      // __swprintf_l(dst, format, locale, args...)
+      hasGlobalName("__swprintf_l")
+      or
+      // wsprintf(dst, format, args...)
+      hasGlobalOrStdName("wsprintf")
+      or
+      // g_strdup_printf(format, ...)
+      hasGlobalName("g_strdup_printf")
+      or
+      // g_sprintf(dst, format, ...)
+      hasGlobalName("g_sprintf")
+      or
+      // __builtin___sprintf_chk(dst, flag, os, format, ...)
       hasGlobalName("__builtin___sprintf_chk")
     ) and
     not exists(getDefinition().getFile().getRelativePath())

cpp/ql/src/semmle/code/cpp/models/implementations/Strcat.qll

@@ -24,6 +24,21 @@ class StrcatFunction extends TaintFunction, DataFlowFunction, ArrayFunction, Sid
     )
   }
 
+  /**
+   * Gets the index of the parameter that is the size of the copy (in characters).
+   */
+  int getParamSize() { exists(getParameter(2)) and result = 2 }
+
+  /**
+   * Gets the index of the parameter that is the source of the copy.
+   */
+  int getParamSrc() { result = 1 }
+
+  /**
+   * Gets the index of the parameter that is the destination to be appended to.
+   */
+  int getParamDest() { result = 0 }
+
   override predicate hasDataFlow(FunctionInput input, FunctionOutput output) {
     input.isParameter(0) and
     output.isReturnValue()

cpp/ql/src/semmle/code/cpp/models/implementations/Strcpy.qll

@@ -1,3 +1,8 @@
+/**
+ * Provides implementation classes modelling `strcpy` and various similar
+ * functions. See `semmle.code.cpp.models.Models` for usage information.
+ */
+
 import semmle.code.cpp.models.interfaces.ArrayFunction
 import semmle.code.cpp.models.interfaces.DataFlow
 import semmle.code.cpp.models.interfaces.Taint
@@ -8,70 +13,110 @@ import semmle.code.cpp.models.interfaces.SideEffect
  */
 class StrcpyFunction extends ArrayFunction, DataFlowFunction, TaintFunction, SideEffectFunction {
   StrcpyFunction() {
-    this.hasName("strcpy") or
-    this.hasName("_mbscpy") or
-    this.hasName("wcscpy") or
-    this.hasName("strncpy") or
-    this.hasName("_strncpy_l") or
-    this.hasName("_mbsncpy") or
-    this.hasName("_mbsncpy_l") or
-    this.hasName("wcsncpy") or
-    this.hasName("_wcsncpy_l")
+    exists(string name | name = getName() |
+      // strcpy(dst, src)
+      name = "strcpy"
+      or
+      // wcscpy(dst, src)
+      name = "wcscpy"
+      or
+      // _mbscpy(dst, src)
+      name = "_mbscpy"
+      or
+      (
+        name = "strcpy_s" or // strcpy_s(dst, max_amount, src)
+        name = "wcscpy_s" or // wcscpy_s(dst, max_amount, src)
+        name = "_mbscpy_s" // _mbscpy_s(dst, max_amount, src)
+      ) and
+      // exclude the 2-parameter template versions
+      // that find the size of a fixed size destination buffer.
+      getNumberOfParameters() = 3
+      or
+      // strncpy(dst, src, max_amount)
+      name = "strncpy"
+      or
+      // _strncpy_l(dst, src, max_amount, locale)
+      name = "_strncpy_l"
+      or
+      // wcsncpy(dst, src, max_amount)
+      name = "wcsncpy"
+      or
+      // _wcsncpy_l(dst, src, max_amount, locale)
+      name = "_wcsncpy_l"
+      or
+      // _mbsncpy(dst, src, max_amount)
+      name = "_mbsncpy"
+      or
+      // _mbsncpy_l(dst, src, max_amount, locale)
+      name = "_mbsncpy_l"
+    )
   }
 
-  override predicate hasArrayInput(int bufParam) { bufParam = 1 }
+  /**
+   * Holds if this is one of the `strcpy_s` variants.
+   */
+  private predicate isSVariant() {
+    exists(string name | name = getName() | name.suffix(name.length() - 2) = "_s")
+  }
 
-  override predicate hasArrayOutput(int bufParam) { bufParam = 0 }
+  /**
+   * Gets the index of the parameter that is the maximum size of the copy (in characters).
+   */
+  int getParamSize() {
+    if isSVariant()
+    then result = 1
+    else
+      if exists(getName().indexOf("ncpy"))
+      then result = 2
+      else none()
+  }
 
-  override predicate hasArrayWithNullTerminator(int bufParam) { bufParam = 1 }
+  /**
+   * Gets the index of the parameter that is the source of the copy.
+   */
+  int getParamSrc() { if isSVariant() then result = 2 else result = 1 }
+
+  /**
+   * Gets the index of the parameter that is the destination of the copy.
+   */
+  int getParamDest() { result = 0 }
+
+  override predicate hasArrayInput(int bufParam) { bufParam = getParamSrc() }
+
+  override predicate hasArrayOutput(int bufParam) { bufParam = getParamDest() }
+
+  override predicate hasArrayWithNullTerminator(int bufParam) { bufParam = getParamSrc() }
 
   override predicate hasArrayWithVariableSize(int bufParam, int countParam) {
-    (
-      this.hasName("strncpy") or
-      this.hasName("_strncpy_l") or
-      this.hasName("_mbsncpy") or
-      this.hasName("_mbsncpy_l") or
-      this.hasName("wcsncpy") or
-      this.hasName("_wcsncpy_l")
-    ) and
-    bufParam = 0 and
-    countParam = 2
+    bufParam = getParamDest() and
+    countParam = getParamSize()
   }
 
   override predicate hasArrayWithUnknownSize(int bufParam) {
-    (
-      this.hasName("strcpy") or
-      this.hasName("_mbscpy") or
-      this.hasName("wcscpy")
-    ) and
-    bufParam = 0
+    not exists(getParamSize()) and
+    bufParam = getParamDest()
   }
 
   override predicate hasDataFlow(FunctionInput input, FunctionOutput output) {
-    input.isParameterDeref(1) and
-    output.isParameterDeref(0)
+    not exists(getParamSize()) and
+    input.isParameterDeref(getParamSrc()) and
+    output.isParameterDeref(getParamDest())
     or
-    input.isParameterDeref(1) and
+    not exists(getParamSize()) and
+    input.isParameterDeref(getParamSrc()) and
     output.isReturnValueDeref()
     or
-    input.isParameter(0) and
+    input.isParameter(getParamDest()) and
     output.isReturnValue()
   }
 
   override predicate hasTaintFlow(FunctionInput input, FunctionOutput output) {
+    // these may do only a partial copy of the input buffer to the output
+    // buffer
+    exists(getParamSize()) and
+    input.isParameter(getParamSrc()) and
     (
-      // these may do only a partial copy of the input buffer to the output
-      // buffer
-      this.hasName("strncpy") or
-      this.hasName("_strncpy_l") or
-      this.hasName("_mbsncpy") or
-      this.hasName("_mbsncpy_l") or
-      this.hasName("wcsncpy") or
-      this.hasName("_wcsncpy_l")
-    ) and
-    input.isParameter(2) and
-    (
-      output.isParameterDeref(0) or
+      output.isParameterDeref(getParamDest()) or
       output.isReturnValueDeref()
     )
   }
@@ -81,17 +126,18 @@ class StrcpyFunction extends ArrayFunction, DataFlowFunction, TaintFunction, Sid
   override predicate hasOnlySpecificWriteSideEffects() { any() }
 
   override predicate hasSpecificWriteSideEffect(ParameterIndex i, boolean buffer, boolean mustWrite) {
-    i = 0 and
+    i = getParamDest() and
     buffer = true and
     mustWrite = false
   }
 
   override predicate hasSpecificReadSideEffect(ParameterIndex i, boolean buffer) {
-    i = 1 and
+    i = getParamSrc() and
     buffer = true
   }
 
   override ParameterIndex getParameterSizeIndex(ParameterIndex i) {
-    hasArrayWithVariableSize(i, result)
+    i = getParamDest() and
+    result = getParamSize()
   }
 }

cpp/ql/src/semmle/code/cpp/models/implementations/Strdup.qll

@@ -1,3 +1,8 @@
+/**
+ * Provides implementation classes modelling `strdup` and various similar
+ * functions. See `semmle.code.cpp.models.Models` for usage information.
+ */
+
 import semmle.code.cpp.models.interfaces.Allocation
 import semmle.code.cpp.models.interfaces.ArrayFunction
 import semmle.code.cpp.models.interfaces.DataFlow

cpp/ql/src/semmle/code/cpp/security/BufferWrite.qll

@@ -10,6 +10,7 @@ import semmle.code.cpp.commons.Alloc
 import semmle.code.cpp.commons.Buffer
 import semmle.code.cpp.commons.Scanf
 import semmle.code.cpp.models.implementations.Strcat
+import semmle.code.cpp.models.implementations.Strcpy
 
 /*
  * --- BufferWrite framework ---
@@ -106,74 +107,19 @@ abstract class BufferWriteCall extends BufferWrite, FunctionCall { }
  * A call to a variant of `strcpy`.
  */
 class StrCopyBW extends BufferWriteCall {
-  StrCopyBW() {
-    exists(TopLevelFunction fn, string name | fn = getTarget() and name = fn.getName() |
-      // strcpy(dst, src)
-      name = "strcpy"
-      or
-      // wcscpy(dst, src)
-      name = "wcscpy"
-      or
-      // _mbscpy(dst, src)
-      name = "_mbscpy"
-      or
-      (
-        name = "strcpy_s" or // strcpy_s(dst, max_amount, src)
-        name = "wcscpy_s" or // wcscpy_s(dst, max_amount, src)
-        name = "_mbscpy_s" // _mbscpy_s(dst, max_amount, src)
-      ) and
-      // exclude the 2-parameter template versions
-      // that find the size of a fixed size destination buffer.
-      fn.getNumberOfParameters() = 3
-      or
-      // strncpy(dst, src, max_amount)
-      name = "strncpy"
-      or
-      // strncpy_l(dst, src, max_amount, locale)
-      name = "strncpy_l"
-      or
-      // wcsncpy(dst, src, max_amount)
-      name = "wcsncpy"
-      or
-      // _wcsncpy_l(dst, src, max_amount, locale)
-      name = "_wcsncpy_l"
-      or
-      // _mbsncpy(dst, src, max_amount)
-      name = "_mbsncpy"
-      or
-      // _mbsncpy_l(dst, src, max_amount, locale)
-      name = "_mbsncpy_l"
-    )
-  }
+  StrcpyFunction f;
+
+  StrCopyBW() { getTarget() = f.(TopLevelFunction) }
 
   /**
    * Gets the index of the parameter that is the maximum size of the copy (in characters).
    */
-  int getParamSize() {
-    exists(TopLevelFunction fn, string name |
-      fn = getTarget() and
-      name = fn.getName() and
-      (
-        if name.suffix(name.length() - 2) = "_s"
-        then result = 1
-        else
-          if exists(name.indexOf("ncpy"))
-          then result = 2
-          else none()
-      )
-    )
-  }
+  int getParamSize() { result = f.getParamSize() }
 
   /**
    * Gets the index of the parameter that is the source of the copy.
    */
-  int getParamSrc() {
-    exists(TopLevelFunction fn, string name |
-      fn = getTarget() and
-      name = fn.getName() and
-      (if name.suffix(name.length() - 2) = "_s" then result = 2 else result = 1)
-    )
-  }
+  int getParamSrc() { result = f.getParamSrc() }
 
   override Type getBufferType() {
     result = this.getTarget().getParameter(getParamSrc()).getUnspecifiedType()
@@ -181,7 +127,7 @@ class StrCopyBW extends BufferWriteCall {
 
   override Expr getASource() { result = getArgument(getParamSrc()) }
 
-  override Expr getDest() { result = getArgument(0) }
+  override Expr getDest() { result = getArgument(f.getParamDest()) }
 
   override predicate hasExplicitLimit() { exists(getParamSize()) }
 
@@ -198,17 +144,19 @@ class StrCopyBW extends BufferWriteCall {
  * A call to a variant of `strcat`.
  */
 class StrCatBW extends BufferWriteCall {
-  StrCatBW() { exists(TopLevelFunction fn | fn = getTarget() and fn instanceof StrcatFunction) }
+  StrcatFunction f;
+
+  StrCatBW() { getTarget() = f.(TopLevelFunction) }
 
   /**
    * Gets the index of the parameter that is the maximum size of the copy (in characters).
    */
-  int getParamSize() { if exists(getArgument(2)) then result = 2 else none() }
+  int getParamSize() { result = f.getParamSize() }
 
   /**
    * Gets the index of the parameter that is the source of the copy.
    */
-  int getParamSrc() { result = 1 }
+  int getParamSrc() { result = f.getParamSrc() }
 
   override Type getBufferType() {
     result = this.getTarget().getParameter(getParamSrc()).getUnspecifiedType()
@@ -216,7 +164,7 @@ class StrCatBW extends BufferWriteCall {
 
   override Expr getASource() { result = getArgument(getParamSrc()) }
 
-  override Expr getDest() { result = getArgument(0) }
+  override Expr getDest() { result = getArgument(f.getParamDest()) }
 
   override predicate hasExplicitLimit() { exists(getParamSize()) }
 
@@ -233,8 +181,10 @@ class StrCatBW extends BufferWriteCall {
  * A call to a variant of `sprintf`.
  */
 class SprintfBW extends BufferWriteCall {
+  FormattingFunction f;
+
   SprintfBW() {
-    exists(TopLevelFunction fn, string name | fn = getTarget() and name = fn.getName() |
+    exists(string name | f = getTarget().(TopLevelFunction) and name = f.getName() |
       /*
        * C sprintf variants:
        */
@@ -270,10 +220,7 @@ class SprintfBW extends BufferWriteCall {
   }
 
   override Type getBufferType() {
-    exists(FormattingFunction f |
-      f = this.getTarget() and
-      result = f.getParameter(f.getFormatParameterIndex()).getUnspecifiedType()
-    )
+    result = f.getParameter(f.getFormatParameterIndex()).getUnspecifiedType()
   }
 
   override Expr getASource() {
@@ -282,7 +229,7 @@ class SprintfBW extends BufferWriteCall {
     result = this.(FormattingFunctionCall).getFormatArgument(_)
   }
 
-  override Expr getDest() { result = getArgument(0) }
+  override Expr getDest() { result = getArgument(f.getOutputParameterIndex()) }
 
   override int getMaxData() {
     exists(FormatLiteral fl |