Inconsistent return value handling rule (#10)

* initial work for InconsistentReturnValueHandling

* InconsistentReturnValueHandling v1 working

* fix string stubs

* InconsistentReturnValueHandling v2 working

* InconsistentReturnValueHandling - reduced FPs

* InconsistentReturnValueHandling - small changes

* fix tests

* finalize

* metadata done

* fix string stubs

Author: GrosQuildu

README.md

@@ -50,6 +50,7 @@ codeql database analyze database.db --format=sarif-latest --output=./tob.sarif -
 |[Async unsafe signal handler](./cpp/src/docs/security/AsyncUnsafeSignalHandler/AsyncUnsafeSignalHandler.md)|Async unsafe signal handler (like the one used in CVE-2024-6387)|warning|high|
 |[Decrementation overflow when comparing](./cpp/src/docs/security/DecOverflowWhenComparing/DecOverflowWhenComparing.md)|This query finds unsigned integer overflows resulting from unchecked decrementation during comparison.|error|high|
 |[Find all problematic implicit casts](./cpp/src/docs/security/UnsafeImplicitConversions/UnsafeImplicitConversions.md)|Find all implicit casts that may be problematic. That is, casts that may result in unexpected truncation, reinterpretation or widening of values.|error|high|
+|[Inconsistent handling of return values from a specific function](./cpp/src/docs/security/InconsistentReturnValueHandling/InconsistentReturnValueHandling.md)|Detects functions whose return values are compared inconsistently across call sites, which may indicate bugs|warning|medium|
 |[Invalid string size passed to string manipulation function](./cpp/src/docs/security/CStrnFinder/CStrnFinder.md)|Finds calls to functions that take as input a string and its size as separate arguments (e.g., `strncmp`, `strncat`, ...) and the size argument is wrong|error|low|
 |[Missing null terminator](./cpp/src/docs/security/NoNullTerminator/NoNullTerminator.md)|This query finds incorrectly initialized strings that are passed to functions expecting null-byte-terminated strings|error|high|
 

cpp/src/docs/security/InconsistentReturnValueHandling/InconsistentReturnValueHandling.md

@@ -0,0 +1,69 @@
+# Inconsistent handling of return values from a specific function
+When a function's return value is checked in `if` statements across multiple call sites, the comparisons typically fall into a consistent pattern (e.g., compared against a numeric literal, `NULL`, or `sizeof`). If a small number of call sites compare the return value in a different way than the majority, these inconsistent comparisons may indicate a bug.
+
+The query categorizes each comparison into one of the following categories:
+
+* Numeric literal (e.g., `ret != -1`)
+* Boolean (e.g., `ret == true`)
+* Null pointer (e.g., `ret != NULL`)
+* Pointer
+* `sizeof` expression (e.g., `ret > sizeof(buf)`)
+* Another function's return value (e.g., `ret != other_func()`)
+* Passed as argument to another function (e.g., `if (check(ret))`)
+* Arithmetic expression
+
+When at least 75% of a function's return value comparisons fall into one category, the remaining comparisons in a different category are flagged as potentially incorrect.
+
+
+## Recommendation
+Review each flagged call site and verify that the comparison matches the function's return value semantics. If the function returns an error code or count, all call sites should compare it consistently. Fix any comparisons that use the wrong type of operand (e.g., comparing an integer return value against `sizeof` when all other sites compare against a numeric literal).
+
+
+## Example
+
+```c
+struct header {
+    int type;
+    int length;
+};
+
+// Returns number of items processed, or -1 on error
+int process_items(int *items, int count) {
+    int processed = 0;
+    for (int i = 0; i < count; i++) {
+        if (items[i] < 0)
+            return -1;
+        processed++;
+    }
+    return processed;
+}
+
+void example() {
+    int items[10];
+    int result;
+
+    // Typical: return value compared with a numeric literal
+    result = process_items(items, 10);
+    if (result > 0) { /* success */ }
+
+    result = process_items(items, 5);
+    if (result != -1) { /* no error */ }
+
+    result = process_items(items, 3);
+    if (result == 0) { /* empty */ }
+
+    result = process_items(items, 8);
+    if (result >= 1) { /* at least one */ }
+
+    // BAD: comparing with sizeof instead of a numeric literal.
+    // This is inconsistent with all other call sites and likely a bug.
+    result = process_items(items, 7);
+    if (result > sizeof(struct header)) { /* wrong comparison */ }
+}
+```
+In this example, `process_items` returns the number of items processed or `-1` on error. Most call sites correctly compare the return value with a numeric literal. However, one call site mistakenly compares it with `sizeof(struct header)`, which is inconsistent with how the return value is used everywhere else.
+
+
+## References
+* [CWE-252: Unchecked Return Value](https://cwe.mitre.org/data/definitions/252.html)
+* [CWE-253: Incorrect Check of Function Return Value](https://cwe.mitre.org/data/definitions/253.html)

cpp/src/security/DecOverflowWhenComparing/DecOverflowWhenComparing.ql

@@ -139,8 +139,7 @@ where
       )
     then cmp.getAFalseSuccessor().getASuccessor*() = varAccAfterOverflow
     else any()
-  ) and
-  // skip vendor code
-  not dec.getFile().getAbsolutePath().toLowerCase().matches(["%vendor%", "%third_party%"])
+  )
+
 select dec, "Unsigned decrementation in comparison ($@) - $@", cmp, cmp.toString(),
   varAccAfterOverflow, varAccAfterOverflow.toString()

