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

[Aaron Ballman]

On Sun, Jul 26, 2015 at 3:16 PM, Tobias Langner
<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]. 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 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
>
>