From ce681752c44f792feab122cbd846b2407a42da72 Mon Sep 17 00:00:00 2001 From: Garrett D'Amore Date: Tue, 2 Jan 2018 13:20:53 -0800 Subject: fixes #191 Several HTTP problems found First, httpbin.org was having some high latency (load) earlier today, so we needed to bump the timeout up. Next, this also uncovered a bug where our cancellation of http channels was a bit dodgy. This is changed to be a bit more robust, separating the "current" active http streams (for read or write) into separate tracking variables variables. Also, now cancellation immediately calls the aio finish for those -- there were assumptions elsewhere (expire timeouts) that cancellation caused nni_aio_finish() to be called. Finally there was a use after free bug in the websocket listener code where the listener could be freed while still having outstanding streams waiting to send the websocket reply. --- src/supplemental/http/http.c | 105 ++++++++++++++++++++------------- src/supplemental/websocket/websocket.c | 16 ++++- tests/CMakeLists.txt | 6 +- tests/httpclient.c | 8 +-- 4 files changed, 85 insertions(+), 50 deletions(-) diff --git a/src/supplemental/http/http.c b/src/supplemental/http/http.c index 723b3e55..229a4a99 100644 --- a/src/supplemental/http/http.c +++ b/src/supplemental/http/http.c @@ -1,6 +1,6 @@ // -// Copyright 2017 Staysail Systems, Inc. -// Copyright 2017 Capitar IT Group BV +// Copyright 2018 Staysail Systems, Inc. +// Copyright 2018 Capitar IT Group BV // // This software is supplied under the terms of the MIT License, a // copy of which should be located in the distribution where this @@ -63,8 +63,10 @@ struct nni_http { nni_list rdq; // high level http read requests nni_list wrq; // high level http write requests - nni_aio *rd_aio; // bottom half read operations - nni_aio *wr_aio; // bottom half write operations + nni_aio *rd_uaio; // user aio for read + nni_aio *wr_uaio; // user aio for write + nni_aio *rd_aio; // bottom half read operations + nni_aio *wr_aio; // bottom half write operations nni_mtx mtx; @@ -214,10 +216,18 @@ http_rd_buf(nni_http *http, nni_aio *aio) static void http_rd_start(nni_http *http) { - nni_aio *aio; - - while ((aio = nni_list_first(&http->rdq)) != NULL) { - int rv; + for (;;) { + nni_aio *aio; + int rv; + + if ((aio = http->rd_uaio) == NULL) { + if ((aio = nni_list_first(&http->rdq)) == NULL) { + // No more stuff waiting for read. + return; + } + nni_list_remove(&http->rdq, aio); + http->rd_uaio = aio; + } if (http->closed) { rv = NNG_ECLOSED; @@ -228,11 +238,11 @@ http_rd_start(nni_http *http) case NNG_EAGAIN: return; case 0: - nni_aio_list_remove(aio); + http->rd_uaio = NULL; nni_aio_finish(aio, 0, aio->a_count); break; default: - nni_aio_list_remove(aio); + http->rd_uaio = NULL; nni_aio_finish_error(aio, rv); http_close(http); break; @@ -252,8 +262,8 @@ http_rd_cb(void *arg) nni_mtx_lock(&http->mtx); if ((rv = nni_aio_result(aio)) != 0) { - if ((uaio = nni_list_first(&http->rdq)) != NULL) { - nni_aio_list_remove(uaio); + if ((uaio = http->rd_uaio) != NULL) { + http->rd_uaio = NULL; nni_aio_finish_error(uaio, rv); } http_close(http); @@ -276,8 +286,12 @@ http_rd_cb(void *arg) // be no data left in the user buffer. NNI_ASSERT(http->rd_get == http->rd_put); - uaio = nni_list_first(&http->rdq); - NNI_ASSERT(uaio != NULL); + if ((uaio = http->rd_uaio) == NULL) { + // This indicates that a read request was canceled. This + // can occur only when shutting down, really. + nni_mtx_unlock(&http->mtx); + return; + } for (int i = 0; (uaio->a_niov != 0) && (cnt != 0); i++) { // Pull up data from the buffer if possible. @@ -312,13 +326,13 @@ http_rd_cancel(nni_aio *aio, int rv) nni_http *http = aio->a_prov_data; nni_mtx_lock(&http->mtx); - if (nni_aio_list_active(aio)) { - if (aio == nni_list_first(&http->rdq)) { - nni_aio_cancel(http->rd_aio, NNG_ECANCELED); - } else { - nni_aio_list_remove(aio); - nni_aio_finish_error(aio, rv); - } + if (aio == http->rd_uaio) { + http->rd_uaio = NULL; + nni_aio_cancel(http->rd_aio, rv); + nni_aio_finish_error(aio, rv); + } else if (nni_aio_list_active(aio)) { + nni_aio_list_remove(aio); + nni_aio_finish_error(aio, rv); } nni_mtx_unlock(&http->mtx); } @@ -334,7 +348,7 @@ http_rd_submit(nni_http *http, nni_aio *aio) return; } nni_list_append(&http->rdq, aio); - if (nni_list_first(&http->rdq) == aio) { + if (http->rd_uaio == NULL) { http_rd_start(http); } } @@ -344,14 +358,20 @@ http_wr_start(nni_http *http) { nni_aio *aio; - if ((aio = nni_list_first(&http->wrq)) != NULL) { - - for (int i = 0; i < aio->a_niov; i++) { - http->wr_aio->a_iov[i] = aio->a_iov[i]; + if ((aio = http->wr_uaio) == NULL) { + if ((aio = nni_list_first(&http->wrq)) == NULL) { + // No more stuff waiting for read. + return; } - http->wr_aio->a_niov = aio->a_niov; - http->wr(http->sock, http->wr_aio); + nni_list_remove(&http->wrq, aio); + http->wr_uaio = aio; + } + + for (int i = 0; i < aio->a_niov; i++) { + http->wr_aio->a_iov[i] = aio->a_iov[i]; } + http->wr_aio->a_niov = aio->a_niov; + http->wr(http->sock, http->wr_aio); } static void @@ -365,12 +385,12 @@ http_wr_cb(void *arg) nni_mtx_lock(&http->mtx); - uaio = nni_list_first(&http->wrq); + uaio = http->wr_uaio; if ((rv = nni_aio_result(aio)) != 0) { // We failed to complete the aio. if (uaio != NULL) { - nni_aio_list_remove(uaio); + http->wr_uaio = NULL; nni_aio_finish_error(uaio, rv); } http_close(http); @@ -379,7 +399,9 @@ http_wr_cb(void *arg) } if (uaio == NULL) { - // write canceled? + // Write canceled? This happens pretty much only during + // shutdown/close, so we don't want to resume writing. + // The stream is probably corrupted at this point anyway. nni_mtx_unlock(&http->mtx); return; } @@ -406,14 +428,15 @@ http_wr_cb(void *arg) aio->a_niov--; } if ((aio->a_niov != 0) && (aio->a_iov[0].iov_len != 0)) { - // We have more to transmit. + // We have more to transmit - start another and leave + // (we will get called again when it is done). http->wr(http->sock, aio); nni_mtx_unlock(&http->mtx); return; } done: - nni_aio_list_remove(uaio); + http->wr_uaio = NULL; nni_aio_finish(uaio, 0, uaio->a_count); // Start next write if another is ready. @@ -428,13 +451,13 @@ http_wr_cancel(nni_aio *aio, int rv) nni_http *http = aio->a_prov_data; nni_mtx_lock(&http->mtx); - if (nni_aio_list_active(aio)) { - if (aio == nni_list_first(&http->wrq)) { - nni_aio_cancel(http->wr_aio, NNG_ECANCELED); - } else { - nni_aio_list_remove(aio); - nni_aio_finish_error(aio, rv); - } + if (aio == http->wr_uaio) { + http->wr_uaio = NULL; + nni_aio_cancel(http->wr_aio, rv); + nni_aio_finish_error(aio, rv); + } else if (nni_aio_list_active(aio)) { + nni_aio_list_remove(aio); + nni_aio_finish_error(aio, rv); } nni_mtx_unlock(&http->mtx); } @@ -450,7 +473,7 @@ http_wr_submit(nni_http *http, nni_aio *aio) return; } nni_list_append(&http->wrq, aio); - if (nni_list_first(&http->wrq) == aio) { + if (http->wr_uaio == NULL) { http_wr_start(http); } } diff --git a/src/supplemental/websocket/websocket.c b/src/supplemental/websocket/websocket.c index a8699663..bb16cf3c 100644 --- a/src/supplemental/websocket/websocket.c +++ b/src/supplemental/websocket/websocket.c @@ -62,6 +62,7 @@ struct nni_ws_listener { char * serv; char * path; nni_mtx mtx; + nni_cv cv; nni_list pend; nni_list reply; nni_list aios; @@ -417,6 +418,7 @@ ws_close_cb(void *arg) nni_http_close(ws->http); nni_aio_cancel(ws->txaio, NNG_ECLOSED); nni_aio_cancel(ws->rxaio, NNG_ECLOSED); + nni_aio_cancel(ws->httpaio, NNG_ECLOSED); // This list (receive) should be empty. while ((wm = nni_list_first(&ws->rxmsgs)) != NULL) { @@ -1122,7 +1124,6 @@ nni_ws_fini(nni_ws *ws) static void ws_http_cb_listener(nni_ws *ws, nni_aio *aio) { - // This is only nni_ws_listener *l; l = nni_aio_get_data(aio, 0); @@ -1140,6 +1141,9 @@ ws_http_cb_listener(nni_ws *ws, nni_aio *aio) } else { nni_list_append(&l->pend, ws); } + if (nni_list_empty(&l->reply)) { + nni_cv_wake(&l->cv); + } nni_mtx_unlock(&l->mtx); } @@ -1240,7 +1244,6 @@ err: static void ws_http_cb(void *arg) { - // This is only done on the server/listener side. nni_ws * ws = arg; nni_aio *aio = ws->httpaio; @@ -1294,11 +1297,18 @@ nni_ws_listener_fini(nni_ws_listener *l) nni_ws_listener_close(l); + nni_mtx_lock(&l->mtx); + while (!nni_list_empty(&l->reply)) { + nni_cv_wait(&l->cv); + } + nni_mtx_unlock(&l->mtx); + if (l->server != NULL) { nni_http_server_fini(l->server); l->server = NULL; } + nni_cv_fini(&l->cv); nni_mtx_fini(&l->mtx); nni_strfree(l->url); nni_strfree(l->proto); @@ -1468,6 +1478,7 @@ err: nni_aio_finish(aio, 0, 0); } } + static int ws_parse_url(const char *url, char **schemep, char **hostp, char **servp, char **pathp, char **queryp) @@ -1570,6 +1581,7 @@ nni_ws_listener_init(nni_ws_listener **wslp, const char *url) return (NNG_ENOMEM); } nni_mtx_init(&l->mtx); + nni_cv_init(&l->cv, &l->mtx); nni_aio_list_init(&l->aios); NNI_LIST_INIT(&l->pend, nni_ws, node); diff --git a/tests/CMakeLists.txt b/tests/CMakeLists.txt index 5f1a834f..7e855f23 100644 --- a/tests/CMakeLists.txt +++ b/tests/CMakeLists.txt @@ -3,8 +3,8 @@ # Copyright (c) 2013 GoPivotal, Inc. All rights reserved. # Copyright (c) 2015-2016 Jack R. Dunaway. All rights reserved. # Copyright 2016 Franklin "Snaipe" Mathieu -# Copyright 2017 Garrett D'Amore -# Copyright 2017 Capitar IT Group BV +# Copyright 2018 Garrett D'Amore +# Copyright 2018 Capitar IT Group BV # # Permission is hereby granted, free of charge, to any person obtaining a copy # of this software and associated documentation files (the "Software"), @@ -137,7 +137,7 @@ add_nng_test(base64 5 NNG_SUPP_BASE64) add_nng_test(device 5 ON) add_nng_test(errors 2 ON) add_nng_test(files 5 ON) -add_nng_test(httpclient 30 NNG_SUPP_HTTP) +add_nng_test(httpclient 60 NNG_SUPP_HTTP) add_nng_test(httpserver 30 NNG_SUPP_HTTP) add_nng_test(idhash 5 ON) add_nng_test(inproc 5 NNG_TRANSPORT_INPROC) diff --git a/tests/httpclient.c b/tests/httpclient.c index 6ea6fa6e..e377d334 100644 --- a/tests/httpclient.c +++ b/tests/httpclient.c @@ -1,6 +1,6 @@ // -// Copyright 2017 Garrett D'Amore -// Copyright 2017 Capitar IT Group BV +// Copyright 2018 Garrett D'Amore +// Copyright 2018 Capitar IT Group BV // // This software is supplied under the terms of the MIT License, a // copy of which should be located in the distribution where this @@ -40,7 +40,7 @@ TestMain("HTTP Client", { iaio = (nni_aio *) aio; iaio->a_addr = &rsa; - nng_aio_set_timeout(aio, 1000); + nng_aio_set_timeout(aio, 20000); nni_plat_tcp_resolv("httpbin.org", "80", NNG_AF_INET, 0, iaio); nng_aio_wait(aio); So(nng_aio_result(aio) == 0); @@ -83,7 +83,7 @@ TestMain("HTTP Client", { So(nng_aio_result(aio) == 0); So(nni_http_res_get_status(res) == 200); - Convey("The message contents are correct", { + Convey("The message contents are correct", { uint8_t digest[20]; void * data; const char *cstr; -- cgit v1.2.3-70-g09d2