> On 27 Jul 2015, at 16:18, Aaron Ballman <[email protected]> wrote: > > On Sun, Jul 26, 2015 at 3:16 PM, Tobias Langner > <[email protected] <mailto:[email protected]>> wrote: >> >> On 25 Jul 2015, at 19:07, Aaron Ballman <[email protected]> wrote: >> >> On Sat, Jul 25, 2015 at 4:33 AM, Tobias Langner >> <[email protected]> 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 <[email protected]> wrote: >> >> On Sat, Jul 18, 2015 at 11:44 AM, Tobias Langner >> <[email protected]> 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
_______________________________________________ cfe-commits mailing list [email protected] http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
