Hi, > That said, I think the DigestUpdate and similar checks are unnecessary > and complicate the code more than they help. Those functions really > can't fail.
Hmm, which ones specifically? In particular for DigestUpdate I always wondered why that should fail, but adding a few error checks usually didn't hurt that much and spared me having to wade through OpenSSL code, so I never investigated it. What makes me wonder a bit is that the documentation states: | In OpenSSL 0.9.7 and later if digest contexts are not cleaned up after | use memory leaks will occur. Obviously, this doesn't specify what "cleaning up" actually means, or whether this applies to statically/stack allocated or EVP_MD_CTX_create created contexts or both, or anything really helpful at all, but it might suggest that there is some dynamic allocation happening somewhere in there, which in turn could be a reason for failure? All those checks certainly don't help with clarity, and any avoidable need for error checks certainly would help with the robustness of the API, so I am all for throwing out unnecessary checks, I'd just like to be reasonably sure they are actually unnecessary ;-) > However, I think maybe the free() and BN_clear_free() fixes are still > applicable? The free() is just moved down to ensure it happens even in case one of the new error checks causes an early return, so it's not needed if those aren't needed. The BN_clear_free() fix is independent from that, yeah. Regards, Florian
