Merge pull request github#2122 from geoffw0/bitsign2

CPP: BitwiseSignCheck.ql fix

Author: jbj

change-notes/1.23/analysis-cpp.md

@@ -18,6 +18,7 @@ The following changes in version 1.23 affect C/C++ analysis in all applications.
 | Hard-coded Japanese era start date in call (`cpp/japanese-era/constructor-or-method-with-exact-era-date`) | Deprecated | This query has been deprecated.  Use the new combined query Hard-coded Japanese era start date (`cpp/japanese-era/exact-era-date`) instead. |
 | Hard-coded Japanese era start date in struct (`cpp/japanese-era/struct-with-exact-era-date`) | Deprecated | This query has been deprecated.  Use the new combined query Hard-coded Japanese era start date (`cpp/japanese-era/exact-era-date`) instead. |
 | Hard-coded Japanese era start date (`cpp/japanese-era/exact-era-date`) | More correct results | This query now checks for the beginning date of the Reiwa era (1st May 2019). |
+| Sign check of bitwise operation (`cpp/bitwise-sign-check`) | Fewer false positive results | Results involving `>=` or `<=` are no longer reported. |
 | Too few arguments to formatting function (`cpp/wrong-number-format-arguments`) | Fewer false positive results | Fixed false positives resulting from mistmatching declarations of a formatting function. |
 | Too many arguments to formatting function (`cpp/too-many-format-arguments`) | Fewer false positive results | Fixed false positives resulting from mistmatching declarations of a formatting function. |
 | Unclear comparison precedence (`cpp/comparison-precedence`) | Fewer false positive results | False positives involving template classes and functions have been fixed. |

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

@@ -14,9 +14,16 @@ import cpp
 
 from RelationalOperation e, BinaryBitwiseOperation lhs
 where
-  lhs = e.getGreaterOperand() and
+  // `lhs > 0` (or `0 < lhs`)
+  // (note that `lhs < 0`, `lhs >= 0` or `lhs <= 0` all imply that the signedness of
+  //  `lhs` is understood, so should not be flagged).
+  (e instanceof GTExpr or e instanceof LTExpr) and
+  e.getGreaterOperand() = lhs and
+  e.getLesserOperand().getValue() = "0" and
+  // lhs is signed
   lhs.getActualType().(IntegralType).isSigned() and
+  // if `lhs` has the form `x & c`, with constant `c`, `c` is negative
   forall(int op | op = lhs.(BitwiseAndExpr).getAnOperand().getValue().toInt() | op < 0) and
-  e.getLesserOperand().getValue() = "0" and
+  // exception for cases involving macros
   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,5 +1,4 @@
 | 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

@@ -7,7 +7,7 @@ bool is_bit_set_v2(int x, int bitnum) {
 }
 
 bool plain_wrong(int x, int bitnum) {
-  return (x & (1 << bitnum)) >= 0; // ???
+  return (x & (1 << bitnum)) >= 0; // GOOD (testing for `>= 0` is the logical negation of `< 0`, a negativity test)
 }
 
 bool is_bit24_set(int x) {
@@ -27,5 +27,17 @@ bool is_bit31_set_good(int x) {
 }
 
 bool deliberately_checking_sign(int x, int y) {
-  return (x & y) < 0; // GOOD (use of `<` implies the sign check is intended)
+  return (x & y) < 0; // GOOD (testing for negativity rather the positivity implies that signed values are being considered intentionally by the developer)
+}
+
+bool deliberately_checking_sign2(int x, int y) {
+  return (x & y) >= 0; // GOOD (testing for `>= 0` is the logical negation of `< 0`, a negativity test)
+}
+
+bool is_bit_set_v3(int x, int bitnum) {
+  return (x & (1 << bitnum)) <= 0; // GOOD (testing for `<= 0` is the logical negation of `> 0`, a positivity test, but the way it's written suggests the developer considers the value to be signed)
+}
+
+bool is_bit_set_v4(int x, int bitnum) {
+  return (x & (1 << bitnum)) >= 1; // BAD [NOT DETECTED]
 }