Page MenuHomePhabricator

[clang-tidy] Add checker 'bugprone-suspicious-realloc-usage'.
ClosedPublic

Authored by balazske on Sep 1 2022, 8:44 AM.

Details

Summary

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.

Diff Detail

Event Timeline

balazske created this revision.Sep 1 2022, 8:44 AM
Herald added a project: Restricted Project. · View Herald TranscriptSep 1 2022, 8:44 AM
balazske requested review of this revision.Sep 1 2022, 8:44 AM
Herald added a project: Restricted Project. · View Herald TranscriptSep 1 2022, 8:44 AM
Herald added a subscriber: cfe-commits. · View Herald Transcript

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
21–22

Commented code?

24

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.

martong accepted this revision.Sep 26 2022, 7:41 AM
martong added a reviewer: whisperity.
martong added a subscriber: whisperity.

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?).

This revision is now accepted and ready to land.Sep 26 2022, 7:41 AM

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
21–22

(Nit: using namespace clang -> The clang:: should be redundant here.)

88

(We're creating a temporary here, right?)

whisperity added inline comments.Sep 27 2022, 4:38 AM
clang-tools-extra/clang-tidy/bugprone/SuspiciousReallocUsageCheck.cpp
90–91

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.

taking the returned pointer from 'realloc' overwrites 'XXXPtr' with null
if allocation fails,
which may result in a leak of the original buffer

1st line explains how the issue manifests (explicitly saying that you'll get a nullpointer in a bad place)
2nd line gives the condition, which is a bit more explicit
3rd line explains the real underlying issue

(Getting the null pointer is the "only" bad path here.)

whisperity added inline comments.Sep 27 2022, 4:39 AM
clang-tools-extra/clang-tidy/bugprone/SuspiciousReallocUsageCheck.cpp
90–91

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.

balazske updated this revision to Diff 463575.Sep 28 2022, 8:43 AM
balazske marked 6 inline comments as done.

Implemented review comments, changed warning text, changed documentation.

balazske updated this revision to Diff 463776.Sep 29 2022, 12:58 AM

added test, added a comment

balazske updated this revision to Diff 463812.Sep 29 2022, 2:49 AM

rebase to current main

steakhal accepted this revision.Sep 29 2022, 8:51 AM

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.

balazske updated this revision to Diff 464291.Sep 30 2022, 8:34 AM

Added simple check for create of copy of pointer before the realloc call.

balazske marked an inline comment as done.Sep 30 2022, 8:55 AM

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).

martong accepted this revision.Oct 3 2022, 2:26 AM

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.