On Fri, Aug 17, 2018 at 07:51:26PM -0500, Scott Cheloha wrote: > On Fri, Aug 17, 2018 at 04:43:03PM -0500, Scott Cheloha wrote: > > On Mon, Aug 13, 2018 at 07:20:38PM +0200, Theo Buehler wrote: > > > On Mon, Aug 13, 2018 at 07:01:25PM +0200, Jeremie Courreges-Anglas wrote: > > > > On Mon, Aug 13 2018, Scott Cheloha <[email protected]> wrote: > > > > > tb@ spotted this one. > > > > > > > > > > If BIO_new fails we leak scon, so SSL_free it in that case. > > > > > > > > > > ok? > > > > > > > > Alternative proposal: let doConnection() and benchmark() free the > > > > resources they allocated respectively. > > > > > > > > > > I'd be ok with that as a workaround, but it doesn't change the fact that > > > doConnection() desperately needs a cleanup. > > > > > > As Joel pointed out off-list, it would probably be better to to move the > > > call to SSL_new() out of doConnection() and then simplify the latter. > > > > > > Cheloha is looking into that, and I think we can give this a bit of time. > > > > Here is such a diff. > > > > Move SSL_new() out into benchmark() and eliminate all SSL_new/SSL_free > > juggling from doConnection. Because doConnection is no longer allocating, > > have it return an integer. benchmark() is now less compact but it is > > clearer that we never leak an SSL object as all the allocations and frees > > occur within the same scope. > > > > The only behavior change is that we now always do SSL_set_connect_state > > for the connection instead of skipping that for the object's first use. > > After reading the manpage I'm pretty sure this is harmless. Can someone > > more familiar with libssl weigh in, though? > > > > The for-loop in doConnection remains peculiar, but that's cosmetic. > > Here is a cleaner version of the prior diff with input from tb. > > Thoughts? ok?
Looks good. ok
