[PATCH] D11328: [clang-tidy] new "throw-by-value-catch-by-reference" check for clang-tidy

[Tobias Langner]

> On 25 Jul 2015, at 19:07, Aaron Ballman <aaron at aaronballman.com> wrote:
> 
> On Sat, Jul 25, 2015 at 4:33 AM, Tobias Langner
> <randomcppprogrammer at gmail.com <mailto:randomcppprogrammer at gmail.com>> wrote:
>> Hi,
>> 
>> I modified the code. During this, additional questions popped up that I’d like to resolve before I put up a new patch. Please check my comments below.
>> 
>>> On 20 Jul 2015, at 17:17, Aaron Ballman <aaron at aaronballman.com> wrote:
>>> 
>>> On Sat, Jul 18, 2015 at 11:44 AM, Tobias Langner
>>> <randomcppprogrammer at gmail.com> wrote:
>>>> randomcppprogrammer created this revision.
>>>> randomcppprogrammer added reviewers: klimek, cfe-commits, jbcoe.
>>>> 
>>>> This patch contains an implementation to check whether exceptions where thrown by value and caught by const reference. This is a guideline mentioned in different places, e.g. "C++ Coding Standards" by H. Sutter.
>>> 
>>> I happened to be working on this exact same problem while developing
>>> rules for CERT's C++ secure coding guidelines, using a very similar
>>> approach. Thank you for looking into this! If you'd like another
>>> coding standard to compare this to:
>>> https://www.securecoding.cert.org/confluence/display/cplusplus/ERR61-CPP.+Catch+exceptions+by+lvalue+reference
>>> (we have a recommendation for throwing rvalues as well).
>>> 
>>> A few things I noticed:
>>> 
>>> This is not testing whether you are throwing by value, but are instead
>>> testing whether you are throwing a pointer. Is this intentional?
>> 
>> Yes. It’s easier to test and since you can only throw by pointer or by value, it’s in my opinion equivalent.
> 
> You can throw things other than pointers and values. Consider:
> 
> void f(Foo &f) { throw f; } // Okay
> 
> That's not throwing a value or a pointer.

actually - I think that’s throwing a value since:
		   "First, copy-initializes <http://en.cppreference.com/w/cpp/language/copy_initialization> the exception object from expression (this may call the move constructor for rvalue expression, and the copy/move may be subject to copy elision <http://en.cppreference.com/w/cpp/language/copy_elision>), then transfers control to the  exception handler <http://en.cppreference.com/w/cpp/language/try_catch>with the matching type whose compound statement or member initializer list was most recently entered and not exited by this thread of execution.”

(from: http://en.cppreference.com/w/cpp/language/throw)

and the AST shows a CXXConstructExpr as subexpr of the CXXThrowExpr - so it’s throw by value.

> 
> I think the advice I would like to see is more generally to throw
> rvalues. You want to catch dubious code like:
> 
> void f(Foo param) {
>  Foo SomeLocal;
>  throw SomeLocal; // Not good

that’s actually ok since SomeLocal is copied and that copy may be elided anyway. This is “just” performance - not safety.

>  throw param; // Not good

same here (but I don’t think that a copy elision is possible here - but I’m also not deep into compiler)

> }
> 
> but allow code like:
> 
> Foo g();
> void f(Foo &obj) {
>  throw Foo(); // ok
>  throw g(); // ok

this is exactly what I’d like to avoid. If g() is a factory function that provides a pointer to the newly created object (not too uncommon if for example used with RAII objects), it’s the same as throwing “new Foo()” So it’s more like if the thrown type is not a pointer type - except for StringLiterals which are safe.

>  throw obj; // ok, even though it's not an rvalue, it's a common code pattern
> }
> 
>> 
>>> Also,
>>> not all thrown pointers are dangerous. For instance: throw "This is a
>>> cheap exception"; is very reasonable code, while throw new
>>> BadIdea(12); is not.
>> 
>> this can be handled by allowing StringLiterals to be thrown, but no other pointers.
> 
> Doesn't builtinType() already handle that?

up until now I checked for builtins only on catch locations.

> 
>> My problem with this is that I don’t know how to recognise
>> this at the catch statement without removing the check for pointers. The only idea that I have right now is to check whether the pointed to type is ‘const char’ or ‘const wchar_t’ or one of the new ones for unicode. But it feels a little bit brittle.
> 
> I think that seems like the valid approach, since some pointers are
> acceptable to catch on.
> 
>>> As for catching, catching by const lvalue reference is not always the
>>> correct recommendation. For instance, the catch block could be
>>> modifying the exception object and rethrowing it, at which point you
>>> cannot catch by const reference. Also, not all types really need to be
>>> caught by reference, such as builtin types (think int) or trivial
>>> types.
>> 
>> I added the check for builtin & trivial types - that was a good idea. Regarding catching as reference (opposed to const reference), I could think of 2 different options:
>> - require nolint - which I think is viable. It marks in source code that someone had thought about this potential problem.
>> - having an option for the test (something like enforce const reference).
>> Both have their benefits & drawbacks. If a project requires at most call sites that the exception is modified, the second one is better - but I don’t know how to test different option given the current test infrastructure. If modifying an exception is rare, I think the first option is better as it forces to comment this rare situation.
> 
> I think that modifying the object is less common, so there's something
> to be said for the nolint idea. However, I don't personally see a
> reason to restrict to catching by const reference as opposed to any
> lvalue reference (modulo trivial types, etc).
> 
>> I would not like to remove the test for const-ness on catch because it can be a real performance hog that some might not be aware about.
> 
> I cannot imagine a situation where catching by reference is less
> performant than catching by const reference, in practice. Do you have
> some example code that demonstrates a performance difference?
> 
>> Any advice which way to go?
> 
> I still think that there's no need to guide people towards catching
> const references as opposed to catching by lvalue reference (const or
> otherwise). That addresses the correctness and security concerns.
> Constness seems like an orthogonal checker that focuses on making your
> code const correct (which would also be very useful IMO). By
> separating those concerns, the user has the most flexibility with the
> least amount of annotations or gymnastics.

no - I don’t have performance code and your proposal makes sense. These are different concerns (performance vs. security) and should be handled in different checks. I’ll remove the check for constness.

Tobias
> 
> ~Aaron

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20150726/367370bf/attachment.html>