cpp/src/security/InconsistentReturnValueHandling/InconsistentReturnValueHandling.c

@@ -0,0 +1,38 @@
+struct header {
+    int type;
+    int length;
+};
+
+// Returns number of items processed, or -1 on error
+int process_items(int *items, int count) {
+    int processed = 0;
+    for (int i = 0; i < count; i++) {
+        if (items[i] < 0)
+            return -1;
+        processed++;
+    }
+    return processed;
+}
+
+void example() {
+    int items[10];
+    int result;
+
+    // Typical: return value compared with a numeric literal
+    result = process_items(items, 10);
+    if (result > 0) { /* success */ }
+
+    result = process_items(items, 5);
+    if (result != -1) { /* no error */ }
+
+    result = process_items(items, 3);
+    if (result == 0) { /* empty */ }
+
+    result = process_items(items, 8);
+    if (result >= 1) { /* at least one */ }
+
+    // BAD: comparing with sizeof instead of a numeric literal.
+    // This is inconsistent with all other call sites and likely a bug.
+    result = process_items(items, 7);
+    if (result > sizeof(struct header)) { /* wrong comparison */ }
+}

cpp/src/security/InconsistentReturnValueHandling/InconsistentReturnValueHandling.qhelp

@@ -0,0 +1,62 @@
+<!DOCTYPE qhelp PUBLIC
+  "-//Semmle//qhelp//EN"
+  "qhelp.dtd">
+<qhelp>
+<overview>
+  <p>
+  When a function's return value is checked in <code>if</code> statements across multiple call sites,
+  the comparisons typically fall into a consistent pattern (e.g., compared against a numeric literal,
+  <code>NULL</code>, or <code>sizeof</code>). If a small number of call sites compare the return value
+  in a different way than the majority, these inconsistent comparisons may indicate a bug.
+  </p>
+
+  <p>
+  The query categorizes each comparison into one of the following categories:
+  </p>
+  <ul>
+    <li>Numeric literal (e.g., <code>ret != -1</code>)</li>
+    <li>Boolean (e.g., <code>ret == true</code>)</li>
+    <li>Null pointer (e.g., <code>ret != NULL</code>)</li>
+    <li>Pointer</li>
+    <li><code>sizeof</code> expression (e.g., <code>ret > sizeof(buf)</code>)</li>
+    <li>Another function's return value (e.g., <code>ret != other_func()</code>)</li>
+    <li>Passed as argument to another function (e.g., <code>if (check(ret))</code>)</li>
+    <li>Arithmetic expression</li>
+  </ul>
+
+  <p>
+  When at least 75% of a function's return value comparisons fall into one category,
+  the remaining comparisons in a different category are flagged as potentially incorrect.
+  </p>
+</overview>
+
+<recommendation>
+<p>
+Review each flagged call site and verify that the comparison matches the function's return value semantics.
+If the function returns an error code or count, all call sites should compare it consistently.
+Fix any comparisons that use the wrong type of operand (e.g., comparing an integer return value against
+<code>sizeof</code> when all other sites compare against a numeric literal).
+</p>
+</recommendation>
+
+<example>
+<p>
+In this example, <code>process_items</code> returns the number of items processed or <code>-1</code>
+on error. Most call sites correctly compare the return value with a numeric literal. However, one
+call site mistakenly compares it with <code>sizeof(struct header)</code>, which is inconsistent
+with how the return value is used everywhere else.
+</p>
+
+<sample src="InconsistentReturnValueHandling.c" />
+</example>
+
+<references>
+<li>
+  <a href="https://cwe.mitre.org/data/definitions/252.html">CWE-252: Unchecked Return Value</a>
+</li>
+<li>
+  <a href="https://cwe.mitre.org/data/definitions/253.html">CWE-253: Incorrect Check of Function Return Value</a>
+</li>
+</references>
+
+</qhelp>