This caught my eye because my primary project is Rational PurifyPlus, another bug-finding tool for C and C++ code.
I recommend reviewing these patches carefully. This bug-finding tool (www.emn.fr/x-info/coccinelle) appears to have limitations in its analysis. It seems to be reporting a false positive. In use_after_free.patch, Sune Rievers proposes removing the call to "OPENSSL_free(header)" from inside the loop. Presumably the bug-finding tool saw that "header" is used outside the loop and is freed later anyway. However, this is not really a use-after-free situation. The only way to get out of the loop and use "header" is via the "break" which occurs before the free. So there is actually no code path leading from the free to the outside of the loop where "header" can be used. The analysis tool appears to (falsely) see this as an ordinary loop that might fall through its closing brace. The proposed patch will actually leak one "header" object for each iteration where check_pem returns false. (File crypto/pem/pem_lib.c, function PEM_bytes_read_bio.) Looking at the other patches, I also saw this: in unused.patch, the first proposed patch removes the unused variable "to_return," but it uselessly leaves in an "if (gmp2bn(hptr->r0, r)" statement with an empty body. This "if" can be removed, leaving just the function call. As written (both before and after this proposed patch), the code doesn't check whether gmp2bn failed. Is that another bug? These are examples of a core problem with bug-finding tools: even when they're right (about variables that aren't used, in this case), it's not always obvious what the correct fix is. As I once wrote in an article, bug-finding tools can provide the clues but you, the developer, still have to solve the crime. -- Allan Pratt, [email protected] Rational software division of IBM ______________________________________________________________________ OpenSSL Project http://www.openssl.org Development Mailing List [email protected] Automated List Manager [email protected]
