Merge pull request #1605 from geoffw0/bitwiseneg

CPP: Make BitwiseSignCheck.ql more accurate

Author: Robert Marsh

change-notes/1.22/analysis-cpp.md

@@ -15,6 +15,7 @@
 | Continue statement that does not continue (`cpp/continue-in-false-loop`) | Fewer false positive results | Analysis is now restricted to `do`-`while` loops. This query is now run and displayed by default on LGTM. |
 | Expression has no effect (`cpp/useless-expression`) | Fewer false positive results | Calls to functions with the `weak` attribute are no longer considered to be side effect free, because they could be overridden with a different implementation at link time. |
 | No space for zero terminator (`cpp/no-space-for-terminator`) | Fewer false positive results | False positives involving strings that are not null-terminated have been excluded. |
+| Sign check of bitwise operation (`cpp/bitwise-sign-check`) | Fewer false positive results and more true positive results | The query now understands the direction of each comparison, making it more accurate. |
 | Suspicious pointer scaling (`cpp/suspicious-pointer-scaling`) | Lower precision | The precision of this query has been reduced to "medium". This coding pattern is used intentionally and safely in a number of real-world projects. Results are no longer displayed on LGTM unless you choose to display them. |
 | Non-constant format string (`cpp/non-constant-format`) | Fewer false positive results | Rewritten using the taint-tracking library. |
 | Variable used in its own initializer (`cpp/use-in-own-initializer`) | Fewer false positive results | False positives for constant variables with the same name in different namespaces have been removed. |

cpp/ql/src/Likely Bugs/Arithmetic/BitwiseSignCheck.ql

@@ -12,9 +12,9 @@
 import cpp
 
 from RelationalOperation e, BinaryBitwiseOperation lhs
-where lhs = e.getLeftOperand() and
+where lhs = e.getGreaterOperand() and
       lhs.getActualType().(IntegralType).isSigned() and
       forall(int op | op = lhs.(BitwiseAndExpr).getAnOperand().getValue().toInt() | op < 0) and
-      e.getRightOperand().getValue() = "0" and
+      e.getLesserOperand().getValue() = "0" and
       not e.isAffectedByMacro()
 select e, "Potential unsafe sign check of a bitwise operation."

cpp/ql/test/query-tests/Likely Bugs/Arithmetic/BitwiseSignCheck/BitwiseSignCheck.expected

@@ -1,3 +1,5 @@
 | bsc.cpp:2:10:2:32 | ... > ... | Potential unsafe sign check of a bitwise operation. |
 | bsc.cpp:6:10:6:32 | ... > ... | Potential unsafe sign check of a bitwise operation. |
 | bsc.cpp:10:10:10:33 | ... >= ... | Potential unsafe sign check of a bitwise operation. |
+| bsc.cpp:18:10:18:28 | ... > ... | Potential unsafe sign check of a bitwise operation. |
+| bsc.cpp:22:10:22:28 | ... < ... | Potential unsafe sign check of a bitwise operation. |

cpp/ql/test/query-tests/Likely Bugs/Arithmetic/BitwiseSignCheck/bsc.cpp

@@ -1,15 +1,31 @@
 bool is_bit_set_v1(int x, int bitnum) {
-  return (x & (1 << bitnum)) > 0;
+  return (x & (1 << bitnum)) > 0; // BAD
 }
 
 bool is_bit_set_v2(int x, int bitnum) {
-  return ((1 << bitnum) & x) > 0;
+  return ((1 << bitnum) & x) > 0; // BAD
 }
 
 bool plain_wrong(int x, int bitnum) {
-  return (x & (1 << bitnum)) >= 0;
+  return (x & (1 << bitnum)) >= 0; // ???
 }
 
 bool is_bit24_set(int x) {
-  return (x & (1 << 24)) > 0;
+  return (x & (1 << 24)) > 0; // GOOD (result will always be positive)
+}
+
+bool is_bit31_set_bad_v1(int x) {
+  return (x & (1 << 31)) > 0; // BAD
+}
+
+bool is_bit31_set_bad_v2(int x) {
+  return 0 < (x & (1 << 31)); // BAD
+}
+
+bool is_bit31_set_good(int x) {
+  return (x & (1 << 31)) != 0; // GOOD (uses `!=`)
+}
+
+bool deliberately_checking_sign(int x, int y) {
+  return (x & y) < 0; // GOOD (use of `<` implies the sign check is intended)
 }