From 934fd33d928183d4bf32c2d255820867ee727c4a Mon Sep 17 00:00:00 2001
From: GrosQuildu <e2.8a.95@gmail.com>
Date: Tue, 2 Jul 2024 12:56:18 +0200
Subject: [PATCH 1/6] initial work for InconsistentReturnValueHandling

---
 .../InconsistentReturnValueHandling.qhelp     |  27 ++
 .../InconsistentReturnValueHandling.ql        | 167 +++++++++
 .../InconsistentReturnValueHandling.cpp       | 323 ++++++++++++++++++
 .../InconsistentReturnValueHandling.expected  |   1 +
 .../InconsistentReturnValueHandling.qlref     |   2 +
 5 files changed, 520 insertions(+)
 create mode 100644 cpp/src/security/InconsistentReturnValueHandling/InconsistentReturnValueHandling.qhelp
 create mode 100644 cpp/src/security/InconsistentReturnValueHandling/InconsistentReturnValueHandling.ql
 create mode 100644 cpp/test/query-tests/security/InconsistentReturnValueHandling/InconsistentReturnValueHandling.cpp
 create mode 100644 cpp/test/query-tests/security/InconsistentReturnValueHandling/InconsistentReturnValueHandling.expected
 create mode 100644 cpp/test/query-tests/security/InconsistentReturnValueHandling/InconsistentReturnValueHandling.qlref

diff --git a/cpp/src/security/InconsistentReturnValueHandling/InconsistentReturnValueHandling.qhelp b/cpp/src/security/InconsistentReturnValueHandling/InconsistentReturnValueHandling.qhelp
new file mode 100644
index 0000000..fc60a6c
--- /dev/null
+++ b/cpp/src/security/InconsistentReturnValueHandling/InconsistentReturnValueHandling.qhelp
@@ -0,0 +1,27 @@
+<!DOCTYPE qhelp PUBLIC
+"-//Semmle//qhelp//EN"
+"qhelp.dtd">
+<qhelp>
+<overview>
+<p>
+    Description of the vulnerability.
+</p>
+</overview>
+<recommendation>
+<p>
+    Do XYZ.
+</p>
+</recommendation>
+<example>
+<p>
+    The following example shows ...:
+</p>
+<sample src="SomeFileInCWD.cpp"/>
+</example>
+<references>
+<li>
+    XYZ:
+    <a href="https://trailofbits.com">XYZ</a>
+</li>
+</references>
+</qhelp>
diff --git a/cpp/src/security/InconsistentReturnValueHandling/InconsistentReturnValueHandling.ql b/cpp/src/security/InconsistentReturnValueHandling/InconsistentReturnValueHandling.ql
new file mode 100644
index 0000000..48324d8
--- /dev/null
+++ b/cpp/src/security/InconsistentReturnValueHandling/InconsistentReturnValueHandling.ql
@@ -0,0 +1,167 @@
+/**
+ * @name Inconsistent handling of return values from a specific function
+ * @description If a function returns a value that is used in `if` statements,
+ *  and in some statements the value is compared differently than in some other statements, this implies a possible bugs.
+ * @kind problem
+ * @problem.severity warning
+ * @precision medium
+ * @id tob/cpp/inconsistent-retval-handling
+ * @tags security
+ */
+
+ /**
+ * Possible inconsistencies
+ * - small % of values is compared to a different type (int, bool, nullptr, retval of a function, ...)
+ */
+
+
+import cpp
+import semmle.code.cpp.dataflow.new.DataFlow
+import semmle.code.cpp.controlflow.IRGuards
+
+newtype TCmpClass =
+  Tint()
+  or
+  Tnull()
+  or
+  Tptr()
+  or
+  Tsizeof()
+  or
+  Tcall()
+  or
+  Targ()
+
+class CmpClass extends TCmpClass {
+  string toString() {
+    this = Tint() and result = "int"
+    or
+    this = Tnull() and result = "null"
+    or
+    this = Tptr() and result = "ptr"
+    or
+    this = Tsizeof() and result = "sizeof"
+    or
+    this = Tcall() and result = "call"
+    or
+    this = Targ() and result = "arg"
+  }
+}
+
+Expr getOtherOperand(ComparisonOperation cmp, Expr fcRetVal) {
+    if cmp.getRightOperand() = fcRetVal then
+        result = cmp.getLeftOperand()
+    else
+        result = cmp.getRightOperand()
+}
+
+// TODO: why codeql does not have IntegralLiteral?
+predicate numericLiteral(Expr expr) {
+    expr instanceof Literal
+    and not (
+        expr instanceof BlockExpr
+        or
+        expr instanceof FormatLiteral	
+        or
+        expr instanceof NULL
+        or
+        expr instanceof TextLiteral
+        or
+        expr instanceof LabelLiteral
+        or
+        expr.getType() instanceof NullPointerType
+    )
+}
+
+int countRetValType(Function f, TCmpClass comparedValCategory, FunctionCall fc) {
+    result = count(Expr fcRetVal, IfStmt ifs |
+        fc.getTarget() = f
+        and DataFlow::localFlow(
+            DataFlow::exprNode(fc),
+            DataFlow::exprNode(fcRetVal)
+        )
+        and fcRetVal = ifs.getCondition().getAChild*()
+        and (
+            (
+                // if (retVal != comparedVal)
+                exists(ComparisonOperation cmp, Expr comparedVal |
+                    ifs.getCondition() = cmp
+                    and comparedVal = getOtherOperand(cmp, fcRetVal)
+                    and comparedValCategory = operandCategory(comparedVal)
+                )
+            )
+            or
+            (
+                // if(func(retVal) != anything)
+                exists(FunctionCall anyFc |
+                    ifs.getCondition().getAChild*() = anyFc
+                    and anyFc.getAnArgument() = fcRetVal
+                    and comparedValCategory = Targ()
+                )
+            )
+        )
+        |
+        comparedValCategory
+    )
+}
+
+TCmpClass operandCategory(Expr comparedVal) {
+    (numericLiteral(comparedVal) and result = Tint())
+    or
+    ((comparedVal.getType() instanceof NullPointerType or comparedVal instanceof NULL) and result = Tnull())
+    or
+    (comparedVal.getUnderlyingType() instanceof DerivedType and result = Tptr())
+}
+
+from Function f, int categoryCount,
+    TCmpClass mostCommonCategory, CmpClass mostCommonCategoryClass, int categoryMax
+    // TCmpClass buggyCategory, CmpClass buggyCategoryClass, FunctionCall buggyFc
+where
+    // we are interested only in defined and used functions
+    exists(FunctionCall fc | fc.getTarget() = f)
+    and f.hasDefinition()
+
+    // the function's retVal must be used in some IF statements
+    and categoryCount = countRetValType(f, _, _)
+    and categoryCount > 2
+
+    // now we find how the function's retVal is used most commonly
+    and mostCommonCategory = max(| categoryMax = countRetValType(f, mostCommonCategory, _) | mostCommonCategory order by categoryMax)
+    and mostCommonCategoryClass = mostCommonCategory
+
+    // if threshold for "most common" use case is 80%, then remaining 20% function calls are handled incorrectly
+    // and ((float)(categoryMax * 100) / categoryCount) >= 80
+
+    // // and finally we are looking for calls that use retVal in an uncommon way
+    // and countRetValType(f, buggyCategory, buggyFc) != 0
+    // and buggyCategory != mostCommonCategory
+    // and buggyCategoryClass = buggyCategory 
+    
+// select buggyFc, "Function $@ is usually compared with " + mostCommonCategoryClass + "(" + categoryMax +
+//     " times), but this call compares with " + buggyCategoryClass, f, f.getName()
+
+select f, mostCommonCategoryClass, categoryMax
+
+
+
+// class RetValCheck {
+
+// }
+
+// predicate typicalRetValComparison(Function f) {
+
+// }
+
+// from Function f, FunctionCall fc, ControlFlowNode test, GuardCondition guard
+// where 
+//   fc.getTarget() = f
+//   and guard.controls(fc.getBasicBlock(), _)
+//   and test = guard.getTest()
+//   and fc = rank[1 .. 5](FunctionCall a, string t, int c |
+//     a.getTarget() = f
+//     and any(RelationalOperation ro | ro = test.getElement() |
+//       ro.getGreaterOperand() = a or ro.getLesserOperand() = a
+//     ).getFullyConverted().getType().getName() = t
+//     order by c = count(fc)
+//     ).first()
+// select f, fc, "Inconsistent return value handling found in function calls to \"" + f.getQualifiedName() + "\". The comparison type used in \"" + fc.toString() + "\" is not the most common comparison type."
\ No newline at end of file
diff --git a/cpp/test/query-tests/security/InconsistentReturnValueHandling/InconsistentReturnValueHandling.cpp b/cpp/test/query-tests/security/InconsistentReturnValueHandling/InconsistentReturnValueHandling.cpp
new file mode 100644
index 0000000..5c58a15
--- /dev/null
+++ b/cpp/test/query-tests/security/InconsistentReturnValueHandling/InconsistentReturnValueHandling.cpp
@@ -0,0 +1,323 @@
+#include <stdio.h>
+
+struct something {
+    int x;
+    char w;
+};
+
+struct athing {
+    int x;
+    char w;
+    int y;
+};
+
+bool cmp_func(int a)
+{
+    return a >= 10 ? true : false;
+}
+bool cmp_func2(int a)
+{
+    return a >= 102 ? true : false;
+}
+
+int main() {
+    return 0;
+}
+
+// BAD 1
+int target_func_1(int a)
+{
+    return a + 1;
+}
+
+void test_1_1() {
+    // BAD: comparing with sizeof instead of hard-coded int
+    if (target_func_1(2) != sizeof(something)) {
+        puts("something");
+    }
+}
+
+void test_1_2() {
+    if (target_func_1(2) != 1) {
+        puts("something2");
+    }
+    if (target_func_1(2) > 0) {
+        puts("something2");
+    }
+    int r = target_func_1(3);
+    if (r > 0) {
+        puts("something2");
+    }
+}
+
+
+
+// BAD 2
+int target_func_2(int a)
+{
+    return a - 2;
+}
+
+void test_2_1() {
+    if (target_func_2(2) != sizeof(something)) {
+        puts("something");
+    }
+}
+
+void test_2_2() {
+    if (target_func_2(-123) != sizeof(something)) {
+        puts("something");
+    }
+    int r = target_func_2(123);
+    if (r > sizeof(something)) {
+        return;
+    }
+
+    r = target_func_2(3);
+    // BAD: comparing with hard-coded int instead of sizeof
+    if (r > 0) {
+        puts("something2");
+    }
+}
+
+// BAD 3
+int target_func_3(int a)
+{
+    return a*a;
+}
+
+void test_3_1() {
+    if (target_func_3(2) != sizeof(something)) {
+        puts("something");
+    }
+}
+
+void test_3_2() {
+    if (target_func_3(-123) != sizeof(something)) {
+        puts("something");
+    }
+    int r = target_func_3(123);
+    if (r > sizeof(something)) {
+        return;
+    }
+
+    r = target_func_3(-3);
+    // BAD: comparing with a different sizeof
+    if (r > sizeof(athing)) {
+        puts("something2");
+    }
+}
+
+// BAD 4
+class SomeClass
+{
+private:
+    bool r;
+    int r2;
+public:
+    SomeClass();
+    void DoSomething(int);
+    void DoSomethingElse();
+    int target_func_4(int);
+};
+
+void test_4_1() {
+    SomeClass sc = SomeClass();
+    if (sc.target_func_4(2) != 1) {
+        puts("something");
+    }
+}
+
+void test_4_2() {
+    SomeClass sc = SomeClass();
+    if (sc.target_func_4(-123) != -1) {
+        puts("something");
+    }
+    int r = sc.target_func_4(123);
+    if (r > 0) {
+        return;
+    }
+
+    r = sc.target_func_4(-3);
+    if (r > 123) {
+        puts("something2");
+    }
+}
+
+SomeClass::SomeClass() {
+    r = true;
+    r2 = 33;
+}
+
+void SomeClass::DoSomething(int a) {
+    if (target_func_4(-123) != -1) {
+        puts("something");
+    }
+    int r = target_func_4(123);
+    if (r > 0) {
+        return;
+    }
+
+    r = target_func_4(-3);
+    if (r > 123) {
+        puts("something2");
+    }
+}
+
+void SomeClass::DoSomethingElse() {
+    if (target_func_4(-123) != -1) {
+        puts("something");
+    }
+    int r = target_func_4(123);
+    // BAD: bool instead of int comparison
+    if (r != true) {
+        return;
+    }
+}
+
+int SomeClass::target_func_4(int a) {
+    return a + 2;
+}
+
+// BAD 5
+static int w = 2;
+int* target_func_5(int a)
+{
+    return &w;
+}
+
+void test_5_1() {
+    if (target_func_5(2) != nullptr) {
+        puts("something");
+    }
+}
+
+void test_5_2() {
+    if (target_func_5(-123) != nullptr) {
+        puts("something");
+    }
+    int *r = target_func_5(123);
+    if (r == NULL) {
+        return;
+    }
+
+    r = target_func_5(-3);
+    int *some_ptr = &w;
+    // BAD: comparing with a pointer instead of NULL
+    if (r > some_ptr) {
+        puts("something2");
+    }
+}
+
+// BAD 6
+bool target_func_6(int a)
+{
+    return a > 10 ? true : false;
+}
+
+void test_6_1() {
+    if (target_func_6(2) != cmp_func(2)) {
+        puts("something");
+    }
+}
+
+void test_6_2() {
+    if (target_func_6(-123) != cmp_func2(-123)) {
+        puts("something");
+    }
+    bool r = target_func_6(123);
+    if (r == cmp_func(3333)) {
+        return;
+    }
+
+    r = target_func_6(-3);
+    // BAD: comparing with sizeof instead of some other function
+    if (r != (bool)sizeof(something)) {
+        puts("something2");
+    }
+}
+
+// BAD 7
+bool target_func_7(int a)
+{
+    return a > 10 ? true : false;
+}
+bool some_func_cmp(bool x) {
+    return !x;
+}
+
+void test_6_1() {
+    if (some_func_cmp(target_func_7(2))) {
+        puts("something");
+    }
+}
+
+void test_6_2() {
+    if (some_func_cmp(target_func_7(12)) != cmp_func2(-123)) {
+        puts("something");
+    }
+    bool r = target_func_7(123);
+    if (some_func_cmp(r) < 7) {
+        return;
+    }
+
+    r = target_func_7(-3);
+    // BAD: comparing with something instead of using as argument to a function
+    if (r != cmp_func2(-123)) {
+        puts("something2");
+    }
+}
+
+// GOOD 1: always comparing with NULL
+int* target_func_g1(int a)
+{
+    return &w;
+}
+
+void test_g_1_1() {
+    if (target_func_g1(2) != nullptr) {
+        puts("something");
+    }
+}
+
+void test_g_1_2() {
+    if (target_func_g1(-123) != nullptr) {
+        puts("something");
+    }
+    int *r = target_func_g1(123);
+    if (r == NULL) {
+        return;
+    }
+
+    r = target_func_g1(-3);
+    if (r == nullptr) {
+        puts("something2");
+    }
+}
+
+// GOOD 2: always comparing with some function
+bool target_func_g2(int a)
+{
+    return a > 10 ? true : false;
+}
+
+void test_g_2_1() {
+    if (target_func_g2(2) != cmp_func(2)) {
+        puts("something");
+    }
+}
+
+void test_g_2_2() {
+    if (target_func_g2(-123) != cmp_func(-123)) {
+        puts("something");
+    }
+    bool r = target_func_g2(123);
+    if (r == cmp_func(3333)) {
+        return;
+    }
+
+    r = target_func_g2(-3);
+    if (r > cmp_func2(-3)) {
+        puts("something2");
+    }
+}
\ No newline at end of file
diff --git a/cpp/test/query-tests/security/InconsistentReturnValueHandling/InconsistentReturnValueHandling.expected b/cpp/test/query-tests/security/InconsistentReturnValueHandling/InconsistentReturnValueHandling.expected
new file mode 100644
index 0000000..0ae1442
--- /dev/null
+++ b/cpp/test/query-tests/security/InconsistentReturnValueHandling/InconsistentReturnValueHandling.expected
@@ -0,0 +1 @@
+| new query passed |
diff --git a/cpp/test/query-tests/security/InconsistentReturnValueHandling/InconsistentReturnValueHandling.qlref b/cpp/test/query-tests/security/InconsistentReturnValueHandling/InconsistentReturnValueHandling.qlref
new file mode 100644
index 0000000..112388d
--- /dev/null
+++ b/cpp/test/query-tests/security/InconsistentReturnValueHandling/InconsistentReturnValueHandling.qlref
@@ -0,0 +1,2 @@
+InconsistentReturnValueHandling/InconsistentReturnValueHandling.ql
+

