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

[Aaron Ballman]

On Thu, Jul 30, 2015 at 4:15 PM, Tobias Langner
<randomcppprogrammer at gmail.com> wrote:
>
> 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> 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.

Correct, I think that entry is out of date (we've not had anyone audit
automated detection since the status of the rules is in so much flux).
I'm pretty sure 4031 is watching for catching by reference, which used
to be a part of the cited recommendation, but has since been split out
into a separate rule.

Sutter and Meyers do not make such a recommendation, but they also do
not focus on security, so their guidance is sometimes different than
you'll find in the security community (though there's certainly a ton
of overlap).

> 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).

This recommendation hasn't been updated recently, aside from removing
parts that were upgraded into a rule. The code examples are definitely
not compelling.

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

I think that this would make the most sense as an option.

The security implications for not throwing rvalues are considerably
less pressing than for throwing pointer values, this is why CERT
places it as a recommendation instead of as a normative rule required
for conformance. You can write correct code that throws lvalues, it's
just that the code is more likely to suffer from correctness or
maintenance issues that throwing an rvalue would entirely eliminate.

> 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.

It's not much different than catching any other builtin or trivial
type. Also, because the throw checker is ensuring you throw only
*literals*, you reduce the chances of that catch being used to catch a
throw that performed a dynamic allocation. There's still the
off-chance that the throw expression is in a different TU that the
checker doesn't have access to (such as a library), but that's
starting to get into the long tail, I think.

~Aaron

>
> 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
>
>