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

[Tobias Langner]

> On 27 Jul 2015, at 16:18, Aaron Ballman <aaron at aaronballman.com> wrote:
> 
> On Sun, Jul 26, 2015 at 3:16 PM, Tobias Langner
> <randomcppprogrammer at gmail.com <mailto:randomcppprogrammer at gmail.com>> wrote:
>> 
>> 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> 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 the exception object from expression (this may
>> call the move constructor for rvalue expression, and the copy/move may be
>> subject to copy elision), then transfers control to the  exception
>> handlerwith 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.
> 
> There's some confusion that I may be perpetuating by accident. *Any*
> throw will act as though it is by value because the exception object
> is itself an anonymous block of memory in which the throw expression
> argument is copied (and the copy can be elided). That doesn't play
> into the security aspect of what I would want to see this checker
> focusing on. "Throw by value" is talking about the type of the object
> passed in to the throw expression, not the type of the exception
> object itself (at least, that's how I've read the guidance).
> 
>> 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.
> 
> No, this is security, not performance, which is why the recommendation
> (sometimes) is to not only not throw pointers, but to throw anonymous
> temporaries (aka, rvalues) [Dewhurst 03, CERT].

This is the first time that I’ve seen this. As far as I know, it’s not mentioned in Meyers books, not mentioned in the C++ coding guidelines by Sutter. I’ll check what rule 4031 of PRQAC++ actually checks - but my assumption is that it does not check for r-values.

Anyway - I’ve seen from the example here https://www.securecoding.cert.org/confluence/display/cplusplus/ERR09-CPP.+Throw+anonymous+temporaries that someone could assume this. In my opinion the example is a little bit far fetched since it also assumes that for the exception object (which has value semantics - otherwise it would not copy), comparison works different than copying. It assumes that there is a observable difference between su and the thrown object (except of course the address).

My proposal would be making this either an option or an additional check.


> Throwing pointers has
> security implications because there's no clear ownership semantics for
> the pointer object. Throwing lvalues has some security implications
> because it is not immediately obvious (to many) that the exception
> object is not the same object as what was thrown. Pointers are by far
> the more important of the two to get right for security reasons.
> Perhaps it would make a reasonable option for the checker to also warn
> on throwing something that isn't an rvalue (which is an option I would
> say should be on by default).
> 
>> 
>> 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.
> 
> I agree that if g() returned a pointer we should warn on it, but in my
> example, g() returns an rvalue.
> 
>> 
>> 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.
> 
> FWIW, the throw matcher I was using for research was something like:
> 
> match throwExpr(unless(anyOf(hasDescendant(anyOf(declRefExpr(to(parmVarDecl())),
> declRefExpr(to(varDecl(isExceptionVar()))),
> constructExpr(hasDescendant(materializeTemporaryExpr())),
> expr(hasType(hasCanonicalType(builtinType()))))),
> unless(has(expr())))))
> 
> (it isn't pretty, but it also cut down dramatically on the number of
> false positives.)
> 
> ~Aaron
> 
>> 
>> 
>> 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 still don’t know whether this is a good idea (allowing catch (const char*) or catch (const wchar_t*), …), but I don’t know a better way to allow string literals to be thrown.

Tobias
>> 
>> 
>> 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/20150730/40302acc/attachment.html>