From 2e03d3799c0fa1c005fbfd9b6acc40d2101e5c99 Mon Sep 17 00:00:00 2001 From: Willem Toorop Date: Tue, 30 Jan 2018 12:23:23 +0100 Subject: [PATCH 1/5] Memory leak on some TLS creation error cases --- src/stub.c | 2 ++ 1 file changed, 2 insertions(+) diff --git a/src/stub.c b/src/stub.c index 07f22fcd..a9ec8240 100644 --- a/src/stub.c +++ b/src/stub.c @@ -956,6 +956,7 @@ tls_create_object(getdns_dns_req *dnsreq, int fd, getdns_upstream *upstream) "%-40s : ERROR: Hostname Authentication not available from TLS library (check library version)\n", upstream->addr_str); upstream->tls_hs_state = GETDNS_HS_FAILED; + SSL_free(ssl); return NULL; } #endif @@ -973,6 +974,7 @@ tls_create_object(getdns_dns_req *dnsreq, int fd, getdns_upstream *upstream) DEBUG_STUB("%s %-35s: ERROR: No host name or pubkey pinset provided for TLS authentication\n", STUB_DEBUG_SETUP_TLS, __FUNC__); upstream->tls_hs_state = GETDNS_HS_FAILED; + SSL_free(ssl); return NULL; } } else { From 1f401f725377808b43a332159bb4389d3cbb60bc Mon Sep 17 00:00:00 2001 From: Willem Toorop Date: Tue, 30 Jan 2018 12:40:47 +0100 Subject: [PATCH 2/5] Do not return freed netreqs! --- src/general.c | 2 ++ 1 file changed, 2 insertions(+) diff --git a/src/general.c b/src/general.c index 80b40a2c..3e50124d 100644 --- a/src/general.c +++ b/src/general.c @@ -697,6 +697,8 @@ getdns_general_ns(getdns_context *context, getdns_eventloop *loop, /* clean up the request */ _getdns_context_clear_outbound_request(req); _getdns_dns_req_free(req); + if (return_netreq_p) + *return_netreq_p = NULL; return r; } return GETDNS_RETURN_GOOD; From 97b056c3555b24164a8b08a4e9c28d0705868c3c Mon Sep 17 00:00:00 2001 From: Willem Toorop Date: Tue, 30 Jan 2018 15:21:46 +0100 Subject: [PATCH 3/5] Prevent erred TCP connection to be rescheduled ... for reading (or writing) when an reply comes in. Thanks Maddie! --- src/server.c | 121 +++++++++++++++++++++++++++++---------------------- 1 file changed, 68 insertions(+), 53 deletions(-) diff --git a/src/server.c b/src/server.c index 1f5f218d..ce643aee 100644 --- a/src/server.c +++ b/src/server.c @@ -127,21 +127,25 @@ static void tcp_connection_destroy(tcp_connection *conn) tcp_to_write *cur, *next; - if (!(mf = &conn->super.l->set->context->mf)) - return; - + mf = &conn->super.l->set->context->mf; if (getdns_context_get_eventloop(conn->super.l->set->context, &loop)) return; - if (conn->event.read_cb||conn->event.write_cb||conn->event.timeout_cb) + if (conn->event.ev) loop->vmt->clear(loop, &conn->event); - if (conn->fd >= 0) + if (conn->event.read_cb||conn->event.write_cb||conn->event.timeout_cb) { + conn->event.read_cb = conn->event.write_cb = + conn->event.timeout_cb = NULL; + } + if (conn->fd >= 0) { (void) _getdns_closesocket(conn->fd); - + conn->fd = -1; + } if (conn->read_buf) { GETDNS_FREE(*mf, conn->read_buf); - conn->read_buf = NULL; + conn->read_buf = conn->read_pos = NULL; + conn->to_read = 0; } if ((cur = conn->to_write)) { while (cur) { @@ -198,15 +202,14 @@ static void tcp_write_cb(void *userarg) (const void *)&to_write->write_buf[to_write->written], to_write->write_buf_len - to_write->written, 0)) == -1) { - if (_getdns_socketerror_wants_retry()) - return; - - DEBUG_SERVER("I/O error from send(): %s\n", - _getdns_errnostr()); + if (conn->fd != -1) { + if (_getdns_socketerror_wants_retry()) + return; + DEBUG_SERVER("I/O error from send(): %s\n", + _getdns_errnostr()); + } /* IO error, close connection */ - conn->event.read_cb = conn->event.write_cb = - conn->event.timeout_cb = NULL; tcp_connection_destroy(conn); return; } @@ -317,6 +320,7 @@ getdns_reply( tcp_to_write **to_write_p; tcp_to_write *to_write; + loop->vmt->clear(loop, &conn->event); if (conn->fd == -1) { if (conn->to_answer > 0) --conn->to_answer; @@ -324,8 +328,10 @@ getdns_reply( return GETDNS_RETURN_GOOD; } if (!(to_write = (tcp_to_write *)GETDNS_XMALLOC( - *mf, uint8_t, sizeof(tcp_to_write) + len + 2))) + *mf, uint8_t, sizeof(tcp_to_write) + len + 2))) { + tcp_connection_destroy(conn); return GETDNS_RETURN_MEMORY_ERROR; + } to_write->write_buf_len = len + 2; to_write->write_buf[0] = (len >> 8) & 0xFF; @@ -341,7 +347,6 @@ getdns_reply( ; /* pass */ *to_write_p = to_write; - loop->vmt->clear(loop, &conn->event); conn->event.write_cb = tcp_write_cb; if (conn->to_answer > 0) conn->to_answer--; @@ -373,18 +378,17 @@ static void tcp_read_cb(void *userarg) /* Reset tcp_connection idle timeout */ loop->vmt->clear(loop, &conn->event); - (void) loop->vmt->schedule(loop, conn->fd, - DOWNSTREAM_IDLE_TIMEOUT, &conn->event); - - if ((bytes_read = recv(conn->fd, + if (conn->fd == -1 || + (bytes_read = recv(conn->fd, (void *)conn->read_pos, conn->to_read, 0)) < 0) { - if (_getdns_socketerror_wants_retry()) - return; /* Come back to do the read later */ - - /* IO error, close connection */ - DEBUG_SERVER("I/O error from recv(): %s\n", - _getdns_errnostr()); + if (conn->fd != -1) { + if (_getdns_socketerror_wants_retry()) + return; /* Come back to do the read later */ + /* IO error, close connection */ + DEBUG_SERVER("I/O error from recv(): %s\n", + _getdns_errnostr()); + } tcp_connection_destroy(conn); return; } @@ -398,9 +402,9 @@ static void tcp_read_cb(void *userarg) conn->to_read -= bytes_read; conn->read_pos += bytes_read; if (conn->to_read) - return; /* More to read */ + ; /* Schedule for more reading */ - if (conn->read_pos - conn->read_buf == 2) { + else if (conn->read_pos - conn->read_buf == 2) { /* read length of dns msg to read */ conn->to_read = (conn->read_buf[0] << 8) | conn->read_buf[1]; if (conn->to_read > conn->read_buf_len) { @@ -420,47 +424,49 @@ static void tcp_read_cb(void *userarg) return; } conn->read_pos = conn->read_buf; - return; /* Read DNS message */ - } - if ((r = getdns_wire2msg_dict(conn->read_buf, - (conn->read_pos - conn->read_buf), &request_dict))) - ; /* FROMERR on input, ignore */ + ; /* Schedule for more reading */ - else { - conn->to_answer++; + } else { + /* Ready for reading a new packet */ - /* TODO: wish list item: - * (void) getdns_dict_set_int64( - * request_dict, "request_id", intptr_t)conn); - */ - /* Call request handler */ - conn->super.l->set->handler( - conn->super.l->set->context, GETDNS_CALLBACK_COMPLETE, - request_dict, conn->super.l->set->userarg, (intptr_t)conn); + if (!(r = getdns_wire2msg_dict(conn->read_buf, + (conn->read_pos - conn->read_buf), &request_dict))) { + conn->to_answer++; + + /* TODO: wish list item: + * (void) getdns_dict_set_int64( + * request_dict, "request_id", intptr_t)conn); + */ + /* Call request handler */ + conn->super.l->set->handler( + conn->super.l->set->context, + GETDNS_CALLBACK_COMPLETE, request_dict, + conn->super.l->set->userarg, (intptr_t)conn); + } conn->read_pos = conn->read_buf; conn->to_read = 2; - return; /* Read more requests */ + ; /* Schedule for more reading */ } - conn->read_pos = conn->read_buf; - conn->to_read = 2; /* Read more requests */ + (void) loop->vmt->schedule(loop, conn->fd, + DOWNSTREAM_IDLE_TIMEOUT, &conn->event); } static void tcp_timeout_cb(void *userarg) { tcp_connection *conn = (tcp_connection *)userarg; + getdns_eventloop *loop; assert(userarg); - if (conn->to_answer) { - getdns_eventloop *loop; + if (getdns_context_get_eventloop( + conn->super.l->set->context, &loop)) + return; - if (getdns_context_get_eventloop( - conn->super.l->set->context, &loop)) - return; + loop->vmt->clear(loop, &conn->event); - loop->vmt->clear(loop, &conn->event); + if (conn->to_answer && conn->fd >= 0) { (void) loop->vmt->schedule(loop, conn->fd, DOWNSTREAM_IDLE_TIMEOUT, &conn->event); @@ -737,8 +743,17 @@ static void remove_listeners(listen_set *set) conn_p = (tcp_connection **)&l->connections; while (*conn_p) { + tcp_connection *prev_conn_p = *conn_p; + + loop->vmt->clear(loop, &(*conn_p)->event); tcp_connection_destroy(*conn_p); - if (*conn_p && (*conn_p)->to_answer > 0) + /* tcp_connection_destroy() updates the pointer to the + * connection. For the first connection this is + * l->connections. When the connection is not actually + * destroyed, the value of *conn_p thus remains the + * same. When it is destroyed it is updated. + */ + if (*conn_p == prev_conn_p) conn_p = (tcp_connection **) &(*conn_p)->super.next; } From 9bc98272a123faecb2d915743389c5a20d5e3d0e Mon Sep 17 00:00:00 2001 From: Willem Toorop Date: Wed, 31 Jan 2018 14:33:31 +0100 Subject: [PATCH 4/5] Fixing the fixes --- src/server.c | 71 ++++++++++++++++++++++++++++++++++++++++++++-------- 1 file changed, 60 insertions(+), 11 deletions(-) diff --git a/src/server.c b/src/server.c index ce643aee..bf3e86bb 100644 --- a/src/server.c +++ b/src/server.c @@ -34,6 +34,10 @@ #include #endif +#if defined(HAVE_FCNTL) +#include +#endif + #include "getdns/getdns_extra.h" #include "context.h" #include "types-internal.h" @@ -118,6 +122,25 @@ typedef struct tcp_connection { size_t to_answer; } tcp_connection; +/** best effort to set nonblocking */ +static void +getdns_sock_nonblock(int sockfd) +{ +#if defined(HAVE_FCNTL) + int flag; + if((flag = fcntl(sockfd, F_GETFL)) != -1) { + flag |= O_NONBLOCK; + if(fcntl(sockfd, F_SETFL, flag) == -1) { + /* ignore error, continue blockingly */ + } + } +#elif defined(HAVE_IOCTLSOCKET) + unsigned long on = 1; + if(ioctlsocket(sockfd, FIONBIO, &on) != 0) { + /* ignore error, continue blockingly */ + } +#endif +} static void free_listen_set_when_done(listen_set *set); static void tcp_connection_destroy(tcp_connection *conn) @@ -203,9 +226,11 @@ static void tcp_write_cb(void *userarg) to_write->write_buf_len - to_write->written, 0)) == -1) { if (conn->fd != -1) { - if (_getdns_socketerror_wants_retry()) + if (_getdns_socketerror_wants_retry()) { + (void) loop->vmt->schedule(loop, conn->fd, + DOWNSTREAM_IDLE_TIMEOUT, &conn->event); return; - + } DEBUG_SERVER("I/O error from send(): %s\n", _getdns_errnostr()); } @@ -340,19 +365,27 @@ getdns_reply( to_write->next = NULL; (void) memcpy(to_write->write_buf + 2, buf, len); - /* Appen to_write to conn->to_write list */ + /* Append to_write to conn->to_write list */ for ( to_write_p = &conn->to_write ; *to_write_p ; to_write_p = &(*to_write_p)->next) ; /* pass */ *to_write_p = to_write; - conn->event.write_cb = tcp_write_cb; if (conn->to_answer > 0) conn->to_answer--; - (void) loop->vmt->schedule(loop, - conn->fd, DOWNSTREAM_IDLE_TIMEOUT, - &conn->event); + + /* When event is scheduled, and doesn't have tcp_write_cb: + * reschedule. + */ + if (conn->event.write_cb == NULL) { + if (conn->event.ev) + loop->vmt->clear(loop, &conn->event); + conn->event.write_cb = tcp_write_cb; + (void) loop->vmt->schedule(loop, + conn->fd, DOWNSTREAM_IDLE_TIMEOUT, + &conn->event); + } } /* TODO: other transport types */ @@ -382,9 +415,11 @@ static void tcp_read_cb(void *userarg) (bytes_read = recv(conn->fd, (void *)conn->read_pos, conn->to_read, 0)) < 0) { if (conn->fd != -1) { - if (_getdns_socketerror_wants_retry()) + if (_getdns_socketerror_wants_retry()) { + (void) loop->vmt->schedule(loop, conn->fd, + DOWNSTREAM_IDLE_TIMEOUT, &conn->event); return; /* Come back to do the read later */ - + } /* IO error, close connection */ DEBUG_SERVER("I/O error from recv(): %s\n", _getdns_errnostr()); @@ -439,18 +474,29 @@ static void tcp_read_cb(void *userarg) * request_dict, "request_id", intptr_t)conn); */ /* Call request handler */ + + conn->to_answer += 1; /* conn removal protection */ conn->super.l->set->handler( conn->super.l->set->context, GETDNS_CALLBACK_COMPLETE, request_dict, conn->super.l->set->userarg, (intptr_t)conn); + conn->to_answer -= 1; /* conn removal protection */ + + if (conn->fd == -1) { + tcp_connection_destroy(conn); + return; + } } conn->read_pos = conn->read_buf; conn->to_read = 2; ; /* Schedule for more reading */ } /* Read more requests */ - (void) loop->vmt->schedule(loop, conn->fd, - DOWNSTREAM_IDLE_TIMEOUT, &conn->event); + if (!conn->event.ev) { /* event not scheduled */ + conn->event.write_cb = conn->to_write ? tcp_write_cb : NULL; + (void) loop->vmt->schedule(loop, conn->fd, + DOWNSTREAM_IDLE_TIMEOUT, &conn->event); + } } static void tcp_timeout_cb(void *userarg) @@ -514,8 +560,10 @@ static void tcp_accept_cb(void *userarg) GETDNS_FREE(*mf, conn); return; } + getdns_sock_nonblock(conn->fd); if (!(conn->read_buf = malloc(DNS_REQUEST_SZ))) { /* Memory error */ + (void) _getdns_closesocket(conn->fd); GETDNS_FREE(*mf, conn); return; } @@ -531,6 +579,7 @@ static void tcp_accept_cb(void *userarg) if (!_getdns_rbtree_insert( &l->set->connections_set, &conn->super.super)) { /* Memory error */ + (void) _getdns_closesocket(conn->fd); GETDNS_FREE(*mf, conn); return; } From ec8b8ba903aa854e367f90b26a9480b2d2abf897 Mon Sep 17 00:00:00 2001 From: Willem Toorop Date: Wed, 31 Jan 2018 14:41:13 +0100 Subject: [PATCH 5/5] One more fixing the fixes fix that slipped through --- src/server.c | 1 - 1 file changed, 1 deletion(-) diff --git a/src/server.c b/src/server.c index bf3e86bb..59497bed 100644 --- a/src/server.c +++ b/src/server.c @@ -345,7 +345,6 @@ getdns_reply( tcp_to_write **to_write_p; tcp_to_write *to_write; - loop->vmt->clear(loop, &conn->event); if (conn->fd == -1) { if (conn->to_answer > 0) --conn->to_answer;