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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
