On Thu, Mar 5, 2009 at 6:41 PM, David Schwartz <dav...@webmaster.com> wrote: >> --- crypto\pkcs12\p12_crt.c Wed Mar 4 13:37:26 2009 >>+++ crypto\pkcs12\p12_crt.c Wed Mar 4 12:44:40 2009 >>@@ -168,7 +168,8 @@ PKCS12 *PKCS12_create(char *pass, char * >> sk_PKCS12_SAFEBAG_pop_free(bags, PKCS12_SAFEBAG_free); >> bags = NULL; >> >>- p12 = PKCS12_add_safes(safes, 0); >>+ if (!(p12 = PKCS12_add_safes(safes, 0))) >>+ goto err; >> >> sk_PKCS7_pop_free(safes, PKCS7_free); > > Won't this leak 'safes'? Don't you need to do it like this: > > p12 = PKCS12_add_safes(safes, 0); > sk_PKSC7_pop_free(safes, PKCS7_free); > if(p12==NULL) goto err; >
The patch cuts off a lot of context (while gmail adds to the character encoding :-). I found a link to the full file here: http://svn.python.org/projects/external/openssl-0.9.8g/crypto/pkcs12/p12_crt.c In short, 'goto err' leads to a cleanup section: err: if (p12) PKCS12_free(p12); if (safes) sk_PKCS7_pop_free(safes, PKCS7_free); if (bags) sk_PKCS12_SAFEBAG_pop_free(bags, PKCS12_SAFEBAG_free); return NULL; } > Oh, one more thing. This is a very common type of error. It's very hard to > test all possible out-of-memory paths. Worse, leaks in the error paths is > common (your submitted fix even had one) making it hard to recover from an > out-of-memory condition. Agree, agree, agree, disagree. > If you are trying to code reliable applications, you should not let > your primary memory allocator return NULL. If you are running low on memory, > begin load shedding and take other techniques to reduce load and memory > usage. If those techniques fail, then your application has failed. Actually, I believe the best way to report most any failure is with a C++ exception. This is the first time we've seen an out-of-memory condition in >1.5 years of 24/7 production. Interestingly, it wasn't due to natural load, but a leak we had that was only noticeable in a bad external condition not seen in testing. > It's just dangerous to hope that all your code, and the code you > calls, correctly handles all the places where an allocator might return NULL. There's this great technique in C++ called "RAII" which, if used pervasively, cleans things up just as reliably in the exceptional case as in the normal execution case. Memory allocation never returns null. (Unfortunately, we had a bit of C code in the mix which didn't use this technique, hence the leak). > Using allocators that never return NULL (blocking until memory can be made > available, dropping caches, and so on) is much, MUCH more robust. What if the best solution is to abort the operation requesting the big chunk of unavailable memory? We don't have any significant cache in this process to dump, and it wouldn't have helped for long anyway. Ironically, the best results came from getting logs and a crash dump (the first for this codebase) back to us developers. If it hadn't been for that null pointer dereference (which the patch fixes) the leak condition may have laid dormant a bit longer. > Not that known bugs shouldn't be fixed, of course. For sure. - Marsh ______________________________________________________________________ OpenSSL Project http://www.openssl.org Development Mailing List openssl-dev@openssl.org Automated List Manager majord...@openssl.org
