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 <scottchel...@gmail.com> 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.
Thoughts?
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 17 Aug 2018 20:00:00 -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,31 @@ 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.
+ * 0 on success, nonzero on error
*/
-static SSL *
-doConnection(SSL * scon)
+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);
+ return 1;
-/* BIO_set_conn_port(conn,port);*/
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);
+ SSL_set_connect_state(scon);
+ SSL_set_bio(scon, conn, conn);
/* ok, lets connect */
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 +378,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 1;
}
- return serverCon;
+ return 0;
}
static int
@@ -415,7 +402,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 +437,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) {
