Add a check to detect usages of realloc where the result is assigned
to the same variable (or field) as passed to the first argument.
Details
Diff Detail
- Repository
- rG LLVM Github Monorepo
Event Timeline
Consider adding a test case demonstrating if the checker recognizes and suppresses the check if there are multiple variables referring to the memory being reallocated.
If the check does not handle that case, we should probably state this limitation explicitly. And marking the corresponding test case with a FIXME.
I was thinking about something like this:
void escape(void *); void foo(void *p) { void *q = p; p = realloc(p, 10); // no-warning escape(q); } void bar(void *p) { p = realloc(p, 10); // warn: ... void *q = p; escape(q); }
The MallocOverflowCheck does something similar. It uses isBeforeInTranslationUnit().
clang-tools-extra/clang-tidy/bugprone/SuspiciousReallocUsageCheck.cpp | ||
---|---|---|
20–21 | Commented code? | |
23 | against |
I see that there are problems to solve with this patch. If a global variable is used at realloc or the pointer is a function parameter, it may have aliases that are not checkable. In the function code it is possible to detect aliasing. If all these cases are eliminated probably not much is left that is detected by the check, but we can test it.
We had a discussion about this with @dkrupp . We think that the p = realloc(p, var) construct in itself is an error-prone style of using realloc. This style does not necessarily give birth to erroneous code (see the error-free escaping example in foo above from @steakhal). However, the form p = realloc(p, var) is an indication that the programmer might missed the non-trivial error handling case, and chances are high. Thus, one should use the p2 = realloc(p1, var) form as a best practice. So, this looks good to me, but please consider this a weak approve and wait for someone else's approve who has more confidence in clang-tidy (@whisperity could you please take a look?).
I agree with @martong that $ = realloc($, ...) is indicative of something going wrong, even if the developer has saved the original value somewhere. Are we trying to catch this with Tidy specifically for this reason (instead of CSA's stdlib checker, or some taint-related mechanism?). However, @steakhal has some merit in saying that developers might prioritise the original over the copy, even if they made a copy. I think an easy solution from a documentation perspective is to have a test for this at the bottom of the test file. And document that we cannot (for one reason or the other) catch this supposed solution, but if you have made a copy already(!), then you might as well could have done void* q = realloc(p, ...) anyway! Having this documented leaves at least some paths open for us to fix false positives later, if they become a tremendous problem. (I have a gut feeling that they will not, that much.)
clang-tools-extra/clang-tidy/bugprone/SuspiciousReallocUsageCheck.cpp | ||
---|---|---|
20–21 | (Nit: using namespace clang -> The clang:: should be redundant here.) | |
87 | (We're creating a temporary here, right?) |
clang-tools-extra/clang-tidy/bugprone/SuspiciousReallocUsageCheck.cpp | ||
---|---|---|
89–90 | I have some reservations about this message, though. It only indirectly states the problem: first, the user needs to know what a memory leak is, and then needs to know how realloc could fail, and then make the mental connection about the overwriting of the pointer. (Also, you're passing a pointer... "memory block" is a more abstract term, requiring more knowledge.) I think for the sake of clarity, we should be more explicit about this.
1st line explains how the issue manifests (explicitly saying that you'll get a nullpointer in a bad place) (Getting the null pointer is the "only" bad path here.) |
clang-tools-extra/clang-tidy/bugprone/SuspiciousReallocUsageCheck.cpp | ||
---|---|---|
89–90 | Also: diag(...) << PtrInputExpr.getSourceRange() << PtrResultExpr.getSourceRange(); So the connection that the recipient of the assignment and the 1st parameter is the same is more obvious. |
LGTM, no objection here.
clang-tools-extra/test/clang-tidy/checkers/bugprone/suspicious-realloc-usage.cpp | ||
---|---|---|
82–86 | I believe we should also put a FIXME here. |
I added a simple detection of create a copy of p before p = realloc(p, ...). This can remove the warning at very obvious cases when a copy of p is created (but even if the copy is made inside an if branch for example).
Okay, I am comfortable with this update, even though the recognition of the assignment cannot be fully precise. So, still looks good to me.
Commented code?