From de7298c98be14c1fbdc9a6dfecc481829e2e47ff Mon Sep 17 00:00:00 2001
From: GrosQuildu <e2.8a.95@gmail.com>
Date: Mon, 8 Jul 2024 15:25:09 +0200
Subject: [PATCH 2/6] InconsistentReturnValueHandling v1 working

---
 .../InconsistentReturnValueHandling.ql        | 260 +++++++++++-------
 .../InconsistentReturnValueHandling.cpp       |  66 ++++-
 .../InconsistentReturnValueHandling.expected  |   9 +-
 .../InconsistentReturnValueHandling.qlref     |   2 +-
 4 files changed, 222 insertions(+), 115 deletions(-)

diff --git a/cpp/src/security/InconsistentReturnValueHandling/InconsistentReturnValueHandling.ql b/cpp/src/security/InconsistentReturnValueHandling/InconsistentReturnValueHandling.ql
index 48324d8..55aea8c 100644
--- a/cpp/src/security/InconsistentReturnValueHandling/InconsistentReturnValueHandling.ql
+++ b/cpp/src/security/InconsistentReturnValueHandling/InconsistentReturnValueHandling.ql
@@ -1,7 +1,10 @@
 /**
  * @name Inconsistent handling of return values from a specific function
- * @description If a function returns a value that is used in `if` statements,
- *  and in some statements the value is compared differently than in some other statements, this implies a possible bugs.
+ * @description If a function's return value is used in `if` statements,
+ *              and in a few statements the value is compared somehow differently than it is usually,
+ *              then this rare comparisons may indicate bugs.
+ *              The query categorizes uses of return values into a few categories
+ *              (cmp with int, bool, nullptr, sizeof, another function, ...)
  * @kind problem
  * @problem.severity warning
  * @precision medium
@@ -9,12 +12,6 @@
  * @tags security
  */
 
