Merge pull request #2222 from geoffw0/libraryperf

CPP: Improvements for ConditionallyInitializedVariable.ql

cpp/ql/src/Security/CWE/CWE-457/ConditionallyUninitializedVariableBad.c

@@ -19,7 +19,7 @@ int notify(int deviceNumber) {
   DeviceConfig config;
   initDeviceConfig(&config, deviceNumber);
   // BAD: Using config without checking the status code that is returned
-  if (config->isEnabled) {
-    notifyChannel(config->channel);
+  if (config.isEnabled) {
+    notifyChannel(config.channel);
   }
-}
+}

cpp/ql/src/Security/CWE/CWE-457/ConditionallyUninitializedVariableGood.c

@@ -20,8 +20,8 @@ void notify(int deviceNumber) {
   int statusCode = initDeviceConfig(&config, deviceNumber);
   if (statusCode == 0) {
     // GOOD: Status code returned by initialization function is checked, so this is safe
-    if (config->isEnabled) {
-      notifyChannel(config->channel);
+    if (config.isEnabled) {
+      notifyChannel(config.channel);
     }
   }
-}
+}

cpp/ql/src/Security/CWE/CWE-457/InitializationFunctions.qll

@@ -620,32 +620,47 @@ Function getAPossibleDefinition(Function undefinedFunction) {
 }
 
 /**
- * Gets a possible target for the Call, using the name and parameter matching if we did not associate
+ * Helper predicate for `getTarget`, that computes possible targets of a `Call`.
+ *
+ * If there is at least one defined target after performing some simple virtual dispatch
+ * resolution, then the result is all the defined targets.
+ */
+private Function getTarget1(Call c) {
+  result = VirtualDispatch::getAViableTarget(c) and
+  result.isDefined()
+}
+
+/**
+ * Helper predicate for `getTarget`, that computes possible targets of a `Call`.
+ *
+ * If we can use the heuristic matching of functions to find definitions for some of the viable
+ * targets, return those.
+ */
+private Function getTarget2(Call c) {
+  not exists(getTarget1(c)) and
+  result = getAPossibleDefinition(VirtualDispatch::getAViableTarget(c))
+}
+
+/**
+ * Helper predicate for `getTarget`, that computes possible targets of a `Call`.
+ *
+ * Otherwise, the result is the undefined `Function` instances.
+ */
+private Function getTarget3(Call c) {
+  not exists(getTarget1(c)) and
+  not exists(getTarget2(c)) and
+  result = VirtualDispatch::getAViableTarget(c)
+}
+
+/**
+ * Gets a possible target for the `Call`, using the name and parameter matching if we did not associate
  * this call with a specific definition at link or compile time, and performing simple virtual
  * dispatch resolution.
  */
 Function getTarget(Call c) {
-  if VirtualDispatch::getAViableTarget(c).isDefined()
-  then
-    /*
-     * If there is at least one defined target after performing some simple virtual dispatch
-     * resolution, then the result is all the defined targets.
-     */
-
-    result = VirtualDispatch::getAViableTarget(c) and
-    result.isDefined()
-  else
-    if exists(getAPossibleDefinition(VirtualDispatch::getAViableTarget(c)))
-    then
-      /*
-       * If we can use the heuristic matching of functions to find definitions for some of the viable
-       * targets, return those.
-       */
-
-      result = getAPossibleDefinition(VirtualDispatch::getAViableTarget(c))
-    else
-      // Otherwise, the result is the undefined `Function` instances
-      result = VirtualDispatch::getAViableTarget(c)
+  result = getTarget1(c) or
+  result = getTarget2(c) or
+  result = getTarget3(c)
 }
 
 /**

cpp/ql/src/semmle/code/cpp/dispatch/VirtualDispatch.qll

@@ -28,6 +28,19 @@ module VirtualDispatch {
     not result.hasName("IUnknown")
   }
 
+  /**
+   * Helper predicate for `getAViableTarget`, which computes the viable targets for
+   * virtual calls based on the qualifier type.
+   */
+  private Function getAViableVirtualCallTarget(Class qualifierType, MemberFunction staticTarget) {
+    exists(Class qualifierSubType |
+      result = getAPossibleImplementation(staticTarget) and
+      qualifierType = qualifierSubType.getABaseClass*() and
+      mayInherit(qualifierSubType, result) and
+      not cannotInherit(qualifierSubType, result)
+    )
+  }
+
   /**
    * Gets a viable target for the given function call.
    *
@@ -42,18 +55,9 @@ module VirtualDispatch {
    * If `c` is not a virtual call, the result will be `c.getTarget()`.
    */
   Function getAViableTarget(Call c) {
-    exists(Function staticTarget | staticTarget = c.getTarget() |
-      if c.(FunctionCall).isVirtual() and staticTarget instanceof MemberFunction
-      then
-        exists(Class qualifierType, Class qualifierSubType |
-          result = getAPossibleImplementation(staticTarget) and
-          qualifierType = getCallQualifierType(c) and
-          qualifierType = qualifierSubType.getABaseClass*() and
-          mayInherit(qualifierSubType, result) and
-          not cannotInherit(qualifierSubType, result)
-        )
-      else result = staticTarget
-    )
+    if c.(FunctionCall).isVirtual() and c.getTarget() instanceof MemberFunction
+    then result = getAViableVirtualCallTarget(getCallQualifierType(c), c.getTarget())
+    else result = c.getTarget()
   }
 
   /** Holds if `f` is declared in `c` or a transitive base class of `c`. */

