From d13b72b3887096f54d3570b723f2f518cf0e903b Mon Sep 17 00:00:00 2001 From: Nathan French Date: Fri, 19 May 2017 14:49:05 -0700 Subject: [PATCH 1/3] FIX : Socket leakage on error #6 Cleanup open file descriptors when bind_sockaddr fails. Thanks to @hyperblast for reporting this issue https://github.com/criticalstack/libevhtp/issues/6 --- evhtp.c | 63 ++++++++++++++++++++++++++++++++++++++++++--------------- 1 file changed, 47 insertions(+), 16 deletions(-) diff --git a/evhtp.c b/evhtp.c index 8941bfe..81338a1 100644 --- a/evhtp.c +++ b/evhtp.c @@ -3642,31 +3642,63 @@ evhtp_accept_socket(evhtp_t * htp, evutil_socket_t sock, int backlog) int evhtp_bind_sockaddr(evhtp_t * htp, struct sockaddr * sa, size_t sin_len, int backlog) { + + evutil_socket_t fd = -1; + int on = 1; + int error = 1; + + if (htp == NULL) { + return -1; + } + #ifndef WIN32 signal(SIGPIPE, SIG_IGN); #endif - evutil_socket_t fd; - int on = 1; - fd = socket(sa->sa_family, SOCK_STREAM, 0); - evhtp_errno_assert(fd != -1); + do { + if ((fd = socket(sa->sa_family, SOCK_STREAM, 0)) == -1) + { + return -1; + } - evutil_make_socket_closeonexec(fd); - evutil_make_socket_nonblocking(fd); - setsockopt(fd, SOL_SOCKET, SO_KEEPALIVE, (void *)&on, sizeof(on)); - setsockopt(fd, SOL_SOCKET, SO_REUSEADDR, (void *)&on, sizeof(on)); + evutil_make_socket_closeonexec(fd); + evutil_make_socket_nonblocking(fd); - if (sa->sa_family == AF_INET6) - { - int rc; + if (setsockopt(fd, SOL_SOCKET, SO_KEEPALIVE, (void *)&on, sizeof(on)) == -1) + { + break; + } - rc = setsockopt(fd, IPPROTO_IPV6, IPV6_V6ONLY, &on, sizeof(on)); - evhtp_errno_assert(rc != -1); - } + if (setsockopt(fd, SOL_SOCKET, SO_REUSEADDR, (void *)&on, sizeof(on)) == -1) + { + break; + } - if (bind(fd, sa, sin_len) == -1) + if (sa->sa_family == AF_INET6) + { + if (setsockopt(fd, IPPROTO_IPV6, IPV6_V6ONLY, &on, sizeof(on)) == -1) + { + break; + } + } + + if (bind(fd, sa, sin_len) == -1) + { + break; + } + + error = 0; + } while (0); + + + if (error == 1) { + if (fd != -1) + { + evutil_closesocket(fd); + } + return -1; } @@ -5009,4 +5041,3 @@ evhtp_request_status(evhtp_request_t * r) { return htparser_get_status(r->conn->parser); } - From 18f7ec06e33794f6979779bf4cbc68930aff69d8 Mon Sep 17 00:00:00 2001 From: Nathan French Date: Mon, 22 May 2017 10:10:24 -0700 Subject: [PATCH 2/3] issue #6: Socket leakage on acceot_socket Hold up the same error checking standards as its owner :) --- evhtp.c | 29 +++++++++++++++++++++-------- 1 file changed, 21 insertions(+), 8 deletions(-) diff --git a/evhtp.c b/evhtp.c index 81338a1..be46b3f 100644 --- a/evhtp.c +++ b/evhtp.c @@ -37,9 +37,6 @@ htp_log_connection(evhtp_connection_t * c) #endif - - - static int htp__request_parse_start_(htparser * p); static int htp__request_parse_host_(htparser * p, const char * data, size_t len); static int htp__request_parse_port_(htparser * p, const char * data, size_t len); @@ -3572,6 +3569,7 @@ evhtp_accept_socket(evhtp_t * htp, evutil_socket_t sock, int backlog) { int on = 1; int res = 0; + int err = 1; evhtp_assert(htp != NULL); @@ -3617,7 +3615,7 @@ evhtp_accept_socket(evhtp_t * htp, evutil_socket_t sock, int backlog) if (htp->server == NULL) { - return -1; + break; } #ifndef EVHTP_DISABLE_SSL @@ -3635,19 +3633,34 @@ evhtp_accept_socket(evhtp_t * htp, evutil_socket_t sock, int backlog) } } #endif + err = 0; } while (0); + + if (err == 1) + { + evutil_closesocket(sock); + + if (htp->server != NULL) + { + evhtp_safe_free(htp->server, evconnlistener_free); + } + + return -1; + } + + return res; } /* evhtp_accept_socket */ int evhtp_bind_sockaddr(evhtp_t * htp, struct sockaddr * sa, size_t sin_len, int backlog) { - - evutil_socket_t fd = -1; - int on = 1; + evutil_socket_t fd = -1; + int on = 1; int error = 1; - if (htp == NULL) { + if (htp == NULL) + { return -1; } From d0347dc4bab492910bc80276051eea31d39b23e9 Mon Sep 17 00:00:00 2001 From: Nathan French Date: Mon, 22 May 2017 14:11:01 -0700 Subject: [PATCH 3/3] Issue#6: make evhtp_accept_socket conform to api - evhtp_accept_socket's function definition states that 0 on success, -1 on error. This was not the case. Also evhtp_accept_socket should not close the file descriptor as it is passed as an argument, not an owner. There should be no negative effects of this change of the documentation for the function was used :D --- evhtp.c | 43 +++++++++++++++++++++---------------------- evhtp.h | 3 +++ 2 files changed, 24 insertions(+), 22 deletions(-) diff --git a/evhtp.c b/evhtp.c index be46b3f..4924b18 100644 --- a/evhtp.c +++ b/evhtp.c @@ -3568,12 +3568,9 @@ int evhtp_accept_socket(evhtp_t * htp, evutil_socket_t sock, int backlog) { int on = 1; - int res = 0; int err = 1; - evhtp_assert(htp != NULL); - - if (sock == -1) + if (htp == NULL || sock == -1) { return -1; } @@ -3582,7 +3579,7 @@ evhtp_accept_socket(evhtp_t * htp, evutil_socket_t sock, int backlog) #if defined SO_REUSEPORT if (htp->enable_reuseport) { - if ((res = setsockopt(sock, SOL_SOCKET, SO_REUSEPORT, (void *)&on, sizeof(on)) != 0)) + if (setsockopt(sock, SOL_SOCKET, SO_REUSEPORT, (void *)&on, sizeof(on)) == -1) { break; } @@ -3592,7 +3589,7 @@ evhtp_accept_socket(evhtp_t * htp, evutil_socket_t sock, int backlog) #if defined TCP_NODELAY if (htp->enable_nodelay == 1) { - if ((res = setsockopt(sock, IPPROTO_TCP, TCP_NODELAY, (void *)&on, sizeof(on))) != 0) + if (setsockopt(sock, IPPROTO_TCP, TCP_NODELAY, (void *)&on, sizeof(on)) == -1) { break; } @@ -3602,7 +3599,7 @@ evhtp_accept_socket(evhtp_t * htp, evutil_socket_t sock, int backlog) #if defined TCP_DEFER_ACCEPT if (htp->enable_defer_accept == 1) { - if ((res = setsockopt(sock, IPPROTO_TCP, TCP_DEFER_ACCEPT, (void *)&on, sizeof(on))) != 0) + if (setsockopt(sock, IPPROTO_TCP, TCP_DEFER_ACCEPT, (void *)&on, sizeof(on)) == -1) { break; } @@ -3638,8 +3635,6 @@ evhtp_accept_socket(evhtp_t * htp, evutil_socket_t sock, int backlog) if (err == 1) { - evutil_closesocket(sock); - if (htp->server != NULL) { evhtp_safe_free(htp->server, evconnlistener_free); @@ -3648,8 +3643,7 @@ evhtp_accept_socket(evhtp_t * htp, evutil_socket_t sock, int backlog) return -1; } - - return res; + return 0; } /* evhtp_accept_socket */ int @@ -3664,6 +3658,7 @@ evhtp_bind_sockaddr(evhtp_t * htp, struct sockaddr * sa, size_t sin_len, int bac return -1; } + /* XXX: API's should not set signals */ #ifndef WIN32 signal(SIGPIPE, SIG_IGN); #endif @@ -3715,26 +3710,32 @@ evhtp_bind_sockaddr(evhtp_t * htp, struct sockaddr * sa, size_t sin_len, int bac return -1; } - return evhtp_accept_socket(htp, fd, backlog); + if (evhtp_accept_socket(htp, fd, backlog) == -1) + { + /* accept_socket() does not close the descriptor + * on error, but this function does. + */ + evutil_closesocket(fd); + + return -1; + } + + return 0; } /* evhtp_bind_sockaddr */ int evhtp_bind_socket(evhtp_t * htp, const char * baddr, uint16_t port, int backlog) { #ifndef NO_SYS_UN - struct sockaddr_un sun; + struct sockaddr_un sun = { 0 }; #endif - struct sockaddr_in6 sin6; - struct sockaddr_in sin; struct sockaddr * sa; + struct sockaddr_in6 sin6 = { 0 }; + struct sockaddr_in sin = { 0 }; size_t sin_len; - memset(&sin, 0, sizeof(sin)); - if (!strncmp(baddr, "ipv6:", 5)) { - memset(&sin6, 0, sizeof(sin6)); - baddr += 5; sin_len = sizeof(struct sockaddr_in6); sin6.sin6_port = htons(port); @@ -3752,8 +3753,6 @@ evhtp_bind_socket(evhtp_t * htp, const char * baddr, uint16_t port, int backlog) return -1; } - memset(&sun, 0, sizeof(sun)); - sin_len = sizeof(struct sockaddr_un); sun.sun_family = AF_UNIX; @@ -3771,7 +3770,6 @@ evhtp_bind_socket(evhtp_t * htp, const char * baddr, uint16_t port, int backlog) } sin_len = sizeof(struct sockaddr_in); - sin.sin_family = AF_INET; sin.sin_port = htons(port); sin.sin_addr.s_addr = inet_addr(baddr); @@ -5041,6 +5039,7 @@ evhtp_make_request(evhtp_connection_t * c, evhtp_request_t * r, evhtp_headers_for_each(r->headers_out, htp__create_headers_, obuf); evbuffer_add_reference(obuf, "\r\n", 2, NULL, NULL); + if (evbuffer_get_length(r->buffer_out)) { evbuffer_add_buffer(obuf, r->buffer_out); diff --git a/evhtp.h b/evhtp.h index e0d7a58..b696edf 100644 --- a/evhtp.h +++ b/evhtp.h @@ -845,6 +845,9 @@ EVHTP_EXPORT void evhtp_unbind_socket(evhtp_t * htp); * @brief create the listener plus setup various options with an already-bound * socket. * + * @note Since the file descriptor is passed to the function, it will not + * attempt to close it if an error occurs. + * * @param htp * @param sock * @param backlog