From a34e4338ce76aa0a06f6fcab8eb3424ab70cf16b Mon Sep 17 00:00:00 2001 From: Willem Toorop Date: Fri, 9 Jun 2017 08:49:52 +0200 Subject: [PATCH 1/6] Find out what went wrong --- .travis.yml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.travis.yml b/.travis.yml index 33a6c780..98b98d95 100644 --- a/.travis.yml +++ b/.travis.yml @@ -17,4 +17,4 @@ addons: script: - mkdir tests - cd tests - - ../src/test/tpkg/run-parallel.sh + - ../src/test/tpkg/run-all.sh From e00100b388f009be14122c27877417d9057665bc Mon Sep 17 00:00:00 2001 From: Willem Toorop Date: Fri, 9 Jun 2017 11:24:51 +0200 Subject: [PATCH 2/6] s/recieve/receive/ --- src/stub.c | 2 +- src/test/check_getdns_transport.c | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/src/stub.c b/src/stub.c index 2fa121f1..ee77c85c 100644 --- a/src/stub.c +++ b/src/stub.c @@ -362,7 +362,7 @@ process_keepalive( /* Use server sent value unless the client specified a shorter one. Convert to ms first (wire value has units of 100ms) */ uint64_t server_keepalive = ((uint64_t)gldns_read_uint16(position))*100; - DEBUG_STUB("%s %-35s: FD: %d Server Keepalive recieved: %d ms\n", + DEBUG_STUB("%s %-35s: FD: %d Server Keepalive received: %d ms\n", STUB_DEBUG_READ, __FUNC__, upstream->fd, (int)server_keepalive); if (netreq->owner->context->idle_timeout < server_keepalive) diff --git a/src/test/check_getdns_transport.c b/src/test/check_getdns_transport.c index bf457257..300abc4d 100644 --- a/src/test/check_getdns_transport.c +++ b/src/test/check_getdns_transport.c @@ -529,7 +529,7 @@ getdns_transport_suite(void) { /* Note that the exact number of messages received depends on if a trust * anchor is configured so these tests just check that no messages are - * received on the wrong transport and at least one is recieved on the + * received on the wrong transport and at least one is received on the * expected transport */ /* Positive test cases */ From e2be41d3528df8bc5de81195c8f8b2d379aeed8b Mon Sep 17 00:00:00 2001 From: Willem Toorop Date: Wed, 14 Jun 2017 15:36:53 +0200 Subject: [PATCH 3/6] Don't segfault on IPv6 unavailability Resolved issue #306? Review needed! Shoud upstream_failed cancel all the netreqs? --- src/stub.c | 54 ++++++++++++++++++++++++++++++++++++++++++++---------- 1 file changed, 44 insertions(+), 10 deletions(-) diff --git a/src/stub.c b/src/stub.c index ee77c85c..2514cf7f 100644 --- a/src/stub.c +++ b/src/stub.c @@ -565,14 +565,16 @@ upstream_failed(getdns_upstream *upstream, int during_setup) } else { upstream->conn_shutdowns++; /* [TLS1]TODO: Re-try these queries if possible.*/ + /* getdns_network_req *netreq; while (upstream->netreq_by_query_id.count) { netreq = (getdns_network_req *) _getdns_rbtree_first(&upstream->netreq_by_query_id); stub_cleanup(netreq); - _getdns_netreq_change_state(netreq, NET_REQ_FINISHED); + _getdns_netreq_change_state(netreq, NET_REQ_ERRORED); _getdns_check_dns_req_complete(netreq->owner); } + */ } upstream->conn_state = GETDNS_CONN_TEARDOWN; @@ -778,6 +780,8 @@ stub_tcp_write(int fd, getdns_tcp_state *tcp, getdns_network_req *netreq) } while (!_getdns_rbtree_insert( &netreq->upstream->netreq_by_query_id, &netreq->node)); + netreq->query_id = query_id; + GLDNS_ID_SET(netreq->query, query_id); if (netreq->opt) { _getdns_network_req_clear_upstream_options(netreq); @@ -836,8 +840,13 @@ stub_tcp_write(int fd, getdns_tcp_state *tcp, getdns_network_req *netreq) return STUB_TCP_WOULDBLOCK; - } else if (written == -1) + } else if (written == -1) { + DEBUG_STUB("%s %-35s: MSG: %p error while writing to TCP socket:" + " %s\n", STUB_DEBUG_WRITE, __FUNC__, (void*)netreq + , strerror(errno)); + return STUB_TCP_ERROR; + } /* We were able to write everything! Start reading. */ return (int) query_id; @@ -856,8 +865,13 @@ stub_tcp_write(int fd, getdns_tcp_state *tcp, getdns_network_req *netreq) if (written == -1) { if (_getdns_EWOULDBLOCK) return STUB_TCP_WOULDBLOCK; - else + else { + DEBUG_STUB("%s %-35s: MSG: %p error while writing to TCP socket:" + " %s\n", STUB_DEBUG_WRITE, __FUNC__, (void*)netreq + , strerror(errno)); + return STUB_TCP_ERROR; + } } tcp->written += written; if (tcp->written < tcp->write_buf_len) @@ -1480,6 +1494,7 @@ stub_udp_write_cb(void *userarg) getdns_network_req *netreq = (getdns_network_req *)userarg; getdns_dns_req *dnsreq = netreq->owner; size_t pkt_len; + ssize_t written; DEBUG_STUB("%s %-35s: MSG: %p \n", STUB_DEBUG_WRITE, __FUNC__, (void *)netreq); @@ -1504,15 +1519,34 @@ stub_udp_write_cb(void *userarg) return; /* too many upstream options */ } pkt_len = _getdns_network_req_add_tsig(netreq); - if ((ssize_t)pkt_len != sendto( + if ((ssize_t)pkt_len != (written = sendto( netreq->fd, (const void *)netreq->query, pkt_len, 0, (struct sockaddr *)&netreq->upstream->addr, - netreq->upstream->addr_len)) { -#ifdef USE_WINSOCK - closesocket(netreq->fd); -#else - close(netreq->fd); + netreq->upstream->addr_len))) { + +#if defined(STUB_DEBUG) && STUB_DEBUG + if (written == -1) + DEBUG_STUB( "%s %-35s: MSG: %p error: %s\n" + , STUB_DEBUG_WRITE, __FUNC__, (void *)netreq + , strerror(errno)); + else + DEBUG_STUB( "%s %-35s: MSG: %p returned: %d, expeced: %d\n" + , STUB_DEBUG_WRITE, __FUNC__, (void *)netreq + , (int)written, (int)pkt_len); #endif + stub_cleanup(netreq); + _getdns_netreq_change_state(netreq, NET_REQ_ERRORED); + /* Handle upstream*/ + if (netreq->fd >= 0) { +#ifdef USE_WINSOCK + closesocket(netreq->fd); +#else + close(netreq->fd); +#endif + stub_next_upstream(netreq); + } + netreq->debug_end_time = _getdns_get_time_as_uintt64(); + _getdns_check_dns_req_complete(netreq->owner); return; } GETDNS_SCHEDULE_EVENT(dnsreq->loop, netreq->fd, @@ -1709,7 +1743,7 @@ upstream_write_cb(void *userarg) #endif if (fallback_on_write(netreq) == STUB_TCP_ERROR) { /* TODO: Need new state to report transport unavailable*/ - _getdns_netreq_change_state(netreq, NET_REQ_FINISHED); + _getdns_netreq_change_state(netreq, NET_REQ_ERRORED); _getdns_check_dns_req_complete(netreq->owner); } return; From d9158e639b3df09ed58111216a5a30c55f5c72b9 Mon Sep 17 00:00:00 2001 From: Willem Toorop Date: Thu, 15 Jun 2017 17:21:05 +0200 Subject: [PATCH 4/6] Clear netreq_by_query_id on upstream failure But don't error the specific netreq then! --- src/stub.c | 13 ++++++++----- 1 file changed, 8 insertions(+), 5 deletions(-) diff --git a/src/stub.c b/src/stub.c index 2514cf7f..8e045388 100644 --- a/src/stub.c +++ b/src/stub.c @@ -565,7 +565,6 @@ upstream_failed(getdns_upstream *upstream, int during_setup) } else { upstream->conn_shutdowns++; /* [TLS1]TODO: Re-try these queries if possible.*/ - /* getdns_network_req *netreq; while (upstream->netreq_by_query_id.count) { netreq = (getdns_network_req *) @@ -574,7 +573,6 @@ upstream_failed(getdns_upstream *upstream, int during_setup) _getdns_netreq_change_state(netreq, NET_REQ_ERRORED); _getdns_check_dns_req_complete(netreq->owner); } - */ } upstream->conn_state = GETDNS_CONN_TEARDOWN; @@ -781,8 +779,8 @@ stub_tcp_write(int fd, getdns_tcp_state *tcp, getdns_network_req *netreq) &netreq->upstream->netreq_by_query_id, &netreq->node)); netreq->query_id = query_id; - GLDNS_ID_SET(netreq->query, query_id); + if (netreq->opt) { _getdns_network_req_clear_upstream_options(netreq); /* no limits on the max udp payload size with tcp */ @@ -1282,7 +1280,9 @@ stub_tls_write(getdns_upstream *upstream, getdns_tcp_state *tcp, } while (!_getdns_rbtree_insert( &netreq->upstream->netreq_by_query_id, &netreq->node)); + netreq->query_id = query_id; GLDNS_ID_SET(netreq->query, query_id); + /* TODO: Review if more EDNS0 handling can be centralised.*/ if (netreq->opt) { _getdns_network_req_clear_upstream_options(netreq); @@ -1741,7 +1741,8 @@ upstream_write_cb(void *userarg) STUB_DEBUG_DAEMON, upstream->addr_str, (upstream->transport == GETDNS_TRANSPORT_TLS ? "TLS" : "TCP")); #endif - if (fallback_on_write(netreq) == STUB_TCP_ERROR) { + if (q != STUB_TCP_ERROR && + fallback_on_write(netreq) == STUB_TCP_ERROR) { /* TODO: Need new state to report transport unavailable*/ _getdns_netreq_change_state(netreq, NET_REQ_ERRORED); _getdns_check_dns_req_complete(netreq->owner); @@ -1760,7 +1761,7 @@ upstream_write_cb(void *userarg) /* Need this because auth status is reset on connection close */ netreq->debug_tls_auth_status = netreq->upstream->tls_auth_state; upstream->queries_sent++; - netreq->query_id = (uint16_t) q; + /* Unqueue the netreq from the write_queue */ if (!(upstream->write_queue = netreq->write_queue_tail)) { upstream->write_queue_last = NULL; @@ -2090,6 +2091,8 @@ upstream_find_for_netreq(getdns_network_req *netreq) netreq->transport_current = i; netreq->upstream = upstream; netreq->keepalive_sent = 0; + + DEBUG_STUB("%s %-35s: MSG: %p found upstream %p with transport %d, fd: %d\n", STUB_DEBUG_SCHEDULE, __FUNC__, (void*)netreq, (void *)upstream, (int)netreq->transports[i], fd); return fd; } /* Handle better, will give generic error*/ From 1d874378541e55dcf2d94b5f4b705ff2ffd87c4b Mon Sep 17 00:00:00 2001 From: Willem Toorop Date: Thu, 15 Jun 2017 21:15:00 +0200 Subject: [PATCH 5/6] ERROR all outstanding netreqs whith a failed statefull upstream Remove the currently processed netreq first, so it can be retries with another upstream/transport. We MUST add netreq to the netreqs_by_query_id map even before we write to it, to have a reliable store of taken query ids. --- src/stub.c | 24 +++++++++++++----------- 1 file changed, 13 insertions(+), 11 deletions(-) diff --git a/src/stub.c b/src/stub.c index 8e045388..564a3415 100644 --- a/src/stub.c +++ b/src/stub.c @@ -540,6 +540,8 @@ stub_cleanup(getdns_network_req *netreq) static void upstream_failed(getdns_upstream *upstream, int during_setup) { + getdns_network_req *netreq; + DEBUG_STUB("%s %-35s: FD: %d Failure during connection setup = %d\n", STUB_DEBUG_CLEANUP, __FUNC__, upstream->fd, during_setup); /* Fallback code should take care of queue queries and then close conn @@ -565,16 +567,14 @@ upstream_failed(getdns_upstream *upstream, int during_setup) } else { upstream->conn_shutdowns++; /* [TLS1]TODO: Re-try these queries if possible.*/ - getdns_network_req *netreq; - while (upstream->netreq_by_query_id.count) { - netreq = (getdns_network_req *) - _getdns_rbtree_first(&upstream->netreq_by_query_id); - stub_cleanup(netreq); - _getdns_netreq_change_state(netreq, NET_REQ_ERRORED); - _getdns_check_dns_req_complete(netreq->owner); - } } - + while (upstream->netreq_by_query_id.count) { + netreq = (getdns_network_req *) + _getdns_rbtree_first(&upstream->netreq_by_query_id); + stub_cleanup(netreq); + _getdns_netreq_change_state(netreq, NET_REQ_ERRORED); + _getdns_check_dns_req_complete(netreq->owner); + } upstream->conn_state = GETDNS_CONN_TEARDOWN; } @@ -1723,6 +1723,7 @@ upstream_write_cb(void *userarg) */ case STUB_TCP_WOULDBLOCK: return; + case STUB_OUT_OF_OPTIONS: case STUB_TCP_ERROR: /* New problem with the TCP connection itself. Need to fallback.*/ /* Fall through */ @@ -1730,6 +1731,8 @@ upstream_write_cb(void *userarg) /* Could not complete the set up. Need to fallback.*/ DEBUG_STUB("%s %-35s: Upstream: %p ERROR = %d\n", STUB_DEBUG_WRITE, __FUNC__, (void*)userarg, q); + (void) _getdns_rbtree_delete( + &upstream->netreq_by_query_id, (void *)(intptr_t)netreq->query_id); upstream_failed(upstream, (q == STUB_TCP_ERROR ? 0:1)); /* Fall through */ case STUB_CONN_GONE: @@ -1741,8 +1744,7 @@ upstream_write_cb(void *userarg) STUB_DEBUG_DAEMON, upstream->addr_str, (upstream->transport == GETDNS_TRANSPORT_TLS ? "TLS" : "TCP")); #endif - if (q != STUB_TCP_ERROR && - fallback_on_write(netreq) == STUB_TCP_ERROR) { + if (fallback_on_write(netreq) == STUB_TCP_ERROR) { /* TODO: Need new state to report transport unavailable*/ _getdns_netreq_change_state(netreq, NET_REQ_ERRORED); _getdns_check_dns_req_complete(netreq->owner); From 9a273cf14476eb9e833a0ecbc236c271eb4947c6 Mon Sep 17 00:00:00 2001 From: Willem Toorop Date: Thu, 15 Jun 2017 21:24:40 +0200 Subject: [PATCH 6/6] Get rid of superfluous struct member query_id --- src/mdns.c | 5 ++--- src/request-internal.c | 1 - src/stub.c | 17 ++++++----------- src/types-internal.h | 1 - 4 files changed, 8 insertions(+), 16 deletions(-) diff --git a/src/mdns.c b/src/mdns.c index 28de951f..56ce7013 100644 --- a/src/mdns.c +++ b/src/mdns.c @@ -1746,7 +1746,7 @@ mdns_udp_read_cb(void *userarg) if (read < GLDNS_HEADER_SIZE) return; /* Not DNS */ - if (GLDNS_ID_WIRE(netreq->response) != netreq->query_id) + if (GLDNS_ID_WIRE(netreq->response) != GLDNS_ID_WIRE(netreq->query)) return; /* Cache poisoning attempt ;) */ // TODO: check whether EDNS server cookies are required for MDNS @@ -1788,8 +1788,7 @@ mdns_udp_write_cb(void *userarg) netreq->debug_start_time = _getdns_get_time_as_uintt64(); netreq->debug_udp = 1; - netreq->query_id = (uint16_t) arc4random(); - GLDNS_ID_SET(netreq->query, netreq->query_id); + GLDNS_ID_SET(netreq->query, (uint16_t) arc4random()); /* do we need to handle options valid in the MDNS context? */ diff --git a/src/request-internal.c b/src/request-internal.c index d91563f4..15f97179 100644 --- a/src/request-internal.c +++ b/src/request-internal.c @@ -125,7 +125,6 @@ netreq_reset(getdns_network_req *net_req) _getdns_netreq_change_state(net_req, NET_REQ_NOT_SENT); net_req->dnssec_status = GETDNS_DNSSEC_INDETERMINATE; net_req->tsig_status = GETDNS_DNSSEC_INDETERMINATE; - net_req->query_id = 0; net_req->response_len = 0; /* Some fields to record info for return_call_reporting */ net_req->debug_start_time = 0; diff --git a/src/stub.c b/src/stub.c index 564a3415..f900d35c 100644 --- a/src/stub.c +++ b/src/stub.c @@ -505,7 +505,6 @@ stub_cleanup(getdns_network_req *netreq) getdns_dns_req *dnsreq = netreq->owner; getdns_network_req *r, *prev_r; getdns_upstream *upstream; - intptr_t query_id_intptr; GETDNS_CLEAR_EVENT(dnsreq->loop, &netreq->event); @@ -514,9 +513,8 @@ stub_cleanup(getdns_network_req *netreq) return; /* Delete from upstream->netreq_by_query_id (if present) */ - query_id_intptr = (intptr_t)netreq->query_id; - (void) _getdns_rbtree_delete( - &upstream->netreq_by_query_id, (void *)query_id_intptr); + (void) _getdns_rbtree_delete(&upstream->netreq_by_query_id, + (void *)(intptr_t)GLDNS_ID_WIRE(netreq->query)); /* Delete from upstream->write_queue (if present) */ for (prev_r = NULL, r = upstream->write_queue; r; @@ -778,7 +776,6 @@ stub_tcp_write(int fd, getdns_tcp_state *tcp, getdns_network_req *netreq) } while (!_getdns_rbtree_insert( &netreq->upstream->netreq_by_query_id, &netreq->node)); - netreq->query_id = query_id; GLDNS_ID_SET(netreq->query, query_id); if (netreq->opt) { @@ -1280,7 +1277,6 @@ stub_tls_write(getdns_upstream *upstream, getdns_tcp_state *tcp, } while (!_getdns_rbtree_insert( &netreq->upstream->netreq_by_query_id, &netreq->node)); - netreq->query_id = query_id; GLDNS_ID_SET(netreq->query, query_id); /* TODO: Review if more EDNS0 handling can be centralised.*/ @@ -1430,7 +1426,7 @@ stub_udp_read_cb(void *userarg) if (read < GLDNS_HEADER_SIZE) return; /* Not DNS */ - if (GLDNS_ID_WIRE(netreq->response) != netreq->query_id) + if (GLDNS_ID_WIRE(netreq->response) != GLDNS_ID_WIRE(netreq->query)) return; /* Cache poisoning attempt ;) */ if (netreq->owner->edns_cookies && match_and_process_server_cookie( @@ -1502,8 +1498,7 @@ stub_udp_write_cb(void *userarg) netreq->debug_start_time = _getdns_get_time_as_uintt64(); netreq->debug_udp = 1; - netreq->query_id = arc4random(); - GLDNS_ID_SET(netreq->query, netreq->query_id); + GLDNS_ID_SET(netreq->query, (uint16_t)arc4random()); if (netreq->opt) { _getdns_network_req_clear_upstream_options(netreq); if (netreq->edns_maximum_udp_payload_size == -1) @@ -1731,8 +1726,8 @@ upstream_write_cb(void *userarg) /* Could not complete the set up. Need to fallback.*/ DEBUG_STUB("%s %-35s: Upstream: %p ERROR = %d\n", STUB_DEBUG_WRITE, __FUNC__, (void*)userarg, q); - (void) _getdns_rbtree_delete( - &upstream->netreq_by_query_id, (void *)(intptr_t)netreq->query_id); + (void) _getdns_rbtree_delete(&upstream->netreq_by_query_id, + (void *)(intptr_t)GLDNS_ID_WIRE(netreq->query)); upstream_failed(upstream, (q == STUB_TCP_ERROR ? 0:1)); /* Fall through */ case STUB_CONN_GONE: diff --git a/src/types-internal.h b/src/types-internal.h index 22cf649a..7834fc32 100644 --- a/src/types-internal.h +++ b/src/types-internal.h @@ -226,7 +226,6 @@ typedef struct getdns_network_req size_t transport_current; getdns_tls_authentication_t tls_auth_min; getdns_eventloop_event event; - uint16_t query_id; int edns_maximum_udp_payload_size; uint16_t max_udp_payload_size;