From 092a24eea6ae494be8f7a5fe543e634cca01022e Mon Sep 17 00:00:00 2001 From: Garrett D'Amore Date: Mon, 30 Dec 2019 14:18:35 -0800 Subject: fixes #1075 WebSocket heap use after free This also introduces a new atomic boolean type, so we can use that to trigger whether we've added the HTTP handler or not. --- src/core/platform.h | 9 ++++ src/platform/posix/posix_atomic.c | 76 +++++++++++++++++++++++++++++--- src/platform/posix/posix_impl.h | 12 ++++- src/platform/windows/win_impl.h | 4 ++ src/platform/windows/win_thread.c | 24 ++++++++++ src/supplemental/http/http_server.c | 88 ++++++++++++++++++++++--------------- 6 files changed, 169 insertions(+), 44 deletions(-) (limited to 'src') diff --git a/src/core/platform.h b/src/core/platform.h index b1d76899..b56f9a68 100644 --- a/src/core/platform.h +++ b/src/core/platform.h @@ -169,6 +169,15 @@ typedef struct nni_atomic_flag nni_atomic_flag; extern bool nni_atomic_flag_test_and_set(nni_atomic_flag *); extern void nni_atomic_flag_reset(nni_atomic_flag *); +// nni_atomic_bool is for boolean flags that need to be checked without +// changing their value. This might require a lock on some systems. +typedef struct nni_atomic_bool nni_atomic_bool; + +extern void nni_atomic_init_bool(nni_atomic_bool *); +extern void nni_atomic_set_bool(nni_atomic_bool *, bool); +extern bool nni_atomic_get_bool(nni_atomic_bool *); +extern bool nni_atomic_swap_bool(nni_atomic_bool *, bool); + typedef struct nni_atomic_u64 nni_atomic_u64; extern void nni_atomic_init64(nni_atomic_u64 *); diff --git a/src/platform/posix/posix_atomic.c b/src/platform/posix/posix_atomic.c index 56e2c370..b1183865 100644 --- a/src/platform/posix/posix_atomic.c +++ b/src/platform/posix/posix_atomic.c @@ -1,5 +1,5 @@ // -// Copyright 2018 Staysail Systems, Inc. +// Copyright 2019 Staysail Systems, Inc. // Copyright 2018 Capitar IT Group BV // // This software is supplied under the terms of the MIT License, a @@ -29,34 +29,61 @@ nni_atomic_flag_reset(nni_atomic_flag *f) atomic_flag_clear(&f->f); } +void +nni_atomic_set_bool(nni_atomic_bool *v, bool b) +{ + atomic_store(&v->v, b); +} + +bool +nni_atomic_get_bool(nni_atomic_bool *v) +{ + return (atomic_load(&v->v)); +} + +bool +nni_atomic_swap_bool(nni_atomic_bool *v, bool b) +{ + return (atomic_exchange(&v->v, b)); + +} + +void +nni_atomic_init_bool(nni_atomic_bool *v) +{ + atomic_init(&v->v, false); +} + void nni_atomic_add64(nni_atomic_u64 *v, uint64_t bump) { - (void) atomic_fetch_add_explicit(&v->v, bump, memory_order_relaxed); + (void) atomic_fetch_add_explicit( + &v->v, (uint_fast64_t) bump, memory_order_relaxed); } void nni_atomic_sub64(nni_atomic_u64 *v, uint64_t bump) { - (void) atomic_fetch_sub_explicit(&v->v, bump, memory_order_relaxed); + (void) atomic_fetch_sub_explicit( + &v->v, (uint_fast64_t) bump, memory_order_relaxed); } uint64_t nni_atomic_get64(nni_atomic_u64 *v) { - return (atomic_load(&v->v)); + return ((uint64_t) atomic_load(&v->v)); } void nni_atomic_set64(nni_atomic_u64 *v, uint64_t u) { - atomic_store(&v->v, u); + atomic_store(&v->v, (uint_fast64_t) u); } uint64_t nni_atomic_swap64(nni_atomic_u64 *v, uint64_t u) { - return (atomic_exchange(&v->v, u)); + return ((uint64_t) atomic_exchange(&v->v, (uint_fast64_t) u)); } void @@ -77,7 +104,7 @@ nni_atomic_dec64_nv(nni_atomic_u64 *v) uint64_t ov; // C11 atomics give the old rather than new value. - ov = atomic_fetch_sub(&v->v, 1); + ov = (uint64_t) atomic_fetch_sub(&v->v, 1); return (ov - 1); } @@ -106,6 +133,41 @@ nni_atomic_flag_reset(nni_atomic_flag *f) pthread_mutex_unlock(&plat_atomic_lock); } +void +nni_atomic_set_bool(nni_atomic_bool *b) +{ + pthread_mutex_lock(&plat_atomic_lock); + b->b = false; + pthread_mutex_unlock(&plat_atomic_lock); +} + +void +nni_atomic_get_bool(nni_atomic_bool *b) +{ + bool v; + pthread_mutex_lock(&plat_atomic_lock); + v = b->b; + pthread_mutex_unlock(&plat_atomic_lock); + return (v); +} + +void +nni_atomic_swap_bool(nni_atomic_bool *b, bool n) +{ + bool v; + pthread_mutex_lock(&plat_atomic_lock); + v = b->b; + b->b = n; + pthread_mutex_unlock(&plat_atomic_lock); + return (v); +} + +void +nni_atomic_initbool(nni_atomic_bool *b) +{ + b->b = false; +} + void nni_atomic_add64(nni_atomic_u64 *v, uint64_t bump) { diff --git a/src/platform/posix/posix_impl.h b/src/platform/posix/posix_impl.h index b07d268b..a1cb62c5 100644 --- a/src/platform/posix/posix_impl.h +++ b/src/platform/posix/posix_impl.h @@ -1,5 +1,5 @@ // -// Copyright 2018 Staysail Systems, Inc. +// Copyright 2019 Staysail Systems, Inc. // Copyright 2018 Capitar IT Group BV // // This software is supplied under the terms of the MIT License, a @@ -84,7 +84,11 @@ struct nni_atomic_flag { }; struct nni_atomic_u64 { - _Atomic unsigned long long v; + atomic_uint_fast64_t v; +}; + +struct nni_atomic_bool { + atomic_bool v; }; #else // NNG_HAVE_C11_ATOMIC @@ -92,6 +96,10 @@ struct nni_atomic_flag { bool f; }; +struct nni_atomic_bol { + bool b; +}; + struct nni_atomic_u64 { uint64_t v; }; diff --git a/src/platform/windows/win_impl.h b/src/platform/windows/win_impl.h index cd15804a..befcd185 100644 --- a/src/platform/windows/win_impl.h +++ b/src/platform/windows/win_impl.h @@ -50,6 +50,10 @@ struct nni_atomic_flag { unsigned f; }; +struct nni_atomic_bool { + LONG v; +}; + struct nni_atomic_u64 { LONGLONG v; }; diff --git a/src/platform/windows/win_thread.c b/src/platform/windows/win_thread.c index 076da9e4..763dcf7f 100644 --- a/src/platform/windows/win_thread.c +++ b/src/platform/windows/win_thread.c @@ -131,6 +131,30 @@ nni_atomic_flag_reset(nni_atomic_flag *f) InterlockedExchange(&f->f, 0); } +void +nni_atomic_set_bool(nni_atomic_bool *v, bool b) +{ + InterlockedExchange(&v->v, (LONG) b); +} + +bool +nni_atomic_get_bool(nni_atomic_bool *v) +{ + return ((bool) InterlockedAdd(&v->v, 0)); +} + +bool +nni_atomic_swap_bool(nni_atomic_bool *v, bool b) +{ + return ((bool) InterlockedExchange(&v->v, (LONG) b)); +} + +void +nni_atomic_init_bool(nni_atomic_bool *v) +{ + InterlockedExchange(&v->v, 0); +} + void nni_atomic_add64(nni_atomic_u64 *v, uint64_t bump) { diff --git a/src/supplemental/http/http_server.c b/src/supplemental/http/http_server.c index b927d8f3..06a93553 100644 --- a/src/supplemental/http/http_server.c +++ b/src/supplemental/http/http_server.c @@ -32,16 +32,17 @@ static nni_initializer http_server_initializer = { }; struct nng_http_handler { - nni_list_node node; - char * uri; - char * method; - char * host; - bool tree; - int refcnt; - size_t maxbody; - bool getbody; - void * data; - nni_cb dtor; + nni_list_node node; + char * uri; + char * method; + char * host; + bool tree; + nni_atomic_u64 ref; + nni_atomic_bool busy; + size_t maxbody; + bool getbody; + void * data; + nni_cb dtor; void (*cb)(nni_aio *); }; @@ -96,6 +97,9 @@ nni_http_handler_init( if ((h = NNI_ALLOC_STRUCT(h)) == NULL) { return (NNG_ENOMEM); } + nni_atomic_init64(&h->ref); + nni_atomic_inc64(&h->ref); + // Default for HTTP is /. But remap it to "" for ease of matching. if ((uri == NULL) || (strlen(uri) == 0) || (strcmp(uri, "/") == 0)) { uri = ""; @@ -111,17 +115,18 @@ nni_http_handler_init( h->dtor = NULL; h->host = NULL; h->tree = false; - h->refcnt = 0; h->maxbody = 1024 * 1024; // By default we accept up to 1MB of body h->getbody = true; *hp = h; return (0); } +// nni_http_handler_fini just drops the reference count, only destroying +// the handler if the reference drops to zero. void nni_http_handler_fini(nni_http_handler *h) { - if (h->refcnt != 0) { + if (nni_atomic_dec64_nv(&h->ref) != 0) { return; } if (h->dtor != NULL) { @@ -143,7 +148,7 @@ nni_http_handler_collect_body(nni_http_handler *h, bool want, size_t maxbody) int nni_http_handler_set_data(nni_http_handler *h, void *data, nni_cb dtor) { - if (h->refcnt != 0) { + if (nni_atomic_get_bool(&h->busy)) { return (NNG_EBUSY); } h->data = data; @@ -169,7 +174,7 @@ nni_http_handler_get_uri(nni_http_handler *h) int nni_http_handler_set_tree(nni_http_handler *h) { - if (h->refcnt != 0) { + if (nni_atomic_get_bool(&h->busy) != 0) { return (NNG_EBUSY); } h->tree = true; @@ -179,8 +184,9 @@ nni_http_handler_set_tree(nni_http_handler *h) int nni_http_handler_set_host(nni_http_handler *h, const char *host) { - char *duphost; - if (h->refcnt != 0) { + char *dup; + + if (nni_atomic_get_bool(&h->busy) != 0) { return (NNG_EBUSY); } if (host == NULL) { @@ -188,19 +194,20 @@ nni_http_handler_set_host(nni_http_handler *h, const char *host) h->host = NULL; return (0); } - if ((duphost = nni_strdup(host)) == NULL) { + if ((dup = nni_strdup(host)) == NULL) { return (NNG_ENOMEM); } nni_strfree(h->host); - h->host = duphost; + h->host = dup; return (0); } int nni_http_handler_set_method(nni_http_handler *h, const char *method) { - char *dupmeth; - if (h->refcnt != 0) { + char *dup; + + if (nni_atomic_get_bool(&h->busy) != 0) { return (NNG_EBUSY); } if (method == NULL) { @@ -208,11 +215,11 @@ nni_http_handler_set_method(nni_http_handler *h, const char *method) h->method = NULL; return (0); } - if ((dupmeth = nni_strdup(method)) == NULL) { + if ((dup = nni_strdup(method)) == NULL) { return (NNG_ENOMEM); } nni_strfree(h->method); - h->method = dupmeth; + h->method = dup; return (0); } @@ -650,7 +657,9 @@ finish: return; } nni_aio_set_data(sc->cbaio, 1, h); - h->refcnt++; + // Set a reference -- this because the callback may be running + // asynchronously even after it gets removed from the server. + nni_atomic_inc64(&h->ref); nni_mtx_unlock(&s->mtx); h->cb(sc->cbaio); } @@ -664,23 +673,25 @@ http_sconn_cbdone(void *arg) nni_http_handler *h; nni_http_server * s = sc->server; + // Get the handler. It may be set regardless of success or + // failure. Clear it, and drop our reference, since we're + // done with the handler for now. + h = nni_aio_get_data(aio, 1); + nni_aio_set_data(aio, 1, NULL); + + if (h != NULL) { + nni_http_handler_fini(h); + } + if (nni_aio_result(aio) != 0) { // Hard close, no further feedback. http_sconn_close(sc); return; } - h = nni_aio_get_data(aio, 1); res = nni_aio_get_output(aio, 0); - nni_mtx_lock(&s->mtx); - h->refcnt--; - if (h->refcnt == 0) { - nni_http_handler_fini(h); - } - nni_mtx_unlock(&s->mtx); - - // If its an upgrader, and they didn't give us back a response, + // If it's an upgrader, and they didn't give us back a response, // it means that they took over, and we should just discard // this session, without closing the underlying channel. if (sc->conn == NULL) { @@ -815,7 +826,6 @@ http_server_fini(nni_http_server *s) nng_stream_listener_free(s->listener); while ((h = nni_list_first(&s->handlers)) != NULL) { nni_list_remove(&s->handlers, h); - h->refcnt--; nni_http_handler_fini(h); } nni_mtx_unlock(&s->mtx); @@ -1155,8 +1165,14 @@ nni_http_server_add_handler(nni_http_server *s, nni_http_handler *h) return (NNG_EADDRINUSE); } } - h->refcnt = 1; nni_list_append(&s->handlers, h); + + // Note that we have borrowed the reference count on the handler. + // Thus we own it, and if the server is destroyed while we have it, + // then we must finalize it it too. We do mark it busy so + // that other settings cannot change. + nni_atomic_set_bool(&h->busy, true); + nni_mtx_unlock(&s->mtx); return (0); } @@ -1169,13 +1185,15 @@ nni_http_server_del_handler(nni_http_server *s, nni_http_handler *h) nni_mtx_lock(&s->mtx); NNI_LIST_FOREACH (&s->handlers, srch) { if (srch == h) { + // NB: We are giving the caller our reference + // on the handler. nni_list_remove(&s->handlers, h); - h->refcnt--; rv = 0; break; } } nni_mtx_unlock(&s->mtx); + return (rv); } -- cgit v1.2.3-70-g09d2