This is an archive of the discontinued LLVM Phabricator instance.

[clang-tidy] Add check bugprone-multiple-new-in-one-expression.
ClosedPublic

Authored by balazske on Nov 28 2022, 1:05 AM.

Diff Detail

Event Timeline

balazske created this revision.Nov 28 2022, 1:05 AM
Herald added a project: Restricted Project. · View Herald Transcript
balazske requested review of this revision.Nov 28 2022, 1:05 AM
Herald added a project: Restricted Project. · View Herald TranscriptNov 28 2022, 1:05 AM
Herald added a subscriber: cfe-commits. · View Herald Transcript

Some of the code is copied from UnhandledExceptionAtNewCheck. Probably these checks can be merged into one NewExceptionHandlingCheck?

Thanks for this new check. Could you please link here results of this checker on som relevant open source projects?

Eugene.Zelenko added inline comments.
clang-tools-extra/clang-tidy/bugprone/MultipleNewInOneExpressionCheck.h
25

I think check is applicable to early versions too.

clang-tools-extra/docs/clang-tidy/checks/bugprone/multiple-new-in-one-expression.rst
9

Please follow 80 characters limit.

bitcoin, contour, qtbase, xerces was checked but nothing was detected by the check.

I'm really not sure this is the right approach to solve the problem. If the concern here is leaking on exceptions, then the goalpost should be flagging all calls to global new and recommend a smart pointer factory instead. I think that check could even be made as a cppcoreguideline check

CppCoreGuidelines rule r13-perform-at-most-one-explicit-resource-allocation-in-a-single-expression-statement is applicable for this problem. The current check is more restricted. If the check is moved to cppcoreguidelines it should check for any multiple new in a single expression and probably for other "resource allocation" that is not new (if there is such case at all that can not be changed to a safe function call or is not deprecated by other rules). Probably we can have an option to switch the behavior (any 2 new or the current way) and move the check to cppcoreguidelines.

This check is not about removing any use of new. It is only about the memory leak problem that is easy to detect and mentioned in the CppCoreGuidelines and CERT MEM52-CPP rules. If it is moved to the cppcoreguidelines module I would add an option that makes the check work like it works in this patch now, if it is turned off the check can find any expression where multiple new occurs (even when || and similar operators are used). My question is would the reviewers accept the check in the current form or should it be moved to cppcoreguidelines?

donat.nagy added inline comments.Feb 14 2023, 6:23 AM
clang-tools-extra/clang-tidy/bugprone/MultipleNewInOneExpressionCheck.cpp
44–45

In general, a value can be stored by passing it to a CompoundAssignOperator. This is not relevant in the current application (pointers do not appear on the RHS of compound assignment operators); but perhaps it's worth to mention this in a short comment in case someone wishes to reuse this function in some other checker.

clang-tools-extra/docs/clang-tidy/checks/bugprone/multiple-new-in-one-expression.rst
9–14

I think the handling of the case of the short-circuiting operators is ambiguous in this long paragraph: without the examples it's unclear whether the checker reports e.g. f2(new A) || f1(new B); as an error ("Even if the order is fixed ... It is best to avoid any expression that contains more than one operator `new` call ... " suggests that even these cases are reported). Try to restructure this description, perhaps by splitting it into two paragraphs: (1) quote/state all the relevant rules of C++ evaluation order (2) describe how "bad" ordering of the two allocation causes a memory leak.

balazske updated this revision to Diff 497654.Feb 15 2023, 5:58 AM

make check available in all C++ versions, add compound operator related test and code

LegalizeAdulthood resigned from this revision.Mar 29 2023, 8:07 AM
balazske updated this revision to Diff 512425.Apr 11 2023, 6:25 AM

Fixed documentation issues.
Check is added to list.rst.

balazske marked 2 inline comments as done.Apr 11 2023, 6:26 AM
PiotrZSL added inline comments.Apr 11 2023, 7:04 AM
clang-tools-extra/clang-tidy/bugprone/MultipleNewInOneExpressionCheck.cpp
92

what about: `catch(...)`

103

this doesnt look valid, arg2 isn't known at this point yet, so this could be removed.
and this may not work for case like this:

callSomething({new A,  new B});

Most probably this equalsBoundNode should be on HasNewExpr2 level, to avoid duplicated newExpr, not arguments of call

and image situation like this:

try {
  something(std::shared_ptr<int>(new int), std::shared_ptr<int>(new int));
} catch(const std::bad_alloc&) {}

this in theory could also produce false-positive.

other issue is that first call could be anything, maloc, new, calloc, wzalloc, operator new(), it doesn't need to be just new.
You could try to use some simple way of checking this like, isBeforeInTransationUnit....

And this will also produce false-positive if we use new(std::nothrow) on second.
There are some "utils" to check sequence order, maybe would be good to investigate them.

106

To be honest, I don't see any reason, how this try-catch would change anything, there can be one in parent function, and we going to heave a leak if we have try-catch or not.

126

other issue I see is that same new could be matched multiple times by those Matchers

balazske marked an inline comment as done.Apr 11 2023, 9:07 AM
balazske added inline comments.
clang-tools-extra/clang-tidy/bugprone/MultipleNewInOneExpressionCheck.cpp
92

hasHandlerFor checks for it (and it is used in the test).

103

I am not an expert in how AST matchers work exactly, but this code works with the provided tests and the results look correct. I did not experience that two new in the same argument is matched, this is why the unless(equalsBoundNode(...)) is added. The "arg1" and "arg2" nodes are the direct expressions in the function call, not descendants, and a new in the same argument (any descendant) is not matched twice in this way. Otherwise this would show up in failed tests.

The code something(std::shared_ptr<int>(new int), std::shared_ptr<int>(new int)) should produce warning (it may be possible that result of the first new int is not passed to shared_ptr before the other new int is called that fails, good solution is use of std::make_shared in such case). The test code (void)f(g(new A), new B); is somewhat similar AST, the new A should be found in all cases because hasDescendant is used at HasNewExpr1 and 2.

Probably unless(equalsBoundNode("arg2")) can be removed, it is enough to have one of these checks.

InitListExpr is not handled by the current code, I need to add this case (it has fixed evaluation order).

103

The check only works with memory allocation failures that throw exceptions, but really only with new, this is the most often used. Functions like malloc do not throw exceptions.

106

The problem is that it is difficult to check if the parent function has a try-catch block, and the function can be called in different ways. This can results in false negatives. But without any try-catch block the program may not use exception handling at all (a failed new terminates the program), and it can be false positive to emit any warnings.

126

This may be possible but I think that if the same warning is found multiple times it is shown only once, otherwise every case found by the matchers is a valid warning case even if the same new is involved.

balazske updated this revision to Diff 512808.Apr 12 2023, 6:27 AM

Simplified AST matchers a bit.
Added check for list-initialization syntax at constructor call.
Added some tests.
Updated documentation.

balazske marked 2 inline comments as done.Apr 12 2023, 6:31 AM
balazske added inline comments.
clang-tools-extra/clang-tidy/bugprone/MultipleNewInOneExpressionCheck.cpp
44–45

It is mentioned now in the comment before the function.

donat.nagy accepted this revision.Apr 18 2023, 1:12 PM
This revision is now accepted and ready to land.Apr 18 2023, 1:12 PM
This revision was landed with ongoing or failed builds.May 2 2023, 2:00 AM
This revision was automatically updated to reflect the committed changes.
balazske marked an inline comment as done.