Merge pull request #2647 from geoffw0/modelpure

CPP: Improve strlen model

Author: jbj

change-notes/1.24/analysis-cpp.md

@@ -35,6 +35,7 @@ The following changes in version 1.24 affect C/C++ analysis in all applications.
     about the _name or scope_ of variables should remain unchanged.
   * The `LocalScopeVariableReachability` library is deprecated in favor of
     `StackVariableReachability`. The functionality is the same.
+* The models library models `strlen` in more detail, and includes common variations such as `wcslen`.
 * The taint tracking library (`semmle.code.cpp.dataflow.TaintTracking`) has had
   the following improvements:
   * The library now models data flow through `strdup` and similar functions.

cpp/ql/src/semmle/code/cpp/commons/NullTermination.qll

@@ -80,14 +80,6 @@ predicate functionArgumentMustBeNullTerminated(Function f, int i) {
   f.(ArrayFunction).hasArrayInput(i)
   or
   f instanceof StrcatFunction and i = 0
-  or
-  f.hasName("strlen") and i = 0
-  or
-  f.hasName("strcmp") and i in [0 .. 1]
-  or
-  f.hasName("strchr") and i = 0
-  or
-  f.hasName("strstr") and i in [0 .. 1]
 }
 
 /**

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

@@ -30,7 +30,16 @@ class PureStrFunction extends AliasFunction, ArrayFunction, TaintFunction, SideE
         name = "strtol" or
         name = "strtoll" or
         name = "strtoq" or
-        name = "strtoul"
+        name = "strtoul" or
+        name = "wcslen"
+      )
+      or
+      hasGlobalName(name) and
+      (
+        name = "_mbslen" or
+        name = "_mbslen_l" or
+        name = "_mbstrlen" or
+        name = "_mbstrlen_l"
       )
     )
   }
@@ -39,6 +48,10 @@ class PureStrFunction extends AliasFunction, ArrayFunction, TaintFunction, SideE
     getParameter(bufParam).getUnspecifiedType() instanceof PointerType
   }
 
+  override predicate hasArrayWithNullTerminator(int bufParam) {
+    getParameter(bufParam).getUnspecifiedType() instanceof PointerType
+  }
+
   override predicate hasTaintFlow(FunctionInput input, FunctionOutput output) {
     exists(ParameterIndex i |
       input.isParameter(i) and

cpp/ql/test/library-tests/nulltermination/mayAddNullTerminator.expected

@@ -1,23 +1,23 @@
-| test.c:15:3:15:18 | ... = ... | test.c:15:3:15:8 | buffer |
-| test.c:22:11:22:34 | ... = ... | test.c:22:24:22:27 | args |
-| test.c:23:5:23:15 | ... = ... | test.c:23:6:23:8 | ptr |
-| test.c:36:3:36:12 | call to addNullAsm | test.c:36:14:36:19 | buffer |
-| test.c:41:3:41:13 | call to expression | test.c:41:7:41:12 | buffer |
-| test.c:48:2:48:10 | ... = ... | test.c:48:3:48:6 | data |
-| test.c:49:2:49:15 | ... = ... | test.c:49:3:49:6 | data |
-| test.c:52:2:52:12 | ... = ... | test.c:52:2:52:5 | data |
-| test.c:55:16:55:19 | data | test.c:55:16:55:19 | data |
-| test.c:56:2:56:13 | ... = ... | test.c:56:2:56:6 | data2 |
-| test.c:59:2:59:14 | ... = ... | test.c:59:11:59:14 | data |
-| test.c:60:2:60:18 | ... = ... | test.c:60:11:60:14 | data |
-| test.c:63:2:63:7 | call to strcpy | test.c:63:9:63:12 | data |
-| test.c:64:2:64:8 | call to addNull | test.c:64:10:64:13 | data |
-| test.c:65:2:65:15 | call to addNullVarargs | test.c:65:20:65:23 | data |
-| test.c:66:2:66:11 | call to addNullAsm | test.c:66:13:66:16 | data |
-| test.c:67:2:67:15 | call to addNullWrapper | test.c:67:17:67:20 | data |
-| test.c:68:2:68:23 | call to addNullFunctionPointer | test.c:68:25:68:28 | data |
-| test.c:86:2:86:13 | ... = ... | test.c:86:2:86:6 | data2 |
-| test.c:100:2:100:7 | call to strcpy | test.c:100:9:100:14 | buffer |
-| test.c:108:2:108:21 | ... = ... | test.c:108:2:108:5 | data |
-| test.c:114:2:114:18 | call to strlenWrapperSafe | test.c:114:20:114:23 | data |
-| test.c:115:2:115:7 | call to strcpy | test.c:115:9:115:12 | data |
+| test.c:18:3:18:18 | ... = ... | test.c:18:3:18:8 | buffer |
+| test.c:25:11:25:34 | ... = ... | test.c:25:24:25:27 | args |
+| test.c:26:5:26:15 | ... = ... | test.c:26:6:26:8 | ptr |
+| test.c:39:3:39:12 | call to addNullAsm | test.c:39:14:39:19 | buffer |
+| test.c:44:3:44:13 | call to expression | test.c:44:7:44:12 | buffer |
+| test.c:51:2:51:10 | ... = ... | test.c:51:3:51:6 | data |
+| test.c:52:2:52:15 | ... = ... | test.c:52:3:52:6 | data |
+| test.c:55:2:55:12 | ... = ... | test.c:55:2:55:5 | data |
+| test.c:58:16:58:19 | data | test.c:58:16:58:19 | data |
+| test.c:59:2:59:13 | ... = ... | test.c:59:2:59:6 | data2 |
+| test.c:62:2:62:14 | ... = ... | test.c:62:11:62:14 | data |
+| test.c:63:2:63:18 | ... = ... | test.c:63:11:63:14 | data |
+| test.c:66:2:66:7 | call to strcpy | test.c:66:9:66:12 | data |
+| test.c:67:2:67:8 | call to addNull | test.c:67:10:67:13 | data |
+| test.c:68:2:68:15 | call to addNullVarargs | test.c:68:20:68:23 | data |
+| test.c:69:2:69:11 | call to addNullAsm | test.c:69:13:69:16 | data |
+| test.c:70:2:70:15 | call to addNullWrapper | test.c:70:17:70:20 | data |
+| test.c:71:2:71:23 | call to addNullFunctionPointer | test.c:71:25:71:28 | data |
+| test.c:89:2:89:13 | ... = ... | test.c:89:2:89:6 | data2 |
+| test.c:103:2:103:7 | call to strcpy | test.c:103:9:103:14 | buffer |
+| test.c:111:2:111:21 | ... = ... | test.c:111:2:111:5 | data |
+| test.c:117:2:117:18 | call to strlenWrapperSafe | test.c:117:20:117:23 | data |
+| test.c:118:2:118:7 | call to strcpy | test.c:118:9:118:12 | data |

cpp/ql/test/library-tests/nulltermination/test.c

@@ -8,6 +8,9 @@ char* strcat(char* destination, const char* source);
 char* strcpy(char* destination, const char* source);
 int strlen(const char* str);
 char* strcpy(char* destination, const char* source);
+char* strstr(char* s1, const char* s2);
+const char* strchr(const char* s, int c);
+int strcmp(const char *s1, const char *s2);
 
 char* global = "         ";
 

cpp/ql/test/library-tests/nulltermination/variableMustBeNullTerminated.expected

@@ -1,10 +1,10 @@
-| test.c:94:16:94:19 | data |
-| test.c:99:9:99:12 | data |
-| test.c:100:17:100:20 | data |
-| test.c:101:9:101:14 | buffer |
-| test.c:101:17:101:20 | data |
+| test.c:97:16:97:19 | data |
 | test.c:102:9:102:12 | data |
-| test.c:103:9:103:12 | data |
-| test.c:103:15:103:20 | buffer |
-| test.c:104:16:104:19 | data |
-| test.c:109:16:109:19 | data |
+| test.c:103:17:103:20 | data |
+| test.c:104:9:104:14 | buffer |
+| test.c:104:17:104:20 | data |
+| test.c:105:9:105:12 | data |
+| test.c:106:9:106:12 | data |
+| test.c:106:15:106:20 | buffer |
+| test.c:107:16:107:19 | data |
+| test.c:112:16:112:19 | data |

cpp/ql/test/query-tests/Security/CWE/CWE-131/semmle/NoSpaceForZeroTerminator/NoSpaceForZeroTerminator.expected

@@ -6,4 +6,6 @@
 | test.cpp:24:35:24:40 | call to malloc | This allocation does not include space to null-terminate the string. |
 | test.cpp:63:28:63:33 | call to malloc | This allocation does not include space to null-terminate the string. |
 | test.cpp:71:28:71:33 | call to malloc | This allocation does not include space to null-terminate the string. |
+| test.cpp:79:28:79:33 | call to malloc | This allocation does not include space to null-terminate the string. |
+| test.cpp:89:35:89:40 | call to malloc | This allocation does not include space to null-terminate the string. |
 | test.cpp:106:24:106:48 | new[] | This allocation does not include space to null-terminate the string. |

cpp/ql/test/query-tests/Security/CWE/CWE-131/semmle/NoSpaceForZeroTerminator/test.cpp

@@ -75,7 +75,7 @@ void bad6(char *str, char *dest) {
 }
 
 void bad7(char *str, char *str2) {
-    // BAD -- zero-termination proved by strcmp [NOT DETECTED]
+    // BAD -- zero-termination proved by strcmp
     char *buffer = (char *)malloc(strlen(str));
     decode(buffer, str);
     if (strcmp(buffer, str2) == 0) {
@@ -85,7 +85,7 @@ void bad7(char *str, char *str2) {
 }
 
 void bad8(wchar_t *str) {
-    // BAD -- zero-termination proved by wcslen [NOT DETECTED]
+    // BAD -- zero-termination proved by wcslen
     wchar_t *wbuffer = (wchar_t *)malloc(wcslen(str));
     wdecode(wbuffer, str);
     if (wcslen(wbuffer) == 0) {