Merge pull request github#6378 from geoffw0/impropnull

C++: Test and improve cpp/improper-null-termination

Author: MathiasVP

cpp/ql/src/Likely Bugs/Memory Management/ImproperNullTermination.ql

@@ -29,11 +29,19 @@ class ImproperNullTerminationReachability extends StackVariableReachabilityWithR
   override predicate isSourceActual(ControlFlowNode node, StackVariable v) {
     node = declWithNoInit(v)
     or
-    exists(Call c, VariableAccess va |
+    exists(Call c, int bufferArg, int sizeArg |
       c = node and
-      c.getTarget().hasName("readlink") and
-      c.getArgument(1) = va and
-      va.getTarget() = v
+      (
+        c.getTarget().hasName("readlink") and bufferArg = 1 and sizeArg = 2
+        or
+        c.getTarget().hasName("readlinkat") and bufferArg = 2 and sizeArg = 3
+      ) and
+      c.getArgument(bufferArg).(VariableAccess).getTarget() = v and
+      (
+        // buffer size parameter likely matches the full buffer size
+        c.getArgument(sizeArg) instanceof SizeofOperator or
+        c.getArgument(sizeArg).getValue().toInt() = v.getType().getSize()
+      )
     )
   }
 

cpp/ql/test/query-tests/Likely Bugs/Memory Management/ImproperNullTermination/ImproperNullTermination.expected

@@ -0,0 +1,14 @@
+| test.cpp:25:10:25:16 | buffer1 | Variable $@ may not be null terminated. | test.cpp:22:8:22:14 | buffer1 | buffer1 |
+| test.cpp:26:10:26:16 | buffer2 | Variable $@ may not be null terminated. | test.cpp:23:8:23:14 | buffer2 | buffer2 |
+| test.cpp:39:10:39:16 | buffer2 | Variable $@ may not be null terminated. | test.cpp:35:8:35:14 | buffer2 | buffer2 |
+| test.cpp:59:10:59:13 | ptr1 | Variable $@ may not be null terminated. | test.cpp:56:9:56:12 | ptr1 | ptr1 |
+| test.cpp:69:10:69:16 | buffer1 | Variable $@ may not be null terminated. | test.cpp:64:8:64:14 | buffer1 | buffer1 |
+| test.cpp:70:10:70:12 | ptr | Variable $@ may not be null terminated. | test.cpp:64:8:64:14 | buffer1 | buffer1 |
+| test.cpp:81:10:81:16 | buffer2 | Variable $@ may not be null terminated. | test.cpp:65:8:65:14 | buffer2 | buffer2 |
+| test.cpp:82:10:82:12 | ptr | Variable $@ may not be null terminated. | test.cpp:65:8:65:14 | buffer2 | buffer2 |
+| test.cpp:93:10:93:15 | buffer | Variable $@ may not be null terminated. | test.cpp:86:8:86:13 | buffer | buffer |
+| test.cpp:116:10:116:15 | buffer | Variable $@ may not be null terminated. | test.cpp:109:8:109:13 | buffer | buffer |
+| test.cpp:130:14:130:19 | buffer | Variable $@ may not be null terminated. | test.cpp:127:7:127:12 | buffer | buffer |
+| test.cpp:139:10:139:15 | buffer | Variable $@ may not be null terminated. | test.cpp:136:8:136:13 | buffer | buffer |
+| test.cpp:147:14:147:19 | buffer | Variable $@ may not be null terminated. | test.cpp:143:8:143:13 | buffer | buffer |
+| test.cpp:170:10:170:15 | buffer | Variable $@ may not be null terminated. | test.cpp:166:8:166:13 | buffer | buffer |

cpp/ql/test/query-tests/Likely Bugs/Memory Management/ImproperNullTermination/ImproperNullTermination.qlref

@@ -0,0 +1 @@
+Likely Bugs/Memory Management/ImproperNullTermination.ql

cpp/ql/test/query-tests/Likely Bugs/Memory Management/ImproperNullTermination/test.cpp

@@ -0,0 +1,211 @@
+
+typedef unsigned int size_t;
+typedef signed int ssize_t;
+
+size_t strlen(const char *s);
+char *strcpy(char *s1, const char *s2);
+char *strdup(const char *s1);
+
+void *malloc(size_t size);
+
+void *memset(void *s, int c, size_t n);
+void *memcpy(void *s1, const void *s2, size_t n);
+
+ssize_t readlink(const char *path, char *buffer, size_t buffer_size);
+ssize_t readlinkat(int fd, const char *path, char *buffer, size_t buffer_size);
+
+bool cond();
+
+void test_unassigned()
+{
+	{
+		char buffer1[1024];
+		char buffer2[1024];
+
+		strdup(buffer1); // BAD
+		strdup(buffer2); // BAD
+
+		memcpy(buffer2, buffer1, sizeof(buffer2));
+		strdup(buffer1); // BAD [NOT DETECTED]
+		strdup(buffer2); // BAD [NOT DETECTED]
+	}
+
+	{
+		char buffer1[1024];
+		char buffer2[1024];
+
+		strcpy(buffer1, "content");
+		strdup(buffer1); // GOOD
+		strdup(buffer2); // BAD
+
+		memcpy(buffer2, buffer1, sizeof(buffer2));
+		strdup(buffer1); // GOOD
+		strdup(buffer2); // GOOD
+	}
+
+	{
+		char buffer1[1024] = {0};
+		char buffer2[1024];
+
+		memset(buffer2, 0, sizeof(buffer2));
+		strdup(buffer1); // GOOD
+		strdup(buffer2); // GOOD
+	}
+
+	{
+		char *ptr1;
+		char *ptr2 = "content";
+
+		strdup(ptr1); // BAD
+		strdup(ptr2); // GOOD
+	}
+
+	{
+		char buffer1[1024];
+		char buffer2[1024];
+		char *ptr;
+
+		ptr = buffer1;
+		strdup(buffer1); // BAD
+		strdup(ptr); // BAD
+
+		strcpy(buffer1, "content");
+		strdup(buffer1); // GOOD
+		strdup(ptr); // GOOD
+
+		ptr = buffer1;
+		strdup(buffer1); // GOOD
+		strdup(ptr); // GOOD
+
+		ptr = buffer2;
+		strdup(buffer2); // BAD
+		strdup(ptr); // BAD
+	}
+
+	{
+		char buffer[1024];
+
+		if (cond())
+		{
+			strcpy(buffer, "content");
+			strdup(buffer); // GOOD
+		}
+		strdup(buffer); // BAD
+	}
+
+	{
+		char buffer[1024];
+
+		if (cond())
+		{
+			strcpy(buffer, "content");
+		} else {
+			strcpy(buffer, "alternative");
+		}
+		strdup(buffer); // GOOD
+	}
+
+	{
+		char buffer[1024];
+
+		while (cond())
+		{
+			strcpy(buffer, "content");
+			strdup(buffer); // GOOD
+		}
+		strdup(buffer); // BAD
+	}
+}
+
+void test_callee(char *p1, char *p2)
+{
+	strdup(p1);
+}
+
+void test_caller()
+{
+	char buffer[1024];
+
+	test_callee("content", buffer); // GOOD
+	test_callee(buffer, "content"); // BAD
+}
+
+void test_readlink(int fd, const char *path, size_t sz)
+{
+	{
+		char buffer[1024];
+
+		readlink(path, buffer, sizeof(buffer));
+		strdup(buffer); // BAD
+	}
+
+	{
+		char buffer[1024];
+		int v;
+
+		readlinkat(fd, path, buffer, sizeof(buffer));
+		v = strlen(buffer); // BAD
+	}
+
+	{
+		char buffer[1024] = {0};
+
+		readlink(path, buffer, sizeof(buffer) - 1);
+		strdup(buffer); // GOOD
+	}
+
+	{
+		char buffer[1024];
+
+		memset(buffer, 0, sizeof(buffer));
+		readlink(path, buffer, sizeof(buffer) - 1);
+		strdup(buffer); // GOOD
+	}
+
+	{
+		char buffer[1024];
+
+		memset(buffer, 0, sizeof(buffer));
+		readlink(path, buffer, sizeof(buffer));
+		strdup(buffer); // BAD
+	}
+
+	{
+		char buffer[1024];
+
+		memset(buffer, 0, sizeof(buffer));
+		readlink(path, buffer, sizeof(buffer));
+		buffer[sizeof(buffer) - 1] = 0;
+		strdup(buffer); // GOOD
+	}
+
+	{
+		char *buffer = (char *)malloc(1024);
+
+		readlink(path, buffer, 1024);
+		strdup(buffer); // BAD [NOT DETECTED]
+	}
+
+	{
+		char *buffer = (char *)malloc(1024);
+
+		buffer[1023] = 0;
+		readlink(path, buffer, 1023);
+		strdup(buffer); // GOOD
+	}
+
+	{
+		char *buffer = (char *)malloc(sz);
+
+		readlink(path, buffer, sz);
+		strdup(buffer); // BAD [NOT DETECTED]
+	}
+
+	{
+		char *buffer = (char *)malloc(sz);
+
+		memset(buffer, 0, sz);
+		readlink(path, buffer, sz - 1);
+		strdup(buffer); // GOOD
+	}
+}