[PATCH] D92777: [clang-tidy] Add bugprone-strlen-in-array-index checker

[Whisperity via Phabricator via cfe-commits]

whisperity added a comment.

The tests are a bit slim. We definitely need more cases... what if the array isn't allocated locally, but received as a parameter? I take that we need both an `fgets` and a `strlen` call in the same function(?) to emit diagnostics. Are we sure it will work if the array is originally from somewhere else?



================
Comment at: clang-tools-extra/clang-tidy/bugprone/StrlenInArrayIndexCheck.cpp:21
+  Finder->addMatcher(
+      arraySubscriptExpr(
+          allOf(hasBase(implicitCastExpr(hasSourceExpression(
----------------
While certainly not natural, if we're already here, it wouldn't take more than a few additional lines to match the equivalent but "expanded" version of array indexing:

```
*(buf + strlen(buf) - 1) = 0;
```


================
Comment at: clang-tools-extra/clang-tidy/bugprone/StrlenInArrayIndexCheck.cpp:22
+      arraySubscriptExpr(
+          allOf(hasBase(implicitCastExpr(hasSourceExpression(
+                    declRefExpr(to(varDecl().bind("buf")))))),
----------------
What's a hard `implicitCastExpr` doing here? Does a cast always happen? I feel this might be a bit too restrictive... there are matcher adaptors like `ignoringParenImpCasts`, that route should be investigated.


================
Comment at: clang-tools-extra/clang-tidy/bugprone/StrlenInArrayIndexCheck.cpp:26-28
+                        allOf(callee(functionDecl(
+                                  hasAnyName("::strlen", "::std::strlen",
+                                             "::wcslen", "::std::wcslen"))),
----------------
`strlen_s`, `wcslen_s`? If I am reading [[ http://en.cppreference.com/w/c/string/byte/strlen | this correctly ]] then in case the input buffer starts with a `NUL` (and the //safe size// is large enough) then it behaves the same way `strlen` and will still return 0.

If we are already here, it might be worth to also check for [[ http://pubs.opengroup.org/onlinepubs/9699919799/functions/strlen.html | strnlen ]]. It is POSIX and BSD specific, but behaves the same way as `strlen_s`.


================
Comment at: clang-tools-extra/clang-tidy/bugprone/StrlenInArrayIndexCheck.cpp:31-32
+                                                 equalsBoundNode("buf")))))))),
+                    anyOf(hasDescendant(binaryOperator(hasOperatorName("-"))),
+                          binaryOperator(hasOperatorName("-")))))))
+          .bind("arrayExpr"),
----------------
Why is the inner matcher duplicated here? What cases do we have a direct (?) `-` and a non-direct (descendant) one?


================
Comment at: clang-tools-extra/test/clang-tidy/checkers/bugprone-strlen-in-array-index.cpp:16-17
+
+// Examples were copied from the cert-fio37-c issue (with some added calls):
+// https://wiki.sei.cmu.edu/confluence/display/c/FIO37-C.+Do+not+assume+that+fgets%28%29+or+fgetws%28%29+returns+a+nonempty+string+when+successful
+
----------------
Such code examples might be covered by non-permissive copyright, which might be incompatible with LLVM's licence. I was unable to find anything wrt. copyright on SEI CERT's Confluence, which in this case makes the work to be //(strictly) copyrighted// by default. I am not sure what the actual situation is, but such a **direct mention** of copying //could// lead to problems.


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

https://reviews.llvm.org/D92777