From 7c3f419d6628b2ebfcd86fdd3116b35001950819 Mon Sep 17 00:00:00 2001 From: Mark Andrews Date: Fri, 17 Jan 2020 18:24:24 +1100 Subject: [PATCH 1/3] add ISC_MAGIC and reference counting to httpd and httpdmgr --- lib/isc/httpd.c | 157 +++++++++++++++++++++++++++++++++--------------- 1 file changed, 109 insertions(+), 48 deletions(-) diff --git a/lib/isc/httpd.c b/lib/isc/httpd.c index 648f43166f..43c2954837 100644 --- a/lib/isc/httpd.c +++ b/lib/isc/httpd.c @@ -20,6 +20,7 @@ #include #include #include +#include #include #include #include @@ -61,10 +62,19 @@ #define HTTPD_CLOSE 0x0001 /* Got a Connection: close header */ #define HTTPD_FOUNDHOST 0x0002 /* Got a Host: header */ #define HTTPD_KEEPALIVE 0x0004 /* Got a Connection: Keep-Alive */ -#define HTTPD_ACCEPT_DEFLATE 0x0008 +#define HTTPD_ACCEPT_DEFLATE 0x0008 + +#define HTTPD_MAGIC ISC_MAGIC('H', 't', 'p', 'd') +#define VALID_HTTPD(m) ISC_MAGIC_VALID(m, HTTPD_MAGIC) + +#define HTTPDMGR_MAGIC ISC_MAGIC('H', 'p', 'd', 'm') +#define VALID_HTTPDMGR(m) ISC_MAGIC_VALID(m, HTTPDMGR_MAGIC) + /*% http client */ struct isc_httpd { + unsigned int magic; /* HTTPD_MAGIC */ + isc_refcount_t references; isc_httpdmgr_t *mgr; /*%< our parent */ ISC_LINK(isc_httpd_t) link; unsigned int state; @@ -126,6 +136,8 @@ struct isc_httpd { /*% lightweight socket manager for httpd output */ struct isc_httpdmgr { + unsigned int magic; /* HTTPDMGR_MAGIC */ + isc_refcount_t references; isc_mem_t *mctx; isc_socket_t *sock; /*%< listening socket */ isc_task_t *task; /*%< owning task */ @@ -222,8 +234,14 @@ destroy_client(isc_httpd_t **httpdp) { *httpdp = NULL; + if (isc_refcount_decrement(&httpd->references) != 1) { + return; + } + LOCK(&httpdmgr->lock); + httpd->magic = 0; + isc_refcount_destroy(&httpd->references); isc_socket_detach(&httpd->sock); ISC_LIST_UNLINK(httpdmgr->running, httpd, link); @@ -280,6 +298,8 @@ isc_httpdmgr_create(isc_mem_t *mctx, isc_socket_t *sock, isc_task_t *task, ISC_LIST_INIT(httpdmgr->running); ISC_LIST_INIT(httpdmgr->urls); + isc_refcount_init(&httpdmgr->references, 1); + /* XXXMLG ignore errors on isc_socket_listen() */ result = isc_socket_listen(sock, SOMAXCONN); if (result != ISC_R_SUCCESS) { @@ -291,17 +311,24 @@ isc_httpdmgr_create(isc_mem_t *mctx, isc_socket_t *sock, isc_task_t *task, (void)isc_socket_filter(sock, "httpready"); - result = isc_socket_accept(sock, task, isc_httpd_accept, httpdmgr); - if (result != ISC_R_SUCCESS) - goto cleanup; - httpdmgr->render_404 = render_404; httpdmgr->render_500 = render_500; + httpdmgr->magic = HTTPDMGR_MAGIC; + + isc_refcount_increment(&httpdmgr->references); + result = isc_socket_accept(sock, task, isc_httpd_accept, httpdmgr); + if (result != ISC_R_SUCCESS) { + (void) isc_refcount_decrement(&httpdmgr->references); + goto cleanup; + } *httpdmgrp = httpdmgr; return (ISC_R_SUCCESS); cleanup: + httpdmgr->magic = 0; + isc_refcount_decrement(&httpdmgr->references); + isc_refcount_destroy(&httpdmgr->references); isc_task_detach(&httpdmgr->task); isc_socket_detach(&httpdmgr->sock); isc_mem_detach(&httpdmgr->mctx); @@ -312,27 +339,20 @@ isc_httpdmgr_create(isc_mem_t *mctx, isc_socket_t *sock, isc_task_t *task, static void httpdmgr_destroy(isc_httpdmgr_t *httpdmgr) { - isc_mem_t *mctx; isc_httpdurl_t *url; ENTER("httpdmgr_destroy"); + if (isc_refcount_decrement(&httpdmgr->references) != 1) { + return; + } + LOCK(&httpdmgr->lock); - if (!MSHUTTINGDOWN(httpdmgr)) { - NOTICE("httpdmgr_destroy not shutting down yet"); - UNLOCK(&httpdmgr->lock); - return; - } + httpdmgr->magic = 0; - /* - * If all clients are not shut down, don't do anything yet. - */ - if (!ISC_LIST_EMPTY(httpdmgr->running)) { - NOTICE("httpdmgr_destroy clients still active"); - UNLOCK(&httpdmgr->lock); - return; - } + INSIST(MSHUTTINGDOWN(httpdmgr)); + INSIST(ISC_LIST_EMPTY(httpdmgr->running)); NOTICE("httpdmgr_destroy detaching socket, task, and timermgr"); @@ -355,11 +375,11 @@ httpdmgr_destroy(isc_httpdmgr_t *httpdmgr) { UNLOCK(&httpdmgr->lock); isc_mutex_destroy(&httpdmgr->lock); - if (httpdmgr->ondestroy != NULL) + if (httpdmgr->ondestroy != NULL) { (httpdmgr->ondestroy)(httpdmgr->cb_arg); - - mctx = httpdmgr->mctx; - isc_mem_putanddetach(&mctx, httpdmgr, sizeof(isc_httpdmgr_t)); + } + isc_refcount_destroy(&httpdmgr->references); + isc_mem_putanddetach(&httpdmgr->mctx, httpdmgr, sizeof(isc_httpdmgr_t)); EXIT("httpdmgr_destroy"); } @@ -634,6 +654,8 @@ isc_httpd_accept(isc_task_t *task, isc_event_t *ev) { httpd = isc_mem_get(httpdmgr->mctx, sizeof(isc_httpd_t)); + isc_refcount_init(&httpd->references, 1); + isc_refcount_increment(&httpdmgr->references); httpd->mgr = httpdmgr; ISC_LINK_INIT(httpd, link); ISC_LIST_APPEND(httpdmgr->running, httpd, link); @@ -652,20 +674,24 @@ isc_httpd_accept(isc_task_t *task, isc_event_t *ev) { isc_buffer_initnull(&httpd->bodybuffer); httpd->sendbuffer = NULL; reset_client(httpd); + httpd->magic = HTTPD_MAGIC; r.base = (unsigned char *)httpd->recvbuf; r.length = HTTP_RECVLEN - 1; result = isc_socket_recv(httpd->sock, &r, 1, task, isc_httpd_recvdone, httpd); - /* FIXME!!! */ - POST(result); + if (result != ISC_R_SUCCESS) { + destroy_client(&httpd); + } NOTICE("accept queued recv on socket"); requeue: + isc_refcount_increment(&httpdmgr->references); result = isc_socket_accept(httpdmgr->sock, task, isc_httpd_accept, httpdmgr); if (result != ISC_R_SUCCESS) { - /* XXXMLG what to do? Log failure... */ + int32_t refs = isc_refcount_decrement(&httpdmgr->references); + INSIST(refs > 1); NOTICE("accept could not reaccept due to failure"); } @@ -834,24 +860,25 @@ isc_httpd_recvdone(isc_task_t *task, isc_event_t *ev) { if (sev->result != ISC_R_SUCCESS) { NOTICE("recv destroying client"); - destroy_client(&httpd); goto out; } result = process_request(httpd, sev->n); if (result == ISC_R_NOTFOUND) { if (httpd->recvlen >= HTTP_RECVLEN - 1) { - destroy_client(&httpd); goto out; } r.base = (unsigned char *)httpd->recvbuf + httpd->recvlen; r.length = HTTP_RECVLEN - httpd->recvlen - 1; - /* check return code? */ - (void)isc_socket_recv(httpd->sock, &r, 1, task, - isc_httpd_recvdone, httpd); + isc_refcount_increment(&httpd->references); + result = isc_socket_recv(httpd->sock, &r, 1, task, + isc_httpd_recvdone, httpd); + if (result != ISC_R_SUCCESS) { + uint32_t refs = isc_refcount_decrement(&httpd->references); + INSIST(refs > 1); + } goto out; } else if (result != ISC_R_SUCCESS) { - destroy_client(&httpd); goto out; } @@ -864,13 +891,16 @@ isc_httpd_recvdone(isc_task_t *task, isc_event_t *ev) { isc_buffer_initnull(&httpd->bodybuffer); isc_time_now(&now); isc_time_formathttptimestamp(&now, datebuf, sizeof(datebuf)); + LOCK(&httpd->mgr->lock); url = ISC_LIST_HEAD(httpd->mgr->urls); while (url != NULL) { if (strcmp(httpd->url, url->url) == 0) break; url = ISC_LIST_NEXT(url, link); } - if (url == NULL) + UNLOCK(&httpd->mgr->lock); + + if (url == NULL) { result = httpd->mgr->render_404(httpd->url, NULL, httpd->querystring, NULL, NULL, @@ -880,7 +910,7 @@ isc_httpd_recvdone(isc_task_t *task, isc_event_t *ev) { &httpd->bodybuffer, &httpd->freecb, &httpd->freecb_arg); - else + } else { result = url->action(httpd->url, url, httpd->querystring, httpd->headers, @@ -888,6 +918,7 @@ isc_httpd_recvdone(isc_task_t *task, isc_event_t *ev) { &httpd->retcode, &httpd->retmsg, &httpd->mimetype, &httpd->bodybuffer, &httpd->freecb, &httpd->freecb_arg); + } if (result != ISC_R_SUCCESS) { result = httpd->mgr->render_500(httpd->url, url, httpd->querystring, @@ -911,8 +942,9 @@ isc_httpd_recvdone(isc_task_t *task, isc_event_t *ev) { #endif isc_httpd_response(httpd); - if ((httpd->flags & HTTPD_KEEPALIVE) != 0) + if ((httpd->flags & HTTPD_KEEPALIVE) != 0) { isc_httpd_addheader(httpd, "Connection", "Keep-Alive"); + } isc_httpd_addheader(httpd, "Content-Type", httpd->mimetype); isc_httpd_addheader(httpd, "Date", datebuf); isc_httpd_addheader(httpd, "Expires", datebuf); @@ -934,7 +966,7 @@ isc_httpd_recvdone(isc_task_t *task, isc_event_t *ev) { if (is_compressed == true) { isc_httpd_addheader(httpd, "Content-Encoding", "deflate"); isc_httpd_addheaderuint(httpd, "Content-Length", - isc_buffer_usedlength(&httpd->compbuffer)); + isc_buffer_usedlength(&httpd->compbuffer)); } else { isc_httpd_addheaderuint(httpd, "Content-Length", isc_buffer_usedlength(&httpd->bodybuffer)); @@ -958,11 +990,16 @@ isc_httpd_recvdone(isc_task_t *task, isc_event_t *ev) { */ isc_buffer_usedregion(httpd->sendbuffer, &r); - /* check return code? */ - (void)isc_socket_send(httpd->sock, &r, task, - isc_httpd_senddone, httpd); + isc_refcount_increment(&httpd->references); + result = isc_socket_send(httpd->sock, &r, task, + isc_httpd_senddone, httpd); + if (result != ISC_R_SUCCESS) { + uint32_t refs = isc_refcount_decrement(&httpd->references); + INSIST(refs > 1); + } out: + destroy_client(&httpd); isc_event_free(&ev); EXIT("recv"); } @@ -971,8 +1008,11 @@ void isc_httpdmgr_shutdown(isc_httpdmgr_t **httpdmgrp) { isc_httpdmgr_t *httpdmgr; isc_httpd_t *httpd; + + REQUIRE(httpdmgrp != NULL); httpdmgr = *httpdmgrp; *httpdmgrp = NULL; + REQUIRE(VALID_HTTPDMGR(httpdmgr)); ENTER("isc_httpdmgr_shutdown"); @@ -991,6 +1031,8 @@ isc_httpdmgr_shutdown(isc_httpdmgr_t **httpdmgrp) { UNLOCK(&httpdmgr->lock); + httpdmgr_destroy(httpdmgr); + EXIT("isc_httpdmgr_shutdown"); } @@ -1019,6 +1061,8 @@ isc_httpd_response(isc_httpd_t *httpd) { isc_result_t result; unsigned int needlen; + REQUIRE(VALID_HTTPD(httpd)); + needlen = strlen(httpd->protocol) + 1; /* protocol + space */ needlen += 3 + 1; /* room for response code, always 3 bytes */ needlen += strlen(httpd->retmsg) + 2; /* return msg + CRLF */ @@ -1035,15 +1079,16 @@ isc_httpd_response(isc_httpd_t *httpd) { } isc_result_t -isc_httpd_addheader(isc_httpd_t *httpd, const char *name, - const char *val) -{ +isc_httpd_addheader(isc_httpd_t *httpd, const char *name, const char *val) { isc_result_t result; unsigned int needlen; + REQUIRE(VALID_HTTPD(httpd)); + needlen = strlen(name); /* name itself */ - if (val != NULL) + if (val != NULL) { needlen += 2 + strlen(val); /* : and val */ + } needlen += 2; /* CRLF */ while (isc_buffer_availablelength(&httpd->headerbuffer) < needlen) { @@ -1065,6 +1110,8 @@ isc_result_t isc_httpd_endheaders(isc_httpd_t *httpd) { isc_result_t result; + REQUIRE(VALID_HTTPD(httpd)); + while (isc_buffer_availablelength(&httpd->headerbuffer) < 2) { result = grow_headerspace(httpd); if (result != ISC_R_SUCCESS) @@ -1080,6 +1127,8 @@ isc_httpd_addheaderuint(isc_httpd_t *httpd, const char *name, int val) { unsigned int needlen; char buf[sizeof "18446744073709551616"]; + REQUIRE(VALID_HTTPD(httpd)); + snprintf(buf, sizeof(buf), "%d", val); needlen = strlen(name); /* name itself */ @@ -1101,6 +1150,9 @@ isc_httpd_senddone(isc_task_t *task, isc_event_t *ev) { isc_httpd_t *httpd = ev->ev_arg; isc_region_t r; isc_socketevent_t *sev = (isc_socketevent_t *)ev; + isc_result_t result; + + REQUIRE(VALID_HTTPD(httpd)); ENTER("senddone"); INSIST(ISC_HTTPD_ISSEND(httpd)); @@ -1124,12 +1176,10 @@ isc_httpd_senddone(isc_task_t *task, isc_event_t *ev) { } if (sev->result != ISC_R_SUCCESS) { - destroy_client(&httpd); goto out; } if ((httpd->flags & HTTPD_CLOSE) != 0) { - destroy_client(&httpd); goto out; } @@ -1141,11 +1191,17 @@ isc_httpd_senddone(isc_task_t *task, isc_event_t *ev) { r.base = (unsigned char *)httpd->recvbuf; r.length = HTTP_RECVLEN - 1; - /* check return code? */ - (void)isc_socket_recv(httpd->sock, &r, 1, task, - isc_httpd_recvdone, httpd); + + isc_refcount_increment(&httpd->references); + result = isc_socket_recv(httpd->sock, &r, 1, task, + isc_httpd_recvdone, httpd); + if (result != ISC_R_SUCCESS) { + uint32_t refs = isc_refcount_decrement(&httpd->references); + INSIST(refs > 1); + } out: + destroy_client(&httpd); isc_event_free(&ev); EXIT("senddone"); } @@ -1188,6 +1244,8 @@ isc_httpdmgr_addurl2(isc_httpdmgr_t *httpdmgr, const char *url, { isc_httpdurl_t *item; + REQUIRE(VALID_HTTPDMGR(httpdmgr)); + if (url == NULL) { httpdmgr->render_404 = func; return (ISC_R_SUCCESS); @@ -1203,7 +1261,10 @@ isc_httpdmgr_addurl2(isc_httpdmgr_t *httpdmgr, const char *url, isc_time_now(&item->loadtime); ISC_LINK_INIT(item, link); + + LOCK(&httpdmgr->lock); ISC_LIST_APPEND(httpdmgr->urls, item, link); + UNLOCK(&httpdmgr->lock); return (ISC_R_SUCCESS); } From 9643a62dd53129208e534e95cbf89f2571ea6122 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ond=C5=99ej=20Sur=C3=BD?= Date: Mon, 20 Jan 2020 12:37:57 +0100 Subject: [PATCH 2/3] Refactor parts of isc_httpd and isc_httpd for better readability and safety --- lib/isc/httpd.c | 311 +++++++++++++++++++++++------------- lib/isc/include/isc/httpd.h | 2 +- 2 files changed, 202 insertions(+), 111 deletions(-) diff --git a/lib/isc/httpd.c b/lib/isc/httpd.c index 43c2954837..b4e9ccab36 100644 --- a/lib/isc/httpd.c +++ b/lib/isc/httpd.c @@ -34,9 +34,6 @@ /*% * TODO: * - * o Put in better checks to make certain things are passed in correctly. - * This includes a magic number for externally-visible structures, - * checking for NULL-ness before dereferencing, etc. * o Make the URL processing external functions which will fill-in a buffer * structure we provide, or return an error and we will render a generic * page and close the client. @@ -106,8 +103,8 @@ struct isc_httpd { * to the client. * * The bufflist is the list of buffers we are currently transmitting. - * The headerbuffer is where we render our headers to. If we run out of - * space when rendering a header, we will change the size of our + * The headerbuffer is where we render our headers to. If we run out + * of space when rendering a header, we will change the size of our * buffer. We will not free it until we are finished, and will * allocate an additional HTTP_SENDGROW bytes per header space grow. * @@ -215,54 +212,124 @@ struct isc_httpdmgr { static void isc_httpd_accept(isc_task_t *, isc_event_t *); static void isc_httpd_recvdone(isc_task_t *, isc_event_t *); static void isc_httpd_senddone(isc_task_t *, isc_event_t *); -static void destroy_client(isc_httpd_t **); static isc_result_t process_request(isc_httpd_t *, int); -static void httpdmgr_destroy(isc_httpdmgr_t *); static isc_result_t grow_headerspace(isc_httpd_t *); static void reset_client(isc_httpd_t *httpd); static isc_httpdaction_t render_404; static isc_httpdaction_t render_500; +#if ENABLE_AFL static void (*finishhook)(void) = NULL; +#endif /* ENABLE_AFL */ + +static void maybe_destroy_httpd(isc_httpd_t *); +static void destroy_httpd(isc_httpd_t *); +static void maybe_destroy_httpdmgr(isc_httpdmgr_t *); +static void destroy_httpdmgr(isc_httpdmgr_t *); static void -destroy_client(isc_httpd_t **httpdp) { - isc_httpd_t *httpd = *httpdp; - isc_httpdmgr_t *httpdmgr = httpd->mgr; +isc_httpdmgr_attach(isc_httpdmgr_t *, isc_httpdmgr_t **); +static void +isc_httpdmgr_detach(isc_httpdmgr_t **); + +static void +maybe_destroy_httpd(isc_httpd_t *httpd) { + if (isc_refcount_decrement(&httpd->references) == 1) { + destroy_httpd(httpd); + } +} + +static inline void +free_buffer(isc_mem_t *mctx, isc_buffer_t *buffer) { isc_region_t r; - *httpdp = NULL; - - if (isc_refcount_decrement(&httpd->references) != 1) { - return; + isc_buffer_region(buffer, &r); + if (r.length > 0) { + isc_mem_put(mctx, r.base, r.length); } +} +static void +destroy_httpd(isc_httpd_t *httpd) { + isc_httpdmgr_t *httpdmgr; + + REQUIRE(VALID_HTTPD(httpd)); + + httpdmgr = httpd->mgr; + REQUIRE(VALID_HTTPDMGR(httpdmgr)); + + /* + * Unlink before calling isc_socket_detach so + * isc_httpdmgr_shutdown does not dereference a NULL pointer + * when calling isc_socket_cancel(). + */ LOCK(&httpdmgr->lock); + ISC_LIST_UNLINK(httpdmgr->running, httpd, link); + UNLOCK(&httpdmgr->lock); httpd->magic = 0; isc_refcount_destroy(&httpd->references); isc_socket_detach(&httpd->sock); - ISC_LIST_UNLINK(httpdmgr->running, httpd, link); - isc_buffer_region(&httpd->headerbuffer, &r); - if (r.length > 0) { - isc_mem_put(httpdmgr->mctx, r.base, r.length); - } - isc_buffer_region(&httpd->compbuffer, &r); - if (r.length > 0) { - isc_mem_put(httpdmgr->mctx, r.base, r.length); - } + free_buffer(httpdmgr->mctx, &httpd->headerbuffer); + free_buffer(httpdmgr->mctx, &httpd->compbuffer); isc_mem_put(httpdmgr->mctx, httpd, sizeof(isc_httpd_t)); - UNLOCK(&httpdmgr->lock); - - if (finishhook != NULL) +#if ENABLE_AFL + if (finishhook != NULL) { finishhook(); + } +#endif /* ENABLE_AFL */ - httpdmgr_destroy(httpdmgr); + isc_httpdmgr_detach(&httpdmgr); +} + +static inline isc_result_t +httpdmgr_socket_accept(isc_task_t *task, isc_httpdmgr_t* httpdmgr) +{ + isc_result_t result = ISC_R_SUCCESS; + + /* decremented in isc_httpd_accept */ + isc_refcount_increment(&httpdmgr->references); + result = isc_socket_accept(httpdmgr->sock, task, isc_httpd_accept, + httpdmgr); + if (result != ISC_R_SUCCESS) { + INSIST(isc_refcount_decrement(&httpdmgr->references) > 1); + } + return (result); +} + +static inline isc_result_t +httpd_socket_recv(isc_httpd_t *httpd, isc_region_t *region, isc_task_t *task) +{ + isc_result_t result = ISC_R_SUCCESS; + + /* decremented in isc_httpd_recvdone */ + (void)isc_refcount_increment(&httpd->references); + result = isc_socket_recv(httpd->sock, region, 1, task, + isc_httpd_recvdone, httpd); + if (result != ISC_R_SUCCESS) { + INSIST(isc_refcount_decrement(&httpd->references) > 1); + } + return (result); +} + +static inline isc_result_t +httpd_socket_send(isc_httpd_t *httpd, isc_region_t *region, isc_task_t *task) +{ + isc_result_t result = ISC_R_SUCCESS; + + /* decremented in isc_httpd_senddone */ + (void)isc_refcount_increment(&httpd->references); + result = isc_socket_send(httpd->sock, region, task, + isc_httpd_senddone, httpd); + if (result != ISC_R_SUCCESS) { + INSIST(isc_refcount_decrement(&httpd->references) > 1); + } + return (result); } isc_result_t @@ -282,18 +349,19 @@ isc_httpdmgr_create(isc_mem_t *mctx, isc_socket_t *sock, isc_task_t *task, httpdmgr = isc_mem_get(mctx, sizeof(isc_httpdmgr_t)); + *httpdmgr = (isc_httpdmgr_t){ + .timermgr = tmgr, /* XXXMLG no attach function? */ + .client_ok = client_ok, + .ondestroy = ondestroy, + .cb_arg = cb_arg, + .render_404 = render_404, + .render_500 = render_500 + }; + isc_mutex_init(&httpdmgr->lock); - httpdmgr->mctx = NULL; isc_mem_attach(mctx, &httpdmgr->mctx); - httpdmgr->sock = NULL; isc_socket_attach(sock, &httpdmgr->sock); - httpdmgr->task = NULL; isc_task_attach(task, &httpdmgr->task); - httpdmgr->timermgr = tmgr; /* XXXMLG no attach function? */ - httpdmgr->client_ok = client_ok; - httpdmgr->ondestroy = ondestroy; - httpdmgr->cb_arg = cb_arg; - httpdmgr->flags = 0; ISC_LIST_INIT(httpdmgr->running); ISC_LIST_INIT(httpdmgr->urls); @@ -311,14 +379,11 @@ isc_httpdmgr_create(isc_mem_t *mctx, isc_socket_t *sock, isc_task_t *task, (void)isc_socket_filter(sock, "httpready"); - httpdmgr->render_404 = render_404; - httpdmgr->render_500 = render_500; httpdmgr->magic = HTTPDMGR_MAGIC; - isc_refcount_increment(&httpdmgr->references); - result = isc_socket_accept(sock, task, isc_httpd_accept, httpdmgr); + + result = httpdmgr_socket_accept(task, httpdmgr); if (result != ISC_R_SUCCESS) { - (void) isc_refcount_decrement(&httpdmgr->references); goto cleanup; } @@ -338,14 +403,36 @@ isc_httpdmgr_create(isc_mem_t *mctx, isc_socket_t *sock, isc_task_t *task, } static void -httpdmgr_destroy(isc_httpdmgr_t *httpdmgr) { +isc_httpdmgr_attach(isc_httpdmgr_t *source, isc_httpdmgr_t **targetp) { + REQUIRE(VALID_HTTPDMGR(source)); + REQUIRE(targetp != NULL && *targetp == NULL); + + isc_refcount_increment(&source->references); + + *targetp = source; +} + +static void +isc_httpdmgr_detach(isc_httpdmgr_t **httpdmgrp) { + REQUIRE(httpdmgrp != NULL && VALID_HTTPDMGR(*httpdmgrp)); + isc_httpdmgr_t *httpdmgr = *httpdmgrp; + *httpdmgrp = NULL; + + maybe_destroy_httpdmgr(httpdmgr); +} + +static void +maybe_destroy_httpdmgr(isc_httpdmgr_t *httpdmgr) { + if (isc_refcount_decrement(&httpdmgr->references) == 1) { + destroy_httpdmgr(httpdmgr); + } +} + +static void +destroy_httpdmgr(isc_httpdmgr_t *httpdmgr) { isc_httpdurl_t *url; - ENTER("httpdmgr_destroy"); - - if (isc_refcount_decrement(&httpdmgr->references) != 1) { - return; - } + isc_refcount_destroy(&httpdmgr->references); LOCK(&httpdmgr->lock); @@ -378,10 +465,7 @@ httpdmgr_destroy(isc_httpdmgr_t *httpdmgr) { if (httpdmgr->ondestroy != NULL) { (httpdmgr->ondestroy)(httpdmgr->cb_arg); } - isc_refcount_destroy(&httpdmgr->references); isc_mem_putanddetach(&httpdmgr->mctx, httpdmgr, sizeof(isc_httpdmgr_t)); - - EXIT("httpdmgr_destroy"); } #define LENGTHOK(s) (httpd->recvbuf - (s) < (int)httpd->recvlen) @@ -616,15 +700,56 @@ process_request(isc_httpd_t *httpd, int length) { return (ISC_R_SUCCESS); } +static void +isc_httpd_create(isc_httpdmgr_t *httpdmgr, isc_socket_t *sock, + isc_httpd_t **httpdp) +{ + isc_httpd_t *httpd; + char *headerdata; + + REQUIRE(VALID_HTTPDMGR(httpdmgr)); + REQUIRE(httpdp != NULL && *httpdp == NULL); + + httpd = isc_mem_get(httpdmgr->mctx, sizeof(isc_httpd_t)); + + *httpd = (isc_httpd_t){ + .sock = sock + }; + + isc_httpdmgr_attach(httpdmgr, &httpd->mgr); + + isc_refcount_init(&httpd->references, 1); + ISC_HTTPD_SETRECV(httpd); + isc_socket_setname(httpd->sock, "httpd", NULL); + + /* + * Initialize the buffer for our headers. + */ + headerdata = isc_mem_get(httpdmgr->mctx, HTTP_SENDGROW); + isc_buffer_init(&httpd->headerbuffer, headerdata, HTTP_SENDGROW); + + isc_buffer_initnull(&httpd->compbuffer); + isc_buffer_initnull(&httpd->bodybuffer); + reset_client(httpd); + + ISC_LINK_INIT(httpd, link); + ISC_LIST_APPEND(httpdmgr->running, httpd, link); + + httpd->magic = HTTPD_MAGIC; + + *httpdp = httpd; +} + static void isc_httpd_accept(isc_task_t *task, isc_event_t *ev) { isc_result_t result; isc_httpdmgr_t *httpdmgr = ev->ev_arg; - isc_httpd_t *httpd; + isc_httpd_t *httpd = NULL; isc_region_t r; isc_socket_newconnev_t *nev = (isc_socket_newconnev_t *)ev; isc_sockaddr_t peeraddr; - char *headerdata; + + REQUIRE(VALID_HTTPDMGR(httpdmgr)); ENTER("accept"); @@ -652,53 +777,29 @@ isc_httpd_accept(isc_task_t *task, isc_event_t *ev) { goto requeue; } - httpd = isc_mem_get(httpdmgr->mctx, sizeof(isc_httpd_t)); - - isc_refcount_init(&httpd->references, 1); - isc_refcount_increment(&httpdmgr->references); - httpd->mgr = httpdmgr; - ISC_LINK_INIT(httpd, link); - ISC_LIST_APPEND(httpdmgr->running, httpd, link); - ISC_HTTPD_SETRECV(httpd); - httpd->sock = nev->newsocket; - isc_socket_setname(httpd->sock, "httpd", NULL); - httpd->flags = 0; - - /* - * Initialize the buffer for our headers. - */ - headerdata = isc_mem_get(httpdmgr->mctx, HTTP_SENDGROW); - isc_buffer_init(&httpd->headerbuffer, headerdata, HTTP_SENDGROW); - - isc_buffer_initnull(&httpd->compbuffer); - isc_buffer_initnull(&httpd->bodybuffer); - httpd->sendbuffer = NULL; - reset_client(httpd); - httpd->magic = HTTPD_MAGIC; + isc_httpd_create(httpdmgr, nev->newsocket, &httpd); r.base = (unsigned char *)httpd->recvbuf; r.length = HTTP_RECVLEN - 1; - result = isc_socket_recv(httpd->sock, &r, 1, task, isc_httpd_recvdone, - httpd); - if (result != ISC_R_SUCCESS) { - destroy_client(&httpd); + + result = httpd_socket_recv(httpd, &r, task); + if (result == ISC_R_SUCCESS) { + NOTICE("accept queued recv on socket"); } - NOTICE("accept queued recv on socket"); requeue: - isc_refcount_increment(&httpdmgr->references); - result = isc_socket_accept(httpdmgr->sock, task, isc_httpd_accept, - httpdmgr); + result = httpdmgr_socket_accept(task, httpdmgr); if (result != ISC_R_SUCCESS) { - int32_t refs = isc_refcount_decrement(&httpdmgr->references); - INSIST(refs > 1); NOTICE("accept could not reaccept due to failure"); } out: UNLOCK(&httpdmgr->lock); - httpdmgr_destroy(httpdmgr); + if (httpd != NULL) { + maybe_destroy_httpd(httpd); + } + maybe_destroy_httpdmgr(httpdmgr); isc_event_free(&ev); @@ -854,6 +955,8 @@ isc_httpd_recvdone(isc_task_t *task, isc_event_t *ev) { bool is_compressed = false; char datebuf[ISC_FORMATHTTPTIMESTAMP_SIZE]; + REQUIRE(VALID_HTTPD(httpd)); + ENTER("recv"); INSIST(ISC_HTTPD_ISRECV(httpd)); @@ -870,13 +973,8 @@ isc_httpd_recvdone(isc_task_t *task, isc_event_t *ev) { } r.base = (unsigned char *)httpd->recvbuf + httpd->recvlen; r.length = HTTP_RECVLEN - httpd->recvlen - 1; - isc_refcount_increment(&httpd->references); - result = isc_socket_recv(httpd->sock, &r, 1, task, - isc_httpd_recvdone, httpd); - if (result != ISC_R_SUCCESS) { - uint32_t refs = isc_refcount_decrement(&httpd->references); - INSIST(refs > 1); - } + + httpd_socket_recv(httpd, &r, task); goto out; } else if (result != ISC_R_SUCCESS) { goto out; @@ -990,16 +1088,10 @@ isc_httpd_recvdone(isc_task_t *task, isc_event_t *ev) { */ isc_buffer_usedregion(httpd->sendbuffer, &r); - isc_refcount_increment(&httpd->references); - result = isc_socket_send(httpd->sock, &r, task, - isc_httpd_senddone, httpd); - if (result != ISC_R_SUCCESS) { - uint32_t refs = isc_refcount_decrement(&httpd->references); - INSIST(refs > 1); - } + httpd_socket_send(httpd, &r, task); out: - destroy_client(&httpd); + maybe_destroy_httpd(httpd); isc_event_free(&ev); EXIT("recv"); } @@ -1031,7 +1123,7 @@ isc_httpdmgr_shutdown(isc_httpdmgr_t **httpdmgrp) { UNLOCK(&httpdmgr->lock); - httpdmgr_destroy(httpdmgr); + maybe_destroy_httpdmgr(httpdmgr); EXIT("isc_httpdmgr_shutdown"); } @@ -1150,7 +1242,6 @@ isc_httpd_senddone(isc_task_t *task, isc_event_t *ev) { isc_httpd_t *httpd = ev->ev_arg; isc_region_t r; isc_socketevent_t *sev = (isc_socketevent_t *)ev; - isc_result_t result; REQUIRE(VALID_HTTPD(httpd)); @@ -1192,16 +1283,10 @@ isc_httpd_senddone(isc_task_t *task, isc_event_t *ev) { r.base = (unsigned char *)httpd->recvbuf; r.length = HTTP_RECVLEN - 1; - isc_refcount_increment(&httpd->references); - result = isc_socket_recv(httpd->sock, &r, 1, task, - isc_httpd_recvdone, httpd); - if (result != ISC_R_SUCCESS) { - uint32_t refs = isc_refcount_decrement(&httpd->references); - INSIST(refs > 1); - } + httpd_socket_recv(httpd, &r, task); out: - destroy_client(&httpd); + maybe_destroy_httpd(httpd); isc_event_free(&ev); EXIT("senddone"); } @@ -1234,6 +1319,8 @@ isc_result_t isc_httpdmgr_addurl(isc_httpdmgr_t *httpdmgr, const char *url, isc_httpdaction_t *func, void *arg) { + /* REQUIRE(VALID_HTTPDMGR(httpdmgr)); Dummy function */ + return (isc_httpdmgr_addurl2(httpdmgr, url, false, func, arg)); } @@ -1272,5 +1359,9 @@ isc_httpdmgr_addurl2(isc_httpdmgr_t *httpdmgr, const char *url, void isc_httpd_setfinishhook(void (*fn)(void)) { +#if ENABLE_AFL finishhook = fn; +#else /* ENABLE_AFL */ + UNUSED(fn); +#endif /* ENABLE_AFL */ } diff --git a/lib/isc/include/isc/httpd.h b/lib/isc/include/isc/httpd.h index b02b33e83d..b8b476f222 100644 --- a/lib/isc/include/isc/httpd.h +++ b/lib/isc/include/isc/httpd.h @@ -35,7 +35,7 @@ struct isc_httpdurl { char *url; isc_httpdaction_t *action; void *action_arg; - bool isstatic; + bool isstatic; isc_time_t loadtime; ISC_LINK(isc_httpdurl_t) link; }; From 5b448996e51609b2ed9c9a8c3a952f70e695694f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ond=C5=99ej=20Sur=C3=BD?= Date: Tue, 21 Jan 2020 08:25:30 +0100 Subject: [PATCH 3/3] Clean the ENTER/EXIT/NOTICE debugging from production code --- lib/isc/httpd.c | 52 ++++--------------------------------------------- 1 file changed, 4 insertions(+), 48 deletions(-) diff --git a/lib/isc/httpd.c b/lib/isc/httpd.c index b4e9ccab36..88948fb515 100644 --- a/lib/isc/httpd.c +++ b/lib/isc/httpd.c @@ -42,16 +42,6 @@ #define MSHUTTINGDOWN(cm) ((cm->flags & ISC_HTTPDMGR_FLAGSHUTTINGDOWN) != 0) #define MSETSHUTTINGDOWN(cm) (cm->flags |= ISC_HTTPDMGR_FLAGSHUTTINGDOWN) -#ifdef DEBUG_HTTPD -#define ENTER(x) do { fprintf(stderr, "ENTER %s\n", (x)); } while (0) -#define EXIT(x) do { fprintf(stderr, "EXIT %s\n", (x)); } while (0) -#define NOTICE(x) do { fprintf(stderr, "NOTICE %s\n", (x)); } while (0) -#else -#define ENTER(x) do { } while(0) -#define EXIT(x) do { } while(0) -#define NOTICE(x) do { } while(0) -#endif - #define HTTP_RECVLEN 1024 #define HTTP_SENDGROW 1024 #define HTTP_SEND_MAXLEN 10240 @@ -302,7 +292,7 @@ httpdmgr_socket_accept(isc_task_t *task, isc_httpdmgr_t* httpdmgr) return (result); } -static inline isc_result_t +static inline void httpd_socket_recv(isc_httpd_t *httpd, isc_region_t *region, isc_task_t *task) { isc_result_t result = ISC_R_SUCCESS; @@ -314,10 +304,9 @@ httpd_socket_recv(isc_httpd_t *httpd, isc_region_t *region, isc_task_t *task) if (result != ISC_R_SUCCESS) { INSIST(isc_refcount_decrement(&httpd->references) > 1); } - return (result); } -static inline isc_result_t +static inline void httpd_socket_send(isc_httpd_t *httpd, isc_region_t *region, isc_task_t *task) { isc_result_t result = ISC_R_SUCCESS; @@ -329,7 +318,6 @@ httpd_socket_send(isc_httpd_t *httpd, isc_region_t *region, isc_task_t *task) if (result != ISC_R_SUCCESS) { INSIST(isc_refcount_decrement(&httpd->references) > 1); } - return (result); } isc_result_t @@ -441,8 +429,6 @@ destroy_httpdmgr(isc_httpdmgr_t *httpdmgr) { INSIST(MSHUTTINGDOWN(httpdmgr)); INSIST(ISC_LIST_EMPTY(httpdmgr->running)); - NOTICE("httpdmgr_destroy detaching socket, task, and timermgr"); - isc_socket_detach(&httpdmgr->sock); isc_task_detach(&httpdmgr->task); httpdmgr->timermgr = NULL; @@ -544,8 +530,6 @@ process_request(isc_httpd_t *httpd, int length) { char *p; int delim; - ENTER("request"); - httpd->recvlen += length; httpd->recvbuf[httpd->recvlen] = 0; @@ -695,8 +679,6 @@ process_request(isc_httpd_t *httpd, int length) { && ((httpd->flags & HTTPD_FOUNDHOST) == 0)) return (ISC_R_RANGE); - EXIT("request"); - return (ISC_R_SUCCESS); } @@ -742,7 +724,6 @@ isc_httpd_create(isc_httpdmgr_t *httpdmgr, isc_socket_t *sock, static void isc_httpd_accept(isc_task_t *task, isc_event_t *ev) { - isc_result_t result; isc_httpdmgr_t *httpdmgr = ev->ev_arg; isc_httpd_t *httpd = NULL; isc_region_t r; @@ -751,22 +732,17 @@ isc_httpd_accept(isc_task_t *task, isc_event_t *ev) { REQUIRE(VALID_HTTPDMGR(httpdmgr)); - ENTER("accept"); - LOCK(&httpdmgr->lock); if (MSHUTTINGDOWN(httpdmgr)) { - NOTICE("accept shutting down, goto out"); goto out; } if (nev->result == ISC_R_CANCELED) { - NOTICE("accept canceled, goto out"); goto out; } if (nev->result != ISC_R_SUCCESS) { /* XXXMLG log failure */ - NOTICE("accept returned failure, goto requeue"); goto requeue; } @@ -782,16 +758,10 @@ isc_httpd_accept(isc_task_t *task, isc_event_t *ev) { r.base = (unsigned char *)httpd->recvbuf; r.length = HTTP_RECVLEN - 1; - result = httpd_socket_recv(httpd, &r, task); - if (result == ISC_R_SUCCESS) { - NOTICE("accept queued recv on socket"); - } + httpd_socket_recv(httpd, &r, task); requeue: - result = httpdmgr_socket_accept(task, httpdmgr); - if (result != ISC_R_SUCCESS) { - NOTICE("accept could not reaccept due to failure"); - } + (void)httpdmgr_socket_accept(task, httpdmgr); out: UNLOCK(&httpdmgr->lock); @@ -802,8 +772,6 @@ isc_httpd_accept(isc_task_t *task, isc_event_t *ev) { maybe_destroy_httpdmgr(httpdmgr); isc_event_free(&ev); - - EXIT("accept"); } static isc_result_t @@ -957,12 +925,9 @@ isc_httpd_recvdone(isc_task_t *task, isc_event_t *ev) { REQUIRE(VALID_HTTPD(httpd)); - ENTER("recv"); - INSIST(ISC_HTTPD_ISRECV(httpd)); if (sev->result != ISC_R_SUCCESS) { - NOTICE("recv destroying client"); goto out; } @@ -1093,7 +1058,6 @@ isc_httpd_recvdone(isc_task_t *task, isc_event_t *ev) { out: maybe_destroy_httpd(httpd); isc_event_free(&ev); - EXIT("recv"); } void @@ -1106,8 +1070,6 @@ isc_httpdmgr_shutdown(isc_httpdmgr_t **httpdmgrp) { *httpdmgrp = NULL; REQUIRE(VALID_HTTPDMGR(httpdmgr)); - ENTER("isc_httpdmgr_shutdown"); - LOCK(&httpdmgr->lock); MSETSHUTTINGDOWN(httpdmgr); @@ -1125,7 +1087,6 @@ isc_httpdmgr_shutdown(isc_httpdmgr_t **httpdmgrp) { maybe_destroy_httpdmgr(httpdmgr); - EXIT("isc_httpdmgr_shutdown"); } static isc_result_t @@ -1245,7 +1206,6 @@ isc_httpd_senddone(isc_task_t *task, isc_event_t *ev) { REQUIRE(VALID_HTTPD(httpd)); - ENTER("senddone"); INSIST(ISC_HTTPD_ISSEND(httpd)); isc_buffer_free(&httpd->sendbuffer); @@ -1263,7 +1223,6 @@ isc_httpd_senddone(isc_task_t *task, isc_event_t *ev) { b = &httpd->bodybuffer; httpd->freecb(b, httpd->freecb_arg); } - NOTICE("senddone free callback performed"); } if (sev->result != ISC_R_SUCCESS) { @@ -1276,8 +1235,6 @@ isc_httpd_senddone(isc_task_t *task, isc_event_t *ev) { ISC_HTTPD_SETRECV(httpd); - NOTICE("senddone restarting recv on socket"); - reset_client(httpd); r.base = (unsigned char *)httpd->recvbuf; @@ -1288,7 +1245,6 @@ isc_httpd_senddone(isc_task_t *task, isc_event_t *ev) { out: maybe_destroy_httpd(httpd); isc_event_free(&ev); - EXIT("senddone"); } static void