- /**
- * Possible inconsistencies
- * - small % of values is compared to a different type (int, bool, nullptr, retval of a function, ...)
- */
-
-
 import cpp
 import semmle.code.cpp.dataflow.new.DataFlow
 import semmle.code.cpp.controlflow.IRGuards
@@ -22,146 +19,201 @@ import semmle.code.cpp.controlflow.IRGuards
 newtype TCmpClass =
   Tint()
   or
+  Tbool()
+  or
   Tnull()
   or
   Tptr()
   or
-  Tsizeof()
+  Tsizeof(Type s)
   or
   Tcall()
   or
   Targ()
+  or
+  Tarithm()
 
 class CmpClass extends TCmpClass {
   string toString() {
-    this = Tint() and result = "int"
+    this = Tint() and result = " numeric literal"
     or
-    this = Tnull() and result = "null"
+    this = Tbool() and result = " bool"
     or
-    this = Tptr() and result = "ptr"
+    this = Tnull() and result = " null"
     or
-    this = Tsizeof() and result = "sizeof"
+    this = Tptr() and result = " pointer"
     or
-    this = Tcall() and result = "call"
+    exists(Type s | this = Tsizeof(s) and result = " sizeof(" + s + ")")
     or
-    this = Targ() and result = "arg"
+    this = Tcall() and result = " some function's return value"
+    or
+    this = Tarithm() and result = " arithmetic expression"
+    or
+    this = Targ() and result = "in call to some function"
   }
 }
 
