Merge pull request #405 from geoffw0/selfcompare

CPP: Fix false positives in PointlessSelfComparison.ql

Author: jbj

change-notes/1.19/analysis-cpp.md

@@ -21,6 +21,7 @@
 | Missing return statement (`cpp/missing-return`) | Visible by default | The precision of this query has been increased from 'medium' to 'high', which makes it visible by default in LGTM. It was 'medium' in release 1.17 and 1.18 because it had false positives due to an extractor bug that was fixed in 1.18. |
 | Missing return statement | Fewer false positive results | The query is now produces correct results when a function returns a template-dependent type. |
 | Call to memory access function may overflow buffer | More correct results | Array indexing with a negative index is now detected by this query. |
+| Self comparison | Fewer false positive results | Code inside macro invocations is now excluded from the query. |
 | Suspicious call to memset | Fewer false positive results | Types involving decltype are now correctly compared. |
 | Suspicious add with sizeof | Fewer false positive results | Arithmetic with void pointers (where allowed) is now excluded from this query. |
 | Wrong type of arguments to formatting function | Fewer false positive results | False positive results involving typedefs have been removed.  Expected argument types are determined more accurately, especially for wide string and pointer types.  Custom (non-standard) formatting functions are also identified more accurately. |

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

@@ -18,4 +18,12 @@ from ComparisonOperation cmp
 where pointlessSelfComparison(cmp)
   and not nanTest(cmp)
   and not overflowTest(cmp)
+  and not exists(MacroInvocation mi | 
+    // cmp is in mi 
+    mi.getAnExpandedElement() = cmp and 
+  
+    // and cmp was apparently not passed in as a macro parameter 
+    cmp.getLocation().getStartLine() = mi.getLocation().getStartLine() and 
+    cmp.getLocation().getStartColumn() = mi.getLocation().getStartColumn() 
+  ) 
 select cmp, "Self comparison."

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

@@ -3,3 +3,4 @@
 | test.cpp:83:10:83:15 | ... == ... | Self comparison. |
 | test.cpp:90:10:90:15 | ... == ... | Self comparison. |
 | test.cpp:118:7:118:32 | ... != ... | Self comparison. |
+| test.cpp:151:11:151:16 | ... == ... | Self comparison. |

cpp/ql/test/query-tests/Likely Bugs/Arithmetic/BadAdditionOverflowCheck/test.cpp

@@ -123,3 +123,30 @@ int isSmallEnough(unsigned long long x) {
   // get compiled away on others.
   return x == (size_t)x && x == (u64)x; // GOOD
 }
+
+#define markRange(str, x, y) \
+	if ((x) == (y)) { \
+		str[x] = '^'; \
+	} else { \
+		int i; \
+		str[x] = '<'; \
+		for (i = x + 1; i < y; i++) { \
+			str[i] = '-'; \
+		} \
+		str[y] = '>'; \
+	}
+
+void useMarkRange(int offs) {
+	char buffer[100];
+
+	markRange(buffer, 10, 20);
+	markRange(buffer, 30, 30);
+	markRange(buffer, offs, offs + 10);
+	markRange(buffer, offs, offs); // GOOD (comparison is in the macro)
+}
+
+#define MY_MACRO(x) (x)
+
+void myMacroTest(int x) {
+	MY_MACRO(x == x); // BAD
+}