Page MenuHomePhabricator

[PATCH] New checker for mismatched operator new/operator delete definitions
ClosedPublic

Authored by aaron.ballman on Sep 22 2015, 2:18 PM.

Details

Reviewers
klimek
alexfh
Summary

When defining a free store function, it is almost invariably a bug to fail to declare the corresponding free store function. For instance, implementing operator new() but failing to implement operator delete(). This new checker catches instances where the free store functions are mismatched, while still allowing for common code patterns involving deleted functions or placement forms of functions. Specifically, this will not warn on implicit free store functions, placement forms, deleted functions (such as defining a placement new and a deleted operator delete(void*)), and private functions (for code that predates C++11).

This patch corresponds to: https://www.securecoding.cert.org/confluence/display/cplusplus/DCL54-CPP.+Overload+allocation+and+deallocation+functions+as+a+pair+in+the+same+scope

I ran this checker over Clang and LLVM and it did find two issues, one of which I have fixed. YAMLParser.h had a protected operator delete() with a trivial definition that I converted to be a deleted function in r248320. This was a borderline false positive -- the code was correct because the body of operator delete() was trivial. However, defining the function as deleted is cleaner since accidental deletion from within Node or one of its subclasses will now be diagnosed instead of silently accepted and doing nothing. The other is a true positive as best I can tell. The MDNode class in Metadata.h defines a placement new operator, a free store operator delete(), and two placement delete functions. The free store operator delete() has no matching free store operator new() (including when looking in super classes), but the implementation does not appear to trigger undefined behavior -- it just seems like a really confused design (which is mitigated by the fact that the free store delete is protected). Also, one of the placement delete functions is not required by std, as the comments suggest, and should be removed.

~Aaron

Diff Detail

Event Timeline

aaron.ballman retitled this revision from to [PATCH] New checker for mismatched operator new/operator delete definitions.
aaron.ballman updated this object.
aaron.ballman added reviewers: klimek, alexfh.
aaron.ballman added a subscriber: cfe-commits.
alexfh edited edge metadata.Sep 26 2015, 5:43 PM

Please add a documentation file.

clang-tidy/misc/NewDeleteOverloadsCheck.cpp
43

Please no else after return.

aaron.ballman edited edge metadata.

Added documentation, fixed else after return.

~Aaron

aaron.ballman marked an inline comment as done.Sep 28 2015, 8:15 AM
alexfh added inline comments.Sep 28 2015, 8:40 AM
clang-tidy/misc/NewDeleteOverloadsCheck.cpp
67

http://llvm.org/docs/CodingStandards.html#name-types-functions-variables-and-enumerators-properly

Function names [...] should be camel case, and start with a lower case letter (e.g. openFile() or isFoo()).

169

Please don't use "O", "l", "I" as variable names.

171

I just noticed that this will be an O(N^2) from all new/delete overloads in all classes in a TU. This should probably be not much usually, but I can imagine a corner-case, where this is going to be slooow. How about sharding these by the enclosing record declaration?

test/clang-tidy/misc-new-delete-overloads-sized-dealloc.cpp
7

nit: Let's include the check name in brackets to the check pattern once.

Thank you for the review!

clang-tidy/misc/NewDeleteOverloadsCheck.cpp
67

Ugh, I always get this backwards because of other parts of the code base that didn't follow the convention. I'll fix.

169

I thought this was the correct style for identifiers that do not require descriptive names (we use it *everywhere* in Clang)? I'm not opposed, but I am wondering if clang-tidy has different style guides?

171

Yes, the O(N^2) is unfortunate, sorry for not calling that out explicitly. I figured that N should be incredibly minimal, however (especially since we only care about *written* overloads that are not placement overloads). So realistically, the maximum that N can be here is 6: operator new(), operator new[](), operator delete(), operator delete[](), and sized operator delete()/operator delete[](). I figured that this wasn't worth complicating the code over since N is bounded.

But I suppose the worry is if you have these operators defined in a a lot of classes in the same TU? In that case, I suppose I could replace SmallVector<FunctionDecl *> Overloads with MapVector<CXXRecordDecl *, FunctionDecl *> Overloads?

test/clang-tidy/misc-new-delete-overloads-sized-dealloc.cpp
7

Can do!

alexfh added inline comments.Sep 28 2015, 9:20 AM
clang-tidy/misc/NewDeleteOverloadsCheck.cpp
169

I'm not opposed to single-character identifiers, as long as they don't use characters that are indistinguishable from some other characters in some fonts. E.g. I don't want ever to be confused about map[O] vs map[0] (same for "l", "I", and sometimes even "1").

171

But I suppose the worry is if you have these operators defined in a a lot of
classes in the same TU? In that case, I suppose I could replace
SmallVector<FunctionDecl *> Overloads with MapVector<CXXRecordDecl
*, FunctionDecl *> Overloads?

Yes, this is what I meant. Though I didn't know about MapVector<> before you told me ;)

aaron.ballman added inline comments.Sep 28 2015, 9:22 AM
clang-tidy/misc/NewDeleteOverloadsCheck.cpp
169

Ah, this makes perfect sense. Can do. :-)

171

Er, not MapVector, but perhaps a std::map<CXXRecordDecl *, std::vector<FunctionDecl *>>, I guess.

alexfh added inline comments.Sep 28 2015, 9:36 AM
clang-tidy/misc/NewDeleteOverloadsCheck.cpp
171

Might be a DenseMap<CXXRecordDecl *, SmallVector<FunctionDecl *, 6>> as well. Or std::map<..., SmallVector<,>>.

Addressing review comments.

aaron.ballman marked 4 inline comments as done.Sep 28 2015, 9:37 AM
alexfh accepted this revision.Sep 29 2015, 2:50 AM
alexfh edited edge metadata.

Looks good with a few comments.

Thanks for the new check! Do you think whether this could be a compiler warning some day?

clang-tidy/misc/NewDeleteOverloadsCheck.cpp
52

nit: const auto *FPT would be better here to avoid duplication of the type name.

clang-tidy/misc/NewDeleteOverloadsCheck.h
22

I wonder whether "class" here and below is actually needed. Did you try without it?

test/clang-tidy/misc-new-delete-overloads.cpp
3

I think, "unsigned long" should be used instead (and we might have to define it differently for different platforms).

This revision is now accepted and ready to land.Sep 29 2015, 2:50 AM
aaron.ballman marked 3 inline comments as done.Sep 29 2015, 6:14 AM

Looks good with a few comments.

Thanks for the new check! Do you think whether this could be a compiler warning some day?

I thought about implementing it as a frontend warning, but it seemed somewhat expensive for there (since it would have to be triggered on every TU completion). However, it might be reasonable to move it in there at some point.

Thank you for the reviews! I've commit in r248791.

~Aaron

test/clang-tidy/misc-new-delete-overloads.cpp
3

I get a type redefinition error then, so I removed the typedef declaration entirely and will watch the bots to make sure this isn't an artifact of being on an MSVC build.

aaron.ballman closed this revision.Sep 29 2015, 6:14 AM
aaron.ballman marked an inline comment as done.