-Expr getOtherOperand(ComparisonOperation cmp, Expr fcRetVal) {
-    if cmp.getRightOperand() = fcRetVal then
-        result = cmp.getLeftOperand()
-    else
-        result = cmp.getRightOperand()
-}
-
 // TODO: why codeql does not have IntegralLiteral?
 predicate numericLiteral(Expr expr) {
-    expr instanceof Literal
-    and not (
-        expr instanceof BlockExpr
-        or
-        expr instanceof FormatLiteral	
-        or
-        expr instanceof NULL
-        or
-        expr instanceof TextLiteral
-        or
-        expr instanceof LabelLiteral
+    (
+        expr instanceof Literal
+        and not (
+            expr instanceof BlockExpr
+            or
+            expr instanceof FormatLiteral	
+            or
+            expr instanceof NULL
+            or
+            expr instanceof TextLiteral
+            or
+            expr instanceof LabelLiteral
+            or
+            expr.getType() instanceof NullPointerType
+        )
+    )
+    or
+    (
+        expr instanceof UnaryArithmeticOperation
+        and
+        numericLiteral(expr.getAChild())
+    )
+}
+
+predicate binaryComputation(Expr e) {
+    e instanceof BinaryArithmeticOperation
+    or
+    e instanceof BinaryBitwiseOperation
+    or
+    e instanceof BinaryLogicalOperation
+}
+
+/**
+ * Categorize expressions using literals instead of types, because
+ * we want to differentiate between functions calls and hard-coded stuff 
+ */
+TCmpClass operandCategory(Expr comparedVal) {
+    (numericLiteral(comparedVal) and not comparedVal.getType() instanceof BoolType and result = Tint())
+    or
+    (numericLiteral(comparedVal) and comparedVal.getType() instanceof BoolType and result = Tbool())
+    or
+    ((comparedVal.getType() instanceof NullPointerType or comparedVal instanceof NULL) and result = Tnull())
+    or
+    (comparedVal.getUnderlyingType() instanceof DerivedType and result = Tptr())
+    or
+    (comparedVal instanceof FunctionCall and result = Tcall())
+    or
+    (comparedVal instanceof SizeofOperator and
+        (
+        result = Tsizeof(comparedVal.(SizeofExprOperator).getExprOperand().getType())
         or
-        expr.getType() instanceof NullPointerType
+        result = Tsizeof(comparedVal.(SizeofTypeOperator).getTypeOperand())
+        )
     )
+    or
+    (binaryComputation(comparedVal) and result = Tarithm())
+}
+
+module RetValFlowConfig implements DataFlow::ConfigSig {
+    predicate isSource(DataFlow::Node source) {
+        source = source
+    }
+
+    predicate isSink(DataFlow::Node sink) {
+        sink = sink
+    }
 }
+module RetValFlow = DataFlow::Global<RetValFlowConfig>;
 
-int countRetValType(Function f, TCmpClass comparedValCategory, FunctionCall fc) {
-    result = count(Expr fcRetVal, IfStmt ifs |
+Expr getOtherOperand(ComparisonOperation cmp, Expr fcRetVal) {
+    if cmp.getRightOperand().getAChild*() = fcRetVal then
+        result = cmp.getLeftOperand()
+    else
+        result = cmp.getRightOperand()
+}
+
+predicate categorize(Function f, FunctionCall fc, TCmpClass comparedValCategory, IfStmt ifs) {
+    exists(Expr fcRetVal |
         fc.getTarget() = f
-        and DataFlow::localFlow(
+        and RetValFlow::flow(
             DataFlow::exprNode(fc),
             DataFlow::exprNode(fcRetVal)
         )
-        and fcRetVal = ifs.getCondition().getAChild*()
-        and (
-            (
-                // if (retVal != comparedVal)
-                exists(ComparisonOperation cmp, Expr comparedVal |
-                    ifs.getCondition() = cmp
-                    and comparedVal = getOtherOperand(cmp, fcRetVal)
-                    and comparedValCategory = operandCategory(comparedVal)
-                )
+        and ifs.getCondition().getAChild*() = fcRetVal
+        and
+        if
+            // if(func(retVal) == anything)
+            exists(FunctionCall anyFc |
+                ifs.getCondition().getAChild*() = anyFc
+                and anyFc.getAnArgument() = fcRetVal
             )
-            or
-            (
-                // if(func(retVal) != anything)
-                exists(FunctionCall anyFc |
-                    ifs.getCondition().getAChild*() = anyFc
-                    and anyFc.getAnArgument() = fcRetVal
-                    and comparedValCategory = Targ()
-                )
+        then
+            comparedValCategory = Targ()
+        else
+            // if (retVal != comparedVal)
+            exists(ComparisonOperation cmp, Expr comparedVal |
+                ifs.getCondition().getAChild*() = cmp
+                and
+                    // if (2*retVal+1 != comparedVal)
+                    // if (retVal > 2*anything()+sizeof(struct))
+                    if (
+                        cmp.getAnOperand().getAChild*() = fcRetVal
+                        and binaryComputation(cmp.getAnOperand())
+                    )
+                    then
+                        comparedValCategory = Tarithm()
+                    else
+                        // if (retVal != comparedVal)
+                        comparedVal = getOtherOperand(cmp, fcRetVal)
+                        and comparedValCategory = operandCategory(comparedVal)
             )
-        )
-        |
-        comparedValCategory
     )
 }
 
-TCmpClass operandCategory(Expr comparedVal) {
-    (numericLiteral(comparedVal) and result = Tint())
-    or
-    ((comparedVal.getType() instanceof NullPointerType or comparedVal instanceof NULL) and result = Tnull())
-    or
-    (comparedVal.getUnderlyingType() instanceof DerivedType and result = Tptr())
+int countAllRetValTypes(Function f) {
+    result = count(IfStmt ifs |
+        categorize(f, _, _, ifs) |
+        ifs
+    )
 }
 
-from Function f, int categoryCount,
-    TCmpClass mostCommonCategory, CmpClass mostCommonCategoryClass, int categoryMax
-    // TCmpClass buggyCategory, CmpClass buggyCategoryClass, FunctionCall buggyFc
+int mostCommonRetValType(Function f, TCmpClass mostCommonCategory) {
+    result = max(int numberOfRetValTypeInstances |
+        categorize(f, _, mostCommonCategory, _)
+        and numberOfRetValTypeInstances = count(IfStmt ifs | categorize(f, _, mostCommonCategory, ifs) | ifs)
+    )
+}
+
+// uncomment for testing:
+// from Function f, FunctionCall fc, TCmpClass comparedValCategory, CmpClass x, IfStmt ifs
+// where
+//     categorize(f, fc, comparedValCategory, ifs)
+//     and x = comparedValCategory
+// select f, fc, x, ifs
+
+
+from Function f, int retValsTotalAmount,
+    TCmpClass mostCommonCategory, CmpClass mostCommonCategoryClass, int categoryMax,
+    TCmpClass buggyCategory, CmpClass buggyCategoryClass, FunctionCall buggyFc
 where
-    // we are interested only in defined and used functions
+    // we are interested only in used functions
     exists(FunctionCall fc | fc.getTarget() = f)
-    and f.hasDefinition()
+    // and f.hasDefinition()
 
     // the function's retVal must be used in some IF statements
-    and categoryCount = countRetValType(f, _, _)
-    and categoryCount > 2
+    and retValsTotalAmount = countAllRetValTypes(f)
+    and retValsTotalAmount >= 4
 
     // now we find how the function's retVal is used most commonly
-    and mostCommonCategory = max(| categoryMax = countRetValType(f, mostCommonCategory, _) | mostCommonCategory order by categoryMax)
+    and categoryMax = mostCommonRetValType(f, mostCommonCategory)
     and mostCommonCategoryClass = mostCommonCategory
 
-    // if threshold for "most common" use case is 80%, then remaining 20% function calls are handled incorrectly
-    // and ((float)(categoryMax * 100) / categoryCount) >= 80
+    // if threshold for "most common" use case is ~75%, then remaining 25% function calls are handled somehow incorrectly
+    and ((float)(categoryMax * 100) / retValsTotalAmount) >= 74
 
     // // and finally we are looking for calls that use retVal in an uncommon way
-    // and countRetValType(f, buggyCategory, buggyFc) != 0
-    // and buggyCategory != mostCommonCategory
-    // and buggyCategoryClass = buggyCategory 
+    and categorize(f, buggyFc, buggyCategory, _)
+    and buggyCategory != mostCommonCategory
+    and buggyCategoryClass = buggyCategory 
     
-// select buggyFc, "Function $@ is usually compared with " + mostCommonCategoryClass + "(" + categoryMax +
-//     " times), but this call compares with " + buggyCategoryClass, f, f.getName()
-
-select f, mostCommonCategoryClass, categoryMax
-
-
-
-// class RetValCheck {
-
-// }
-
-// predicate typicalRetValComparison(Function f) {
-
-// }
-
-// from Function f, FunctionCall fc, ControlFlowNode test, GuardCondition guard
-// where 
-//   fc.getTarget() = f
-//   and guard.controls(fc.getBasicBlock(), _)
-//   and test = guard.getTest()
-//   and fc = rank[1 .. 5](FunctionCall a, string t, int c |
-//     a.getTarget() = f
-//     and any(RelationalOperation ro | ro = test.getElement() |
-//       ro.getGreaterOperand() = a or ro.getLesserOperand() = a
-//     ).getFullyConverted().getType().getName() = t
-//     order by c = count(fc)
-//     ).first()
-// select f, fc, "Inconsistent return value handling found in function calls to \"" + f.getQualifiedName() + "\". The comparison type used in \"" + fc.toString() + "\" is not the most common comparison type."
\ No newline at end of file
+select buggyFc, "Function $@ return value is usually compared with" + mostCommonCategoryClass + " (" + categoryMax +
+    " times), but this call compares with" + buggyCategoryClass, f, f.getName()
diff --git a/cpp/test/query-tests/security/InconsistentReturnValueHandling/InconsistentReturnValueHandling.cpp b/cpp/test/query-tests/security/InconsistentReturnValueHandling/InconsistentReturnValueHandling.cpp
index 5c58a15..123f3f0 100644
--- a/cpp/test/query-tests/security/InconsistentReturnValueHandling/InconsistentReturnValueHandling.cpp
+++ b/cpp/test/query-tests/security/InconsistentReturnValueHandling/InconsistentReturnValueHandling.cpp
@@ -1,4 +1,4 @@
-#include <stdio.h>
+#include "../../../include/libc/string_stubs.h"
 
 struct something {
     int x;
@@ -59,17 +59,17 @@ int target_func_2(int a)
 }
 
 void test_2_1() {
-    if (target_func_2(2) != sizeof(something)) {
+    if (target_func_2(2) != sizeof(struct something)) {
         puts("something");
     }
 }
 
 void test_2_2() {
-    if (target_func_2(-123) != sizeof(something)) {
+    if (target_func_2(-123) != sizeof(struct something)) {
         puts("something");
     }
     int r = target_func_2(123);
-    if (r > sizeof(something)) {
+    if (r > sizeof(struct something)) {
         return;
     }
 
@@ -87,23 +87,40 @@ int target_func_3(int a)
 }
 
 void test_3_1() {
-    if (target_func_3(2) != sizeof(something)) {
+    if (target_func_3(2) != sizeof(struct something)) {
         puts("something");
     }
 }
 
 void test_3_2() {
+    auto athingInstance = athing{};
+    auto somethingInstance = something{};
+
     if (target_func_3(-123) != sizeof(something)) {
         puts("something");
     }
     int r = target_func_3(123);
-    if (r > sizeof(something)) {
+    if (r > sizeof(struct something)) {
+        return;
+    }
+    if (r > sizeof(somethingInstance)) {
+        return;
+    }
+    if (r >= sizeof(somethingInstance)) {
+        return;
+    }
+    if (r != sizeof(somethingInstance)) {
         return;
     }
 
     r = target_func_3(-3);
     // BAD: comparing with a different sizeof
-    if (r > sizeof(athing)) {
+    if (r > sizeof(struct athing)) {
+        puts("something2");
+    }
+
+    // BAD: comparing with a different sizeof
+    if (r > sizeof(athingInstance)) {
         puts("something2");
     }
 }
@@ -246,13 +263,13 @@ bool some_func_cmp(bool x) {
     return !x;
 }
 
-void test_6_1() {
+void test_7_1() {
     if (some_func_cmp(target_func_7(2))) {
         puts("something");
     }
 }
 
-void test_6_2() {
+void test_7_2() {
     if (some_func_cmp(target_func_7(12)) != cmp_func2(-123)) {
         puts("something");
     }
@@ -268,6 +285,37 @@ void test_6_2() {
     }
 }
 
+// BAD 8
+bool target_func_8(int a)
+{
+    return a > 10 ? true : false;
+}
+int some_func_arithmetic(int x) {
+    return x*3;
+}
+
+void test_8_1() {
+    if (target_func_8(2)*4 == 2*5+6) {
+        puts("something");
+    }
+}
+
+void test_8_2() {
+    if (target_func_8(12) != 4*some_func_arithmetic(-3)) {
+        puts("something");
+    }
+    bool r = target_func_8(123);
+    if (r*2 < sizeof(something)) {
+        return;
+    }
+
+    r = target_func_8(-3);
+    // BAD: comparing with constant instead of dynamically computed value
+    if (r != 3) {
+        puts("something8");
+    }
+}
+
 // GOOD 1: always comparing with NULL
 int* target_func_g1(int a)
 {
diff --git a/cpp/test/query-tests/security/InconsistentReturnValueHandling/InconsistentReturnValueHandling.expected b/cpp/test/query-tests/security/InconsistentReturnValueHandling/InconsistentReturnValueHandling.expected
index 0ae1442..288667c 100644
--- a/cpp/test/query-tests/security/InconsistentReturnValueHandling/InconsistentReturnValueHandling.expected
+++ b/cpp/test/query-tests/security/InconsistentReturnValueHandling/InconsistentReturnValueHandling.expected
@@ -1 +1,8 @@
-| new query passed |
+| InconsistentReturnValueHandling.cpp:35:9:35:21 | call to target_func_1 | Function $@ return value is usually compared with numeric literal (3 times), but this call compares with sizeof(something) | InconsistentReturnValueHandling.cpp:28:5:28:17 | target_func_1 | target_func_1 |
+| InconsistentReturnValueHandling.cpp:76:9:76:21 | call to target_func_2 | Function $@ return value is usually compared with sizeof(something) (3 times), but this call compares with numeric literal | InconsistentReturnValueHandling.cpp:56:5:56:17 | target_func_2 | target_func_2 |
+| InconsistentReturnValueHandling.cpp:116:9:116:21 | call to target_func_3 | Function $@ return value is usually compared with sizeof(something) (6 times), but this call compares with sizeof(athing) | InconsistentReturnValueHandling.cpp:84:5:84:17 | target_func_3 | target_func_3 |
+| InconsistentReturnValueHandling.cpp:188:13:188:25 | call to target_func_4 | Function $@ return value is usually compared with numeric literal (8 times), but this call compares with bool | InconsistentReturnValueHandling.cpp:195:5:195:28 | target_func_4 | target_func_4 |
+| InconsistentReturnValueHandling.cpp:221:9:221:21 | call to target_func_5 | Function $@ return value is usually compared with null (3 times), but this call compares with pointer | InconsistentReturnValueHandling.cpp:201:6:201:18 | target_func_5 | target_func_5 |
+| InconsistentReturnValueHandling.cpp:250:9:250:21 | call to target_func_6 | Function $@ return value is usually compared with some function's return value (3 times), but this call compares with sizeof(something) | InconsistentReturnValueHandling.cpp:230:6:230:18 | target_func_6 | target_func_6 |
+| InconsistentReturnValueHandling.cpp:281:9:281:21 | call to target_func_7 | Function $@ return value is usually compared within call to some function (3 times), but this call compares with some function's return value | InconsistentReturnValueHandling.cpp:258:6:258:18 | target_func_7 | target_func_7 |
+| InconsistentReturnValueHandling.cpp:312:9:312:21 | call to target_func_8 | Function $@ return value is usually compared with arithmetic expression (3 times), but this call compares with numeric literal | InconsistentReturnValueHandling.cpp:289:6:289:18 | target_func_8 | target_func_8 |
diff --git a/cpp/test/query-tests/security/InconsistentReturnValueHandling/InconsistentReturnValueHandling.qlref b/cpp/test/query-tests/security/InconsistentReturnValueHandling/InconsistentReturnValueHandling.qlref
index 112388d..47ca537 100644
--- a/cpp/test/query-tests/security/InconsistentReturnValueHandling/InconsistentReturnValueHandling.qlref
+++ b/cpp/test/query-tests/security/InconsistentReturnValueHandling/InconsistentReturnValueHandling.qlref
@@ -1,2 +1,2 @@
-InconsistentReturnValueHandling/InconsistentReturnValueHandling.ql
+security/InconsistentReturnValueHandling/InconsistentReturnValueHandling.ql
 

From d701d86202385bdf001ca518297dc926290aef7a Mon Sep 17 00:00:00 2001
From: GrosQuildu <e2.8a.95@gmail.com>
Date: Mon, 8 Jul 2024 15:25:35 +0200
Subject: [PATCH 3/6] fix string stubs

---
 cpp/test/include/libc/string_stubs.h | 26 +++++++++++++++++++++++---
 1 file changed, 23 insertions(+), 3 deletions(-)

diff --git a/cpp/test/include/libc/string_stubs.h b/cpp/test/include/libc/string_stubs.h
index 0f7889d..16e9058 100644
--- a/cpp/test/include/libc/string_stubs.h
+++ b/cpp/test/include/libc/string_stubs.h
@@ -1,20 +1,39 @@
 #ifndef USE_HEADERS
 
+#ifndef HEADER_STRINGSTUB_STUB_H
+#define HEADER_STRINGSTUB_STUB_H
+
+#ifdef  __cplusplus
+extern "C" {
+#endif
+
 #ifndef NULL
     #define NULL 0
 #endif
 
-typedef int wchar_t;
+#define wchar_t int
 extern char* strcpy_s(char* dst, int max_amount, char* src);
 extern int _mbsncat(char* dst, char* src, int count);
 extern int _mbsncmp(char* dst, char* src, int count);
 extern int _memicmp(char* a, char* b, long count);
 extern int _mbsnbcmp(char* a, char* b, int count);
-extern void *printf(const char * format, ...);
-extern int strlen(const char *s);
+extern int printf(const char * format, ...);
+extern unsigned long strlen(const char *s);
 extern char* strcpy(char * dst, const char * src);
 extern int wprintf(const wchar_t * format, ...);
 extern wchar_t* wcscpy(wchar_t * s1, const wchar_t * s2);
+extern void perror(const char *s);
+extern int puts(const char *s);
+
+extern void openlog(const char*, int, int);
+extern void syslog(int, const char*, ...);
+extern void closelog(void);
+
+#ifdef  __cplusplus
+}
+#endif
+
+#endif
 
 #else
 
@@ -23,6 +42,7 @@ extern wchar_t* wcscpy(wchar_t * s1, const wchar_t * s2);
 #include <stdio.h>
 #include <stdlib.h>
 #include <wchar.h>
+#include <syslog.h>
 
 #define strcpy_s strcpy
 #define _mbsncat strncat

From f00ccd042935f5480b7c2bbbb2e757be633bd16e Mon Sep 17 00:00:00 2001
From: GrosQuildu <e2.8a.95@gmail.com>
Date: Mon, 8 Jul 2024 18:40:12 +0200
Subject: [PATCH 4/6] InconsistentReturnValueHandling v2 working

---
 .../InconsistentReturnValueHandling.ql        | 126 +++++++++++-------
 .../InconsistentReturnValueHandling.cpp       | 101 +++++++++++++-
 .../InconsistentReturnValueHandling.expected  |  18 +--
 3 files changed, 187 insertions(+), 58 deletions(-)

diff --git a/cpp/src/security/InconsistentReturnValueHandling/InconsistentReturnValueHandling.ql b/cpp/src/security/InconsistentReturnValueHandling/InconsistentReturnValueHandling.ql
index 55aea8c..7766a57 100644
--- a/cpp/src/security/InconsistentReturnValueHandling/InconsistentReturnValueHandling.ql
+++ b/cpp/src/security/InconsistentReturnValueHandling/InconsistentReturnValueHandling.ql
@@ -16,6 +16,9 @@ import cpp
 import semmle.code.cpp.dataflow.new.DataFlow
 import semmle.code.cpp.controlflow.IRGuards
 
+/**
+ * Categories for uses of functions return values
+ */
 newtype TCmpClass =
   Tint()
   or
@@ -53,24 +56,28 @@ class CmpClass extends TCmpClass {
   }
 }
 
-// TODO: why codeql does not have IntegralLiteral?
+/**
+ * Literals like 3, 123, 43, 2
+ */
+pragma[inline]
 predicate numericLiteral(Expr expr) {
-    (
-        expr instanceof Literal
-        and not (
-            expr instanceof BlockExpr
-            or
-            expr instanceof FormatLiteral	
-            or
-            expr instanceof NULL
-            or
-            expr instanceof TextLiteral
-            or
-            expr instanceof LabelLiteral
-            or
-            expr.getType() instanceof NullPointerType
-        )
-    )
+    expr instanceof Literal
+    and not expr instanceof BlockExpr
+    and not expr instanceof FormatLiteral   
+    and not expr instanceof NULL
+    and not expr instanceof TextLiteral
+    and not expr instanceof LabelLiteral
+    and not expr.getType() instanceof NullPointerType
+    and not expr.getType() instanceof BoolType
+}
+
+/**
+ * Literals like 3, 123, -43, +2
+ * TODO: why codeql for cpp does not have IntegralLiteral?
+ */
+pragma[inline]
+predicate numericArithmLiteral(Expr expr) {
+    numericLiteral(expr)
     or
     (
         expr instanceof UnaryArithmeticOperation
@@ -79,6 +86,7 @@ predicate numericLiteral(Expr expr) {
     )
 }
 
+pragma[inline]
 predicate binaryComputation(Expr e) {
     e instanceof BinaryArithmeticOperation
     or
@@ -87,14 +95,23 @@ predicate binaryComputation(Expr e) {
     e instanceof BinaryLogicalOperation
 }
 
+pragma[inline]
+Expr getOtherOperand(ComparisonOperation cmp, Expr fcRetVal) {
+    (cmp.getRightOperand().getAChild*() = fcRetVal and result = cmp.getLeftOperand())
+    or
+    (cmp.getLeftOperand().getAChild*() = fcRetVal and result = cmp.getRightOperand())
+}
+
 /**
- * Categorize expressions using literals instead of types, because
- * we want to differentiate between functions calls and hard-coded stuff 
+ * Categorize expressions using mostly literals instead of types, because
+ * we want to differentiate between functions calls and hard-coded stuff.
+ * We use ugly if-then-else here in hope to speed up computation a bit
  */
+pragma[inline]
 TCmpClass operandCategory(Expr comparedVal) {
-    (numericLiteral(comparedVal) and not comparedVal.getType() instanceof BoolType and result = Tint())
+    (numericArithmLiteral(comparedVal) and result = Tint())
     or
-    (numericLiteral(comparedVal) and comparedVal.getType() instanceof BoolType and result = Tbool())
+    (comparedVal instanceof Literal and comparedVal.getType() instanceof BoolType and result = Tbool())
     or
     ((comparedVal.getType() instanceof NullPointerType or comparedVal instanceof NULL) and result = Tnull())
     or
@@ -113,46 +130,52 @@ TCmpClass operandCategory(Expr comparedVal) {
     (binaryComputation(comparedVal) and result = Tarithm())
 }
 
-module RetValFlowConfig implements DataFlow::ConfigSig {
-    predicate isSource(DataFlow::Node source) {
-        source = source
-    }
+// module RetValFlowConfig implements DataFlow::ConfigSig {
+//     predicate isSource(DataFlow::Node source) {
+//         source.asExpr() = any(FunctionCall f)
+//     }
 
-    predicate isSink(DataFlow::Node sink) {
-        sink = sink
-    }
-}
-module RetValFlow = DataFlow::Global<RetValFlowConfig>;
-
-Expr getOtherOperand(ComparisonOperation cmp, Expr fcRetVal) {
-    if cmp.getRightOperand().getAChild*() = fcRetVal then
-        result = cmp.getLeftOperand()
-    else
-        result = cmp.getRightOperand()
-}
+//     predicate isSink(DataFlow::Node sink) {
+//         exists(IfStmt ifs | ifs.getCondition().getAChild*() = sink.asExpr())
+//     }
+// }
+// module RetValFlow = DataFlow::Global<RetValFlowConfig>;
 
 predicate categorize(Function f, FunctionCall fc, TCmpClass comparedValCategory, IfStmt ifs) {
     exists(Expr fcRetVal |
         fc.getTarget() = f
-        and RetValFlow::flow(
+        and ifs.getCondition().getAChild*() = fcRetVal
+
+        // we could use global DF with a barrier, but that would return a lot of false positives
+        and DataFlow::localFlow(
             DataFlow::exprNode(fc),
             DataFlow::exprNode(fcRetVal)
         )
-        and ifs.getCondition().getAChild*() = fcRetVal
+        
+        // exclude far-reaching flows, when the ret val is not checked but is actually used
+        // in other words, find only the first use in an IF statement 
+        and not exists(IfStmt ifsPrev |
+            ifsPrev != ifs
+            and DataFlow::localFlow(
+                DataFlow::exprNode(fc),
+                DataFlow::exprNode(ifsPrev.getCondition().getAChild*())
+            )
+        )
+
         and
         if
-            // if(func(retVal) == anything)
+            // if(func(retVal))
             exists(FunctionCall anyFc |
                 ifs.getCondition().getAChild*() = anyFc
                 and anyFc.getAnArgument() = fcRetVal
             )
         then
             comparedValCategory = Targ()
-        else
+        else (
             // if (retVal != comparedVal)
-            exists(ComparisonOperation cmp, Expr comparedVal |
+            exists(ComparisonOperation cmp |
                 ifs.getCondition().getAChild*() = cmp
-                and
+                and (
                     // if (2*retVal+1 != comparedVal)
                     // if (retVal > 2*anything()+sizeof(struct))
                     if (
@@ -161,11 +184,13 @@ predicate categorize(Function f, FunctionCall fc, TCmpClass comparedValCategory,
                     )
                     then
                         comparedValCategory = Tarithm()
-                    else
+                    else (
                         // if (retVal != comparedVal)
-                        comparedVal = getOtherOperand(cmp, fcRetVal)
-                        and comparedValCategory = operandCategory(comparedVal)
+                        comparedValCategory = operandCategory(getOtherOperand(cmp, fcRetVal))
+                    )
+                )
             )
+        )
     )
 }
 
@@ -188,12 +213,14 @@ int mostCommonRetValType(Function f, TCmpClass mostCommonCategory) {
 // where
 //     categorize(f, fc, comparedValCategory, ifs)
 //     and x = comparedValCategory
+//     // and f.getName() = "sshbuf_fromb"
 // select f, fc, x, ifs
 
 
 from Function f, int retValsTotalAmount,
     TCmpClass mostCommonCategory, CmpClass mostCommonCategoryClass, int categoryMax,
-    TCmpClass buggyCategory, CmpClass buggyCategoryClass, FunctionCall buggyFc
+    TCmpClass buggyCategory, CmpClass buggyCategoryClass, FunctionCall buggyFc,
+    IfStmt ifs
 where
     // we are interested only in used functions
     exists(FunctionCall fc | fc.getTarget() = f)
@@ -211,9 +238,10 @@ where
     and ((float)(categoryMax * 100) / retValsTotalAmount) >= 74
 
     // // and finally we are looking for calls that use retVal in an uncommon way
-    and categorize(f, buggyFc, buggyCategory, _)
+    and categorize(f, buggyFc, buggyCategory, ifs)
     and buggyCategory != mostCommonCategory
     and buggyCategoryClass = buggyCategory 
     
 select buggyFc, "Function $@ return value is usually compared with" + mostCommonCategoryClass + " (" + categoryMax +
-    " times), but this call compares with" + buggyCategoryClass, f, f.getName()
+    " of " + retValsTotalAmount + " times), but this call compares with" + buggyCategoryClass + " $@",
+    f, f.getName(), ifs, "here"
diff --git a/cpp/test/query-tests/security/InconsistentReturnValueHandling/InconsistentReturnValueHandling.cpp b/cpp/test/query-tests/security/InconsistentReturnValueHandling/InconsistentReturnValueHandling.cpp
index 123f3f0..38fe2c4 100644
--- a/cpp/test/query-tests/security/InconsistentReturnValueHandling/InconsistentReturnValueHandling.cpp
+++ b/cpp/test/query-tests/security/InconsistentReturnValueHandling/InconsistentReturnValueHandling.cpp
@@ -11,6 +11,8 @@ struct athing {
     int y;
 };
 
+static int w = 2;
+
 bool cmp_func(int a)
 {
     return a >= 10 ? true : false;
@@ -103,12 +105,15 @@ void test_3_2() {
     if (r > sizeof(struct something)) {
         return;
     }
+    r = target_func_3(1223);
     if (r > sizeof(somethingInstance)) {
         return;
     }
+    r = target_func_3(1323);
     if (r >= sizeof(somethingInstance)) {
         return;
     }
+    r = target_func_3(1523);
     if (r != sizeof(somethingInstance)) {
         return;
     }
@@ -119,6 +124,7 @@ void test_3_2() {
         puts("something2");
     }
 
+    r = target_func_3(-3);
     // BAD: comparing with a different sizeof
     if (r > sizeof(athingInstance)) {
         puts("something2");
@@ -197,7 +203,6 @@ int SomeClass::target_func_4(int a) {
 }
 
 // BAD 5
-static int w = 2;
 int* target_func_5(int a)
 {
     return &w;
@@ -316,6 +321,34 @@ void test_8_2() {
     }
 }
 
+// BAD 9
+int* target_func_9(int a)
+{
+    return a > 10 ? &w : NULL;
+}
+
+void test_9_1() {
+    if (target_func_9(2) == NULL) {
+        puts("something");
+    }
+}
+
+void test_9_2() {
+    int *tt;
+    if ((tt = target_func_9(12)) != NULL) {
+        puts("something");
+    }
+    tt = target_func_9(123);
+    if (tt == NULL) {
+        return;
+    }
+
+    // BAD: comparing with int instead of NULL
+    if ((int)(tt = target_func_9(-3)) != 0) {
+        puts("something8");
+    }
+}
+
 // GOOD 1: always comparing with NULL
 int* target_func_g1(int a)
 {
@@ -368,4 +401,70 @@ void test_g_2_2() {
     if (r > cmp_func2(-3)) {
         puts("something2");
     }
+}
+
+// GOOD 3: always comparing with int, check only first use
+bool target_func_g3(int a)
+{
+    return a > 10 ? true : false;
+}
+
+void test_g_3_1() {
+    if (target_func_g3(2) != 3) {
+        puts("something");
+    }
+}
+
+void test_g_3_2() {
+    if (target_func_g3(-123) != 4) {
+        puts("something");
+    }
+    bool r = target_func_g3(123);
+    if (r == 777) {
+        return;
+    }
+
+    r = target_func_g3(-3);
+    if (r > -123) {
+        puts("something2");
+    }
+    // We don't want to find this use, because it is not a ret val check
+    // but an actuall use
+    if (cmp_func(r)) {
+        return;
+    }
+}
+
+// GOOD 4: always comparing with int, handle multi-condition IFs
+bool target_func_g4(int a)
+{
+    return a > 10 ? true : false;
+}
+
+void test_g_4_1() {
+    if (target_func_g4(2) != 3) {
+        puts("something");
+    }
+}
+
+void test_g_4_2() {
+    int *www = &w;
+
+    if (target_func_g4(-123) != 4) {
+        puts("something");
+    }
+    bool r = target_func_g4(123);
+    if (r == 777) {
+        return;
+    }
+
+    r = target_func_g4(-3);
+    if (r > -123) {
+        puts("something2");
+    }
+
+    bool rr = target_func_g4(-3);
+    if (www == NULL || rr > -123) {
+        puts("something2");
+    }
 }
\ No newline at end of file
diff --git a/cpp/test/query-tests/security/InconsistentReturnValueHandling/InconsistentReturnValueHandling.expected b/cpp/test/query-tests/security/InconsistentReturnValueHandling/InconsistentReturnValueHandling.expected
index 288667c..decd14a 100644
--- a/cpp/test/query-tests/security/InconsistentReturnValueHandling/InconsistentReturnValueHandling.expected
+++ b/cpp/test/query-tests/security/InconsistentReturnValueHandling/InconsistentReturnValueHandling.expected
@@ -1,8 +1,10 @@
-| InconsistentReturnValueHandling.cpp:35:9:35:21 | call to target_func_1 | Function $@ return value is usually compared with numeric literal (3 times), but this call compares with sizeof(something) | InconsistentReturnValueHandling.cpp:28:5:28:17 | target_func_1 | target_func_1 |
-| InconsistentReturnValueHandling.cpp:76:9:76:21 | call to target_func_2 | Function $@ return value is usually compared with sizeof(something) (3 times), but this call compares with numeric literal | InconsistentReturnValueHandling.cpp:56:5:56:17 | target_func_2 | target_func_2 |
-| InconsistentReturnValueHandling.cpp:116:9:116:21 | call to target_func_3 | Function $@ return value is usually compared with sizeof(something) (6 times), but this call compares with sizeof(athing) | InconsistentReturnValueHandling.cpp:84:5:84:17 | target_func_3 | target_func_3 |
-| InconsistentReturnValueHandling.cpp:188:13:188:25 | call to target_func_4 | Function $@ return value is usually compared with numeric literal (8 times), but this call compares with bool | InconsistentReturnValueHandling.cpp:195:5:195:28 | target_func_4 | target_func_4 |
-| InconsistentReturnValueHandling.cpp:221:9:221:21 | call to target_func_5 | Function $@ return value is usually compared with null (3 times), but this call compares with pointer | InconsistentReturnValueHandling.cpp:201:6:201:18 | target_func_5 | target_func_5 |
-| InconsistentReturnValueHandling.cpp:250:9:250:21 | call to target_func_6 | Function $@ return value is usually compared with some function's return value (3 times), but this call compares with sizeof(something) | InconsistentReturnValueHandling.cpp:230:6:230:18 | target_func_6 | target_func_6 |
-| InconsistentReturnValueHandling.cpp:281:9:281:21 | call to target_func_7 | Function $@ return value is usually compared within call to some function (3 times), but this call compares with some function's return value | InconsistentReturnValueHandling.cpp:258:6:258:18 | target_func_7 | target_func_7 |
-| InconsistentReturnValueHandling.cpp:312:9:312:21 | call to target_func_8 | Function $@ return value is usually compared with arithmetic expression (3 times), but this call compares with numeric literal | InconsistentReturnValueHandling.cpp:289:6:289:18 | target_func_8 | target_func_8 |
+| InconsistentReturnValueHandling.cpp:37:9:37:21 | call to target_func_1 | Function $@ return value is usually compared with numeric literal (3 of 4 times), but this call compares with sizeof(something) | InconsistentReturnValueHandling.cpp:30:5:30:17 | target_func_1 | target_func_1 |
+| InconsistentReturnValueHandling.cpp:78:9:78:21 | call to target_func_2 | Function $@ return value is usually compared with sizeof(something) (3 of 4 times), but this call compares with numeric literal | InconsistentReturnValueHandling.cpp:58:5:58:17 | target_func_2 | target_func_2 |
+| InconsistentReturnValueHandling.cpp:121:9:121:21 | call to target_func_3 | Function $@ return value is usually compared with sizeof(something) (6 of 8 times), but this call compares with sizeof(athing) | InconsistentReturnValueHandling.cpp:86:5:86:17 | target_func_3 | target_func_3 |
+| InconsistentReturnValueHandling.cpp:127:9:127:21 | call to target_func_3 | Function $@ return value is usually compared with sizeof(something) (6 of 8 times), but this call compares with sizeof(athing) | InconsistentReturnValueHandling.cpp:86:5:86:17 | target_func_3 | target_func_3 |
+| InconsistentReturnValueHandling.cpp:194:13:194:25 | call to target_func_4 | Function $@ return value is usually compared with numeric literal (8 of 9 times), but this call compares with bool | InconsistentReturnValueHandling.cpp:201:5:201:28 | target_func_4 | target_func_4 |
+| InconsistentReturnValueHandling.cpp:226:9:226:21 | call to target_func_5 | Function $@ return value is usually compared with null (3 of 4 times), but this call compares with pointer | InconsistentReturnValueHandling.cpp:206:6:206:18 | target_func_5 | target_func_5 |
+| InconsistentReturnValueHandling.cpp:255:9:255:21 | call to target_func_6 | Function $@ return value is usually compared with some function's return value (3 of 4 times), but this call compares with sizeof(something) | InconsistentReturnValueHandling.cpp:235:6:235:18 | target_func_6 | target_func_6 |
+| InconsistentReturnValueHandling.cpp:286:9:286:21 | call to target_func_7 | Function $@ return value is usually compared within call to some function (3 of 4 times), but this call compares with some function's return value | InconsistentReturnValueHandling.cpp:263:6:263:18 | target_func_7 | target_func_7 |
+| InconsistentReturnValueHandling.cpp:317:9:317:21 | call to target_func_8 | Function $@ return value is usually compared with arithmetic expression (3 of 4 times), but this call compares with numeric literal | InconsistentReturnValueHandling.cpp:294:6:294:18 | target_func_8 | target_func_8 |
+| InconsistentReturnValueHandling.cpp:347:20:347:32 | call to target_func_9 | Function $@ return value is usually compared with null (3 of 4 times), but this call compares with numeric literal | InconsistentReturnValueHandling.cpp:325:6:325:18 | target_func_9 | target_func_9 |

From cc57e58e6c651dd338dc65e18d8178674df79c9a Mon Sep 17 00:00:00 2001
From: GrosQuildu <e2.8a.95@gmail.com>
Date: Mon, 8 Jul 2024 19:08:43 +0200
Subject: [PATCH 5/6] InconsistentReturnValueHandling - reduced FPs

---
 .../InconsistentReturnValueHandling.ql        | 46 +++++++++++++------
 .../InconsistentReturnValueHandling.expected  | 20 ++++----
 2 files changed, 43 insertions(+), 23 deletions(-)

diff --git a/cpp/src/security/InconsistentReturnValueHandling/InconsistentReturnValueHandling.ql b/cpp/src/security/InconsistentReturnValueHandling/InconsistentReturnValueHandling.ql
index 7766a57..c9120fa 100644
--- a/cpp/src/security/InconsistentReturnValueHandling/InconsistentReturnValueHandling.ql
+++ b/cpp/src/security/InconsistentReturnValueHandling/InconsistentReturnValueHandling.ql
@@ -17,7 +17,7 @@ import semmle.code.cpp.dataflow.new.DataFlow
 import semmle.code.cpp.controlflow.IRGuards
 
 /**
- * Categories for uses of functions return values
+ * Categories for uses of functions' return values
  */
 newtype TCmpClass =
   Tint()
@@ -105,7 +105,6 @@ Expr getOtherOperand(ComparisonOperation cmp, Expr fcRetVal) {
 /**
  * Categorize expressions using mostly literals instead of types, because
  * we want to differentiate between functions calls and hard-coded stuff.
- * We use ugly if-then-else here in hope to speed up computation a bit
  */
 pragma[inline]
 TCmpClass operandCategory(Expr comparedVal) {
@@ -117,7 +116,7 @@ TCmpClass operandCategory(Expr comparedVal) {
     or
     (comparedVal.getUnderlyingType() instanceof DerivedType and result = Tptr())
     or
-    (comparedVal instanceof FunctionCall and result = Tcall())
+    (comparedVal instanceof Call and result = Tcall())
     or
     (comparedVal instanceof SizeofOperator and
         (
@@ -132,7 +131,7 @@ TCmpClass operandCategory(Expr comparedVal) {
 
 // module RetValFlowConfig implements DataFlow::ConfigSig {
 //     predicate isSource(DataFlow::Node source) {
-//         source.asExpr() = any(FunctionCall f)
+//         source.asExpr() = any(Call f)
 //     }
 
 //     predicate isSink(DataFlow::Node sink) {
@@ -141,7 +140,11 @@ TCmpClass operandCategory(Expr comparedVal) {
 // }
 // module RetValFlow = DataFlow::Global<RetValFlowConfig>;
 
-predicate categorize(Function f, FunctionCall fc, TCmpClass comparedValCategory, IfStmt ifs) {
+/**
+ * Given function's return value, find its first use in an IF statement
+ * and assign proper TCmpClass category
+ */
+predicate categorize(Function f, Call fc, TCmpClass comparedValCategory, IfStmt ifs) {
     exists(Expr fcRetVal |
         fc.getTarget() = f
         and ifs.getCondition().getAChild*() = fcRetVal
@@ -165,9 +168,9 @@ predicate categorize(Function f, FunctionCall fc, TCmpClass comparedValCategory,
         and
         if
             // if(func(retVal))
-            exists(FunctionCall anyFc |
+            exists(Call anyFc |
                 ifs.getCondition().getAChild*() = anyFc
-                and anyFc.getAnArgument() = fcRetVal
+                and anyFc.getAnArgument().getAChild*() = fcRetVal
             )
         then
             comparedValCategory = Targ()
@@ -175,6 +178,11 @@ predicate categorize(Function f, FunctionCall fc, TCmpClass comparedValCategory,
             // if (retVal != comparedVal)
             exists(ComparisonOperation cmp |
                 ifs.getCondition().getAChild*() = cmp
+                // skip if we are passing the return value to some function
+                and not exists(Call tmpCall |
+                    cmp.getAChild*() = tmpCall
+                    and tmpCall.getAnArgument().getAChild*() = fcRetVal
+                )
                 and (
                     // if (2*retVal+1 != comparedVal)
                     // if (retVal > 2*anything()+sizeof(struct))
@@ -194,6 +202,10 @@ predicate categorize(Function f, FunctionCall fc, TCmpClass comparedValCategory,
     )
 }
 
+/**
+ * Count all eligible IF statements that
+ * checks return values of the given function
+ */
 int countAllRetValTypes(Function f) {
     result = count(IfStmt ifs |
         categorize(f, _, _, ifs) |
@@ -201,6 +213,10 @@ int countAllRetValTypes(Function f) {
     )
 }
 
+/**
+ * Determine what is the most commont TCmpClass category
+ * for the given function (by counting eligible IF statements)
+ */
 int mostCommonRetValType(Function f, TCmpClass mostCommonCategory) {
     result = max(int numberOfRetValTypeInstances |
         categorize(f, _, mostCommonCategory, _)
@@ -209,7 +225,7 @@ int mostCommonRetValType(Function f, TCmpClass mostCommonCategory) {
 }
 
 // uncomment for testing:
-// from Function f, FunctionCall fc, TCmpClass comparedValCategory, CmpClass x, IfStmt ifs
+// from Function f, Call fc, TCmpClass comparedValCategory, CmpClass x, IfStmt ifs
 // where
 //     categorize(f, fc, comparedValCategory, ifs)
 //     and x = comparedValCategory
@@ -219,12 +235,12 @@ int mostCommonRetValType(Function f, TCmpClass mostCommonCategory) {
 
 from Function f, int retValsTotalAmount,
     TCmpClass mostCommonCategory, CmpClass mostCommonCategoryClass, int categoryMax,
-    TCmpClass buggyCategory, CmpClass buggyCategoryClass, FunctionCall buggyFc,
+    TCmpClass buggyCategory, CmpClass buggyCategoryClass, Call buggyFc,
     IfStmt ifs
 where
-    // we are interested only in used functions
-    exists(FunctionCall fc | fc.getTarget() = f)
-    // and f.hasDefinition()
+    // we are interested only in defined (e.g., not libc) and used functions
+    exists(Call fc | fc.getTarget() = f)
+    and f.hasDefinition()
 
     // the function's retVal must be used in some IF statements
     and retValsTotalAmount = countAllRetValTypes(f)
@@ -240,7 +256,11 @@ where
     // // and finally we are looking for calls that use retVal in an uncommon way
     and categorize(f, buggyFc, buggyCategory, ifs)
     and buggyCategory != mostCommonCategory
-    and buggyCategoryClass = buggyCategory 
+    and buggyCategoryClass = buggyCategory
+
+    // return value could be used multiple times in a single IF statement
+    // don't show such findings
+    and not categoryMax = retValsTotalAmount
     
 select buggyFc, "Function $@ return value is usually compared with" + mostCommonCategoryClass + " (" + categoryMax +
     " of " + retValsTotalAmount + " times), but this call compares with" + buggyCategoryClass + " $@",
diff --git a/cpp/test/query-tests/security/InconsistentReturnValueHandling/InconsistentReturnValueHandling.expected b/cpp/test/query-tests/security/InconsistentReturnValueHandling/InconsistentReturnValueHandling.expected
index decd14a..6d06444 100644
--- a/cpp/test/query-tests/security/InconsistentReturnValueHandling/InconsistentReturnValueHandling.expected
+++ b/cpp/test/query-tests/security/InconsistentReturnValueHandling/InconsistentReturnValueHandling.expected
@@ -1,10 +1,10 @@
-| InconsistentReturnValueHandling.cpp:37:9:37:21 | call to target_func_1 | Function $@ return value is usually compared with numeric literal (3 of 4 times), but this call compares with sizeof(something) | InconsistentReturnValueHandling.cpp:30:5:30:17 | target_func_1 | target_func_1 |
-| InconsistentReturnValueHandling.cpp:78:9:78:21 | call to target_func_2 | Function $@ return value is usually compared with sizeof(something) (3 of 4 times), but this call compares with numeric literal | InconsistentReturnValueHandling.cpp:58:5:58:17 | target_func_2 | target_func_2 |
-| InconsistentReturnValueHandling.cpp:121:9:121:21 | call to target_func_3 | Function $@ return value is usually compared with sizeof(something) (6 of 8 times), but this call compares with sizeof(athing) | InconsistentReturnValueHandling.cpp:86:5:86:17 | target_func_3 | target_func_3 |
-| InconsistentReturnValueHandling.cpp:127:9:127:21 | call to target_func_3 | Function $@ return value is usually compared with sizeof(something) (6 of 8 times), but this call compares with sizeof(athing) | InconsistentReturnValueHandling.cpp:86:5:86:17 | target_func_3 | target_func_3 |
-| InconsistentReturnValueHandling.cpp:194:13:194:25 | call to target_func_4 | Function $@ return value is usually compared with numeric literal (8 of 9 times), but this call compares with bool | InconsistentReturnValueHandling.cpp:201:5:201:28 | target_func_4 | target_func_4 |
-| InconsistentReturnValueHandling.cpp:226:9:226:21 | call to target_func_5 | Function $@ return value is usually compared with null (3 of 4 times), but this call compares with pointer | InconsistentReturnValueHandling.cpp:206:6:206:18 | target_func_5 | target_func_5 |
-| InconsistentReturnValueHandling.cpp:255:9:255:21 | call to target_func_6 | Function $@ return value is usually compared with some function's return value (3 of 4 times), but this call compares with sizeof(something) | InconsistentReturnValueHandling.cpp:235:6:235:18 | target_func_6 | target_func_6 |
-| InconsistentReturnValueHandling.cpp:286:9:286:21 | call to target_func_7 | Function $@ return value is usually compared within call to some function (3 of 4 times), but this call compares with some function's return value | InconsistentReturnValueHandling.cpp:263:6:263:18 | target_func_7 | target_func_7 |
-| InconsistentReturnValueHandling.cpp:317:9:317:21 | call to target_func_8 | Function $@ return value is usually compared with arithmetic expression (3 of 4 times), but this call compares with numeric literal | InconsistentReturnValueHandling.cpp:294:6:294:18 | target_func_8 | target_func_8 |
-| InconsistentReturnValueHandling.cpp:347:20:347:32 | call to target_func_9 | Function $@ return value is usually compared with null (3 of 4 times), but this call compares with numeric literal | InconsistentReturnValueHandling.cpp:325:6:325:18 | target_func_9 | target_func_9 |
+| InconsistentReturnValueHandling.cpp:37:9:37:21 | call to target_func_1 | Function $@ return value is usually compared with numeric literal (3 of 4 times), but this call compares with sizeof(something) $@ | InconsistentReturnValueHandling.cpp:30:5:30:17 | target_func_1 | target_func_1 | InconsistentReturnValueHandling.cpp:37:5:39:5 | if (...) ...  | here |
+| InconsistentReturnValueHandling.cpp:78:9:78:21 | call to target_func_2 | Function $@ return value is usually compared with sizeof(something) (3 of 4 times), but this call compares with numeric literal $@ | InconsistentReturnValueHandling.cpp:58:5:58:17 | target_func_2 | target_func_2 | InconsistentReturnValueHandling.cpp:80:5:82:5 | if (...) ...  | here |
+| InconsistentReturnValueHandling.cpp:121:9:121:21 | call to target_func_3 | Function $@ return value is usually compared with sizeof(something) (6 of 8 times), but this call compares with sizeof(athing) $@ | InconsistentReturnValueHandling.cpp:86:5:86:17 | target_func_3 | target_func_3 | InconsistentReturnValueHandling.cpp:123:5:125:5 | if (...) ...  | here |
+| InconsistentReturnValueHandling.cpp:127:9:127:21 | call to target_func_3 | Function $@ return value is usually compared with sizeof(something) (6 of 8 times), but this call compares with sizeof(athing) $@ | InconsistentReturnValueHandling.cpp:86:5:86:17 | target_func_3 | target_func_3 | InconsistentReturnValueHandling.cpp:129:5:131:5 | if (...) ...  | here |
+| InconsistentReturnValueHandling.cpp:194:13:194:25 | call to target_func_4 | Function $@ return value is usually compared with numeric literal (8 of 9 times), but this call compares with bool $@ | InconsistentReturnValueHandling.cpp:201:5:201:28 | target_func_4 | target_func_4 | InconsistentReturnValueHandling.cpp:196:5:198:5 | if (...) ...  | here |
+| InconsistentReturnValueHandling.cpp:226:9:226:21 | call to target_func_5 | Function $@ return value is usually compared with null (3 of 4 times), but this call compares with pointer $@ | InconsistentReturnValueHandling.cpp:206:6:206:18 | target_func_5 | target_func_5 | InconsistentReturnValueHandling.cpp:229:5:231:5 | if (...) ...  | here |
+| InconsistentReturnValueHandling.cpp:255:9:255:21 | call to target_func_6 | Function $@ return value is usually compared with some function's return value (3 of 4 times), but this call compares with sizeof(something) $@ | InconsistentReturnValueHandling.cpp:235:6:235:18 | target_func_6 | target_func_6 | InconsistentReturnValueHandling.cpp:257:5:259:5 | if (...) ...  | here |
+| InconsistentReturnValueHandling.cpp:286:9:286:21 | call to target_func_7 | Function $@ return value is usually compared within call to some function (3 of 4 times), but this call compares with some function's return value $@ | InconsistentReturnValueHandling.cpp:263:6:263:18 | target_func_7 | target_func_7 | InconsistentReturnValueHandling.cpp:288:5:290:5 | if (...) ...  | here |
+| InconsistentReturnValueHandling.cpp:317:9:317:21 | call to target_func_8 | Function $@ return value is usually compared with arithmetic expression (3 of 4 times), but this call compares with numeric literal $@ | InconsistentReturnValueHandling.cpp:294:6:294:18 | target_func_8 | target_func_8 | InconsistentReturnValueHandling.cpp:319:5:321:5 | if (...) ...  | here |
+| InconsistentReturnValueHandling.cpp:347:20:347:32 | call to target_func_9 | Function $@ return value is usually compared with null (3 of 4 times), but this call compares with numeric literal $@ | InconsistentReturnValueHandling.cpp:325:6:325:18 | target_func_9 | target_func_9 | InconsistentReturnValueHandling.cpp:347:5:349:5 | if (...) ...  | here |

From 72351a976f1ff5eb4c980165acdc7f1930a7a154 Mon Sep 17 00:00:00 2001
From: GrosQuildu <e2.8a.95@gmail.com>
Date: Tue, 9 Jul 2024 10:32:47 +0200
Subject: [PATCH 6/6] InconsistentReturnValueHandling - small changes

---
 .../InconsistentReturnValueHandling.ql                       | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/cpp/src/security/InconsistentReturnValueHandling/InconsistentReturnValueHandling.ql b/cpp/src/security/InconsistentReturnValueHandling/InconsistentReturnValueHandling.ql
index c9120fa..9e905a0 100644
--- a/cpp/src/security/InconsistentReturnValueHandling/InconsistentReturnValueHandling.ql
+++ b/cpp/src/security/InconsistentReturnValueHandling/InconsistentReturnValueHandling.ql
@@ -52,7 +52,7 @@ class CmpClass extends TCmpClass {
     or
     this = Tarithm() and result = " arithmetic expression"
     or
-    this = Targ() and result = "in call to some function"
+    this = Targ() and result = "in a function"
   }
 }
 
@@ -238,8 +238,9 @@ from Function f, int retValsTotalAmount,
     TCmpClass buggyCategory, CmpClass buggyCategoryClass, Call buggyFc,
     IfStmt ifs
 where
+    not buggyFc.getLocation().getFile().toString().toLowerCase().regexpMatch(".*test.*")
     // we are interested only in defined (e.g., not libc) and used functions
-    exists(Call fc | fc.getTarget() = f)
+    and exists(Call fc | fc.getTarget() = f)
     and f.hasDefinition()
 
     // the function's retVal must be used in some IF statements