[PATCH] D71001: [clang-tidy] New check: bugprone-misplaced-pointer-arithmetic-in-alloc

[Whisperity via Phabricator via cfe-commits]

whisperity added a comment.

While I can't pitch in with actual findings of this check, @MyDeveloperDay, you're right in many aspects, including those specific examples //not// firing. But an example that actually fires this check indicates a very specific **undefined behaviour** case. So if such bogus code (that would trigger on this check) did not cause run-time issues so far, it's because they never `free()` their memory allocations (really bad?), or their platform is particular enough that it allows calling `free` into the buffer, not only for the **start** of the buffer.

You must (only at most once) free each and every pointer as they had returned from a function of the `*alloc` family. `free(malloc(X))` is okay, but `free(malloc(X) + b)` is, as per the rules of the language, clearly undefined behaviour. Which of course may just as well include the OS/runtime going "Oh the stupid `free`d into this buffer, let me find the beginning of the allocation..." until one day it doesn't anymore.

(Snarky "self"-advertisement: Did you check CodeChecker to be that bug database? 😜 )



================
Comment at: clang-tools-extra/clang-tidy/bugprone/MisplacedPointerArithmeticInAllocCheck.cpp:23
+      anyOf(hasName("::malloc"), hasName("std::malloc"), hasName("::alloca"),
+            hasName("std::alloca"), hasName("::calloc"), hasName("std::calloc"),
+            hasName("::realloc"), hasName("std::realloc")));
----------------
`alloca` is a linux/posix-specific thing, and as such, I don't think it //could// be part of the `std` namespace.


================
Comment at: clang-tools-extra/clang-tidy/bugprone/MisplacedPointerArithmeticInAllocCheck.cpp:81-82
+  diag(PtrArith->getBeginLoc(),
+       "pointer arithmetic is applied to the result of %0() instead of its "
+       "argument")
+      << Func->getName() << Hint;
----------------
If I put the `+ b` on `X` as in `malloc(X + b)` instead of `malloc(X) + b`, then it's not //pointer// arithmetic anymore, but (hopefully unsigned) arithmetic. Should the warning message really start with "pointer arithmetic"?

Maybe you could consider the check saying

    arithmetic operation applied to pointer result of ...() instead of size-like argument

optionally, I'd clarify it further by putting at the end:

    resulting in ignoring a prefix of the buffer.

considering you specifically match on the std(-like) allocations. (At least for now.)


Repository:
  rCTE Clang Tools Extra

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

https://reviews.llvm.org/D71001