cpp/ql/test/query-tests/Security/CWE/CWE-457/semmle/ConditionallyUninitializedVariable/ConditionallyUninitializedVariable.expected

@@ -0,0 +1,3 @@
+| examples.cpp:38:3:38:18 | call to initDeviceConfig | The status of this call to $@ is not checked, potentially leaving $@ uninitialized. | examples.cpp:13:5:13:20 | initDeviceConfig | initDeviceConfig | examples.cpp:37:16:37:21 | config | config |
+| test.cpp:22:2:22:17 | call to maybeInitialize1 | The status of this call to $@ is not checked, potentially leaving $@ uninitialized. | test.cpp:4:5:4:20 | maybeInitialize1 | maybeInitialize1 | test.cpp:19:6:19:6 | a | a |
+| test.cpp:68:2:68:17 | call to maybeInitialize2 | The status of this call to $@ is not checked, potentially leaving $@ uninitialized. | test.cpp:51:6:51:21 | maybeInitialize2 | maybeInitialize2 | test.cpp:66:6:66:6 | a | a |

cpp/ql/test/query-tests/Security/CWE/CWE-457/semmle/ConditionallyUninitializedVariable/ConditionallyUninitializedVariable.qlref

@@ -0,0 +1 @@
+Security/CWE/CWE-457/ConditionallyUninitializedVariable.ql

cpp/ql/test/query-tests/Security/CWE/CWE-457/semmle/ConditionallyUninitializedVariable/examples.cpp

@@ -0,0 +1,43 @@
+// based on the qhelp
+
+int getMaxDevices();
+bool fetchIsDeviceEnabled(int deviceNumber);
+int fetchDeviceChannel(int deviceNumber);
+void notifyChannel(int channel);
+
+struct DeviceConfig {
+  bool isEnabled;
+  int channel;
+};
+
+int initDeviceConfig(DeviceConfig *ref, int deviceNumber) {
+  if (deviceNumber >= getMaxDevices()) {
+    // No device with that number, return -1 to indicate failure
+    return -1;
+  }
+  // Device with that number, fetch parameters and initialize struct
+  ref->isEnabled = fetchIsDeviceEnabled(deviceNumber);
+  ref->channel = fetchDeviceChannel(deviceNumber);
+  // Return 0 to indicate success
+  return 0;
+}
+
+void notifyGood(int deviceNumber) {
+  DeviceConfig config;
+  int statusCode = initDeviceConfig(&config, deviceNumber);
+  if (statusCode == 0) {
+    // GOOD: Status code returned by initialization function is checked, so this is safe
+    if (config.isEnabled) {
+      notifyChannel(config.channel);
+    }
+  }
+}
+
+int notifyBad(int deviceNumber) {
+  DeviceConfig config;
+  initDeviceConfig(&config, deviceNumber);
+  // BAD: Using config without checking the status code that is returned
+  if (config.isEnabled) {
+    notifyChannel(config.channel);
+  }
+}

cpp/ql/test/query-tests/Security/CWE/CWE-457/semmle/ConditionallyUninitializedVariable/test.cpp

@@ -0,0 +1,96 @@
+
+void use(int i);
+
+int maybeInitialize1(int *v)
+{
+	static int resources = 100;
+
+	if (resources == 0)
+	{
+		return 0; // FAIL
+	}
+
+	*v = resources--;
+	return 1; // SUCCESS
+}
+
+void test1()
+{
+	int a, b, c, d, e, f;
+	int result1, result2;
+
+	maybeInitialize1(&a); // BAD (initialization not checked)
+	use(a);
+	
+	if (maybeInitialize1(&b) == 1) // GOOD
+	{
+		use(b);
+	}
+	
+	if (maybeInitialize1(&c) == 0) // BAD (initialization check is wrong) [NOT DETECTED]
+	{
+		use(c);
+	}
+
+	result1 = maybeInitialize1(&d); // BAD (initialization stored but not checked) [NOT DETECTED]
+	use(d);
+
+	result2 = maybeInitialize1(&e); // GOOD
+	if (result2 == 1)
+	{
+		use(e);
+	}
+
+	if (maybeInitialize1(&f) == 0) // GOOD
+	{
+		return;
+	}
+	use(f);
+}
+
+bool maybeInitialize2(int *v)
+{
+	static int resources = 100;
+
+	if (resources > 0)
+	{
+		*v = resources--;
+		return true; // SUCCESS
+	}
+
+	return false; // FAIL
+}
+
+void test2()
+{
+	int a, b;
+
+	maybeInitialize2(&a); // BAD (initialization not checked)
+	use(a);
+	
+	if (maybeInitialize2(&b)) // GOOD
+	{
+		use(b);
+	}
+}
+
+int alwaysInitialize(int *v)
+{
+	static int resources = 0;
+
+	*v = resources++;
+	return 1; // SUCCESS
+}
+
+void test3()
+{
+	int a, b;
+
+	alwaysInitialize(&a); // GOOD (initialization never fails)
+	use(a);
+	
+	if (alwaysInitialize(&b) == 1) // GOOD
+	{
+		use(b);
+	}
+}