[PATCH] D66049: [analyzer] PR41729: Fix some false positives and improve strlcat and strlcpy modeling

[Kristóf Umann via Phabricator via cfe-commits]

Szelethus added a comment.

The logic of the patch is fine in my eyes, but I dislike the formatting. Could you please make the code obey the LLVM coding style, and after that use clang-format-diff.py?
https://llvm.org/docs/CodingStandards.html
https://clang.llvm.org/docs/ClangFormat.html#script-for-patch-reformatting



================
Comment at: lib/StaticAnalyzer/Checkers/CStringChecker.cpp:31
 namespace {
+enum class appendKind { none = 0, strcat = 1, strlcat = 2 };
 class CStringChecker : public Checker< eval::Call,
----------------
Please capitalize these. Also, `ConcatFnKind` might be more descriptive? Ad absurdum, we might as well pass the `CallDescription` that is responsible for the concatenation. That maaay just be a little over engineered though.


================
Comment at: lib/StaticAnalyzer/Checkers/CStringChecker.cpp:133-134
   void evalStrlcpy(CheckerContext &C, const CallExpr *CE) const;
-  void evalStrcpyCommon(CheckerContext &C,
-                        const CallExpr *CE,
-                        bool returnEnd,
-                        bool isBounded,
-                        bool isAppending,
+  void evalStrcpyCommon(CheckerContext &C, const CallExpr *CE, bool returnEnd,
+                        bool isBounded, appendKind appendK,
                         bool returnPtr = true) const;
----------------
CaPitALizeE :D


================
Comment at: lib/StaticAnalyzer/Checkers/CStringChecker.cpp:1556
+
+  // Get the string length of the destination
+  SVal dstStrLength = getCStringLength(C, state, Dst, DstVal);
----------------
Please terminate this sentence.


================
Comment at: lib/StaticAnalyzer/Checkers/CStringChecker.cpp:1580
 
   // If the function is strncpy, strncat, etc... it is bounded.
   if (isBounded) {
----------------
Ah, okay, so the assumption is that bounded functions' third argument is always a numerical size parameter. Why isn't that enforced at all?


================
Comment at: lib/StaticAnalyzer/Checkers/CStringChecker.cpp:1596
 
-      // Check if the max number to copy is less than the length of the src.
-      // If the bound is equal to the source length, strncpy won't null-
-      // terminate the result!
-      std::tie(stateSourceTooLong, stateSourceNotTooLong) = state->assume(
-          svalBuilder.evalBinOpNN(state, BO_GE, *strLengthNL, *lenValNL, cmpTy)
-              .castAs<DefinedOrUnknownSVal>());
+      if (appendK == appendKind::none || appendK == appendKind::strcat) {
+        ProgramStateRef stateSourceTooLong, stateSourceNotTooLong;
----------------
Can we just do a switch-case? If someone were to add a new concat type, it would even give a warning in case it was left unhandled.


================
Comment at: lib/StaticAnalyzer/Checkers/CStringChecker.cpp:1628
+          if (Optional<NonLoc> freeSpaceNL = freeSpace.getAs<NonLoc>()) {
+            // Symbolic expression not too complex.
+
----------------
Please put comments like these before the branch. I like the following format better:

```lang=c++
// While unlikely, it is possible that the subtraction is too complexity
// to complex to compute, let's check whether it succeeded.
if (Optional<NonLoc> freeSpaceNL = freeSpace.getAs<NonLoc>())
```


================
Comment at: lib/StaticAnalyzer/Checkers/CStringChecker.cpp:1630
+
+            SVal haveEnoughSpace = svalBuilder.evalBinOpNN(
+                state, BO_LE, *strLengthNL, *freeSpaceNL, cmpTy);
----------------
`hasEnoughSpace`


================
Comment at: lib/StaticAnalyzer/Checkers/CStringChecker.cpp:1637-1640
+            if (TrueState && !FalseState) {
+              // srcStrLength <= size - dstStrLength -1
+              amountCopied = strLength;
+            }
----------------
All of these too, and omit braces too:

```lang=c++
// srcStrLength <= size - dstStrLength -1
if (TrueState && !FalseState)
  amountCopied = strLength;
```


================
Comment at: lib/StaticAnalyzer/Checkers/CStringChecker.cpp:1657
     if (lenValNL) {
-      if (isAppending) {
+      if (appendK == appendKind::strcat) {
         // For strncat, the check is strlen(dst) + lenVal < sizeof(dst)
----------------
Would prefer a switch case here too.


Repository:
  rC Clang

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D66049/new/

https://reviews.llvm.org/D66049