[PATCH] D39121: [clang-tidy] Misplaced Operator in Strlen in Alloc

Balogh, Ádám via Phabricator via cfe-commits cfe-commits at lists.llvm.org
Wed Oct 25 02:18:27 PDT 2017


baloghadamsoftware marked 5 inline comments as done.
baloghadamsoftware added inline comments.


================
Comment at: clang-tidy/misc/MisplacedOperatorInStrlenInAllocCheck.cpp:30
+  Finder->addMatcher(
+      callExpr(callee(functionDecl(hasName("malloc"))),
+               hasArgument(0, anyOf(hasDescendant(BadUse), BadUse)))
----------------
xazax.hun wrote:
> Maybe it is worth to have a configurable list of allocation functions?
> 
> Maybe it would be worth to support`alloca` as well?
Support for ``alloca`` added.

I agree, it is worth to have a configurable list, but I think it is better to be done in a separate patch.


================
Comment at: clang-tidy/misc/MisplacedOperatorInStrlenInAllocCheck.cpp:44
+    const MatchFinder::MatchResult &Result) {
+  // FIXME: Add callback implementation.
+  const auto *Alloc = Result.Nodes.getNodeAs<CallExpr>("Alloc");
----------------
xazax.hun wrote:
> What is this fixme?
Sorry, I forgot to remove it.


================
Comment at: clang-tidy/misc/MisplacedOperatorInStrlenInAllocCheck.h:19
+
+/// FIXME: Write a short description.
+///
----------------
xazax.hun wrote:
> There is a missing description.
No longer. Sorry, I forgot it.


================
Comment at: docs/clang-tidy/checks/list.rst:10
    android-cloexec-creat
+   android-cloexec-dup
    android-cloexec-epoll-create
----------------
Eugene.Zelenko wrote:
> I think will be better to fix order of other checks separately from this check.
I agree, but I did not do it. It was the script that created the new check. I changed it back to the wrong order.


================
Comment at: docs/clang-tidy/checks/misc-misplaced-operator-in-strlen-in-alloc.rst:16
+    void bad_malloc(char *str) {
+      char *c = (char*) malloc(strlen(str + 1));
+    }
----------------
aaron.ballman wrote:
> xazax.hun wrote:
> > What if this code is intentional for some reason?
> > I think in thase case it could be rewritten like the following (which is much cleaner):
> > ```
> > char *c = (char*) malloc(strlen(str)-1);
> > ```
> > I think it might be a good idea to mention this possibility as a way to suppress this warning in the documentation. 
> This is my concern as well -- I'd like to know how chatty this diagnostic is on real-world code bases, especially ones that rely on C rather than C++. A somewhat common code pattern in Win32 coding are double-null-terminated lists of strings, where you have null terminated strings at adjacent memory locations with two null characters for the end of the list. This could result in reasonable code like `malloc(strlen(str + offset) + 1)`.
It is now more conservative: only ``+ 1`` counts, and only if there is no additional ``+ 1`` outside.


https://reviews.llvm.org/D39121





More information about the cfe-commits mailing list