Kent, thanks for the very helpful review. See below for some comments.
On Tue, 15 Jun 2010 13:38:07 -0500 Kent Yoder <[email protected]> wrote: > > @@ -551,6 +559,10 @@ object_mgr_add_to_map( SESSION * sess, > > return CKR_FUNCTION_FAILED; > > } > > object_map = dlist_add_as_first( object_map, map_node ); > > While looking into this code I saw a problem not with your patch, > but in the dlist add routines. If malloc() inside > dlist_add_as_first() fails, the global object_map variable is blown > away by getting set to NULL. Also no error is logged in that case. :-) > Yikes. dlist_add_as_last() does the same. Ping me if you want me to > open a bug for this... Good catch. I'll take a look, no need for BZs at this time I think. > > - node = node->next; > > + // restore the old signal handler > > + if (sigaction(SIGSEGV, &oldact, NULL) != 0) { > > + // Even if we *have* the right handle & could recover, > > failing to > > + // restore the handler might indicate problems. > > + st_err_log(4, __FILE__, __LINE__, __FUNCTION__); > > + result = CKR_FUNCTION_FAILED; > > + goto out; > > + } > > + } > > + else { > > In this path, doesn't oldact stay registered as we exit? Shouldn't > the if block above be moved down to be common to both the segfault > case and the non-segfault case? I am new at reading the flow of these > signal catchers. :-) The above is true. In fact, giving it a little more thinking, I couldn't figure out a way of properly registering this 'catcher' and returning to the exact same environment as we had before sigsetjmp() in a consistent, thread-and-signal-safe way. And after discussing a bit with Peter, I was thinking in removing the "recover from invalid handles" code altogether. This essentially means that the app would crash for most of the times it uses an invalid reference for either an object or session. Apps failing to get those handle right are probably buggy to start with, maybe trying to recover would help even masking the root problem longer. And without sigsetjmp/sigaction/siglongjmp witchery we have a better chance of this working together with apache+mod_nss, gskit and others ;-) -Klaus -- Klaus Heinrich Kiwi | [email protected] | http://blog.klauskiwi.com Open Source Security blog : http://www.ratliff.net/blog IBM Linux Technology Center : http://www.ibm.com/linux/ltc ------------------------------------------------------------------------------ ThinkGeek and WIRED's GeekDad team up for the Ultimate GeekDad Father's Day Giveaway. ONE MASSIVE PRIZE to the lucky parental unit. See the prize list and enter to win: http://p.sf.net/sfu/thinkgeek-promo _______________________________________________ Opencryptoki-tech mailing list [email protected] https://lists.sourceforge.net/lists/listinfo/opencryptoki-tech
