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?
Index: s_time.c
===================================================================
RCS file: /cvs/src/usr.bin/openssl/s_time.c,v
retrieving revision 1.26
diff -u -p -r1.26 s_time.c
--- s_time.c 14 Aug 2018 15:25:04 -0000 1.26
+++ s_time.c 18 Aug 2018 00:51:14 -0000
@@ -90,7 +90,7 @@
extern int verify_depth;
static void s_time_usage(void);
-static SSL *doConnection(SSL * scon);
+static int doConnection(SSL *);
static int benchmark(int);
static SSL_CTX *tm_ctx = NULL;
@@ -345,42 +345,28 @@ s_time_main(int argc, char **argv)
/***********************************************************************
* doConnection - make a connection
* Args:
- * scon = earlier ssl connection for session id, or NULL
+ * scon = earlier ssl connection for session id
* Returns:
- * SSL * = the connection pointer.
+ * 1 on success, 0 on error
*/
-static SSL *
-doConnection(SSL * scon)
+static int
+doConnection(SSL *scon)
{
struct pollfd pfd[1];
- SSL *serverCon;
BIO *conn;
long verify_error;
int i;
if ((conn = BIO_new(BIO_s_connect())) == NULL)
- return (NULL);
-
-/* BIO_set_conn_port(conn,port);*/
+ return 0;
BIO_set_conn_hostname(conn, s_time_config.host);
-
- if (scon == NULL)
- serverCon = SSL_new(tm_ctx);
- else {
- serverCon = scon;
- SSL_set_connect_state(serverCon);
- }
-
- SSL_set_bio(serverCon, conn, conn);
-
- /* ok, lets connect */
+ SSL_set_connect_state(scon);
+ SSL_set_bio(scon, conn, conn);
for (;;) {
- i = SSL_connect(serverCon);
+ i = SSL_connect(scon);
if (BIO_sock_should_retry(i)) {
BIO_printf(bio_err, "DELAY\n");
-
- i = SSL_get_fd(serverCon);
- pfd[0].fd = i;
+ pfd[0].fd = SSL_get_fd(scon);
pfd[0].events = POLLIN;
poll(pfd, 1, -1);
continue;
@@ -389,17 +375,15 @@ doConnection(SSL * scon)
}
if (i <= 0) {
BIO_printf(bio_err, "ERROR\n");
- verify_error = SSL_get_verify_result(serverCon);
+ verify_error = SSL_get_verify_result(scon);
if (verify_error != X509_V_OK)
BIO_printf(bio_err, "verify error:%s\n",
X509_verify_cert_error_string(verify_error));
else
ERR_print_errors(bio_err);
- if (scon == NULL)
- SSL_free(serverCon);
- return NULL;
+ return 0;
}
- return serverCon;
+ return 1;
}
static int
@@ -415,7 +399,9 @@ benchmark(int reuse_session)
if (reuse_session) {
/* Get an SSL object so we can reuse the session id */
- if ((scon = doConnection(NULL)) == NULL) {
+ if ((scon = SSL_new(tm_ctx)) == NULL)
+ goto end;
+ if (!doConnection(scon)) {
fprintf(stderr, "Unable to get connection\n");
goto end;
}
@@ -448,7 +434,11 @@ benchmark(int reuse_session)
for (;;) {
if (finishtime < time(NULL))
break;
- if ((scon = doConnection(reuse_session ? scon : NULL)) == NULL)
+ if (!reuse_session) {
+ if ((scon = SSL_new(tm_ctx)) == NULL)
+ goto end;
+ }
+ if (!doConnection(scon))
goto end;
if (s_time_config.www_path != NULL) {
