From 42a35ac26ec902af99ed7df19656f5b4cbe594b0 Mon Sep 17 00:00:00 2001 From: Ralph Dolmans Date: Wed, 16 Sep 2020 18:25:02 +0200 Subject: [PATCH] - Final round of DoH review feedback processing. --- services/listen_dnsport.c | 18 ++++++++++++------ services/mesh.c | 6 +++--- testcode/dohclient.c | 11 +++++++++-- .../doh_downstream.tdir/doh_downstream.test | 13 ++++--------- .../doh_downstream_endpoint.test | 2 +- .../doh_downstream_post.test | 12 ++++-------- util/configparser.y | 4 +++- util/netevent.c | 10 +++++----- util/netevent.h | 4 ++-- 9 files changed, 43 insertions(+), 37 deletions(-) diff --git a/services/listen_dnsport.c b/services/listen_dnsport.c index 27f92a9dc..7833497b1 100644 --- a/services/listen_dnsport.c +++ b/services/listen_dnsport.c @@ -2227,7 +2227,7 @@ int http2_submit_dns_response(struct http2_session* h2_session) sldns_buffer_write(h2_stream->rbuffer, sldns_buffer_current(h2_session->c->buffer), - sldns_buffer_remaining(h2_stream->rbuffer)); + sldns_buffer_remaining(h2_session->c->buffer)); sldns_buffer_flip(h2_stream->rbuffer); data_prd.source.ptr = h2_session; @@ -2540,14 +2540,16 @@ static int http2_buffer_uri_query(struct http2_session* h2_session, "in http2-query-buffer-size"); return http2_submit_rst_stream(h2_session, h2_stream); } + http2_query_buffer_count += expectb64len; + lock_basic_unlock(&http2_query_buffer_count_lock); if(!(h2_stream->qbuffer = sldns_buffer_new(expectb64len))) { + lock_basic_lock(&http2_query_buffer_count_lock); + http2_query_buffer_count -= expectb64len; lock_basic_unlock(&http2_query_buffer_count_lock); log_err("http2_req_header fail, qbuffer " "malloc failure"); return 0; } - http2_query_buffer_count += expectb64len; - lock_basic_unlock(&http2_query_buffer_count_lock); if(!(b64len = sldns_b64url_pton( (char const *)start, length, @@ -2627,7 +2629,7 @@ static int http2_req_header_cb(nghttp2_session* session, * stream. */ #define HTTP_QUERY_PARAM "?dns=" size_t el = strlen(h2_session->c->http_endpoint); - size_t qpl = sizeof(HTTP_QUERY_PARAM) - 1; + size_t qpl = strlen(HTTP_QUERY_PARAM); if(valuelen < el || memcmp(h2_session->c->http_endpoint, value, el) != 0) { @@ -2727,9 +2729,13 @@ static int http2_req_data_chunk_recv_cb(nghttp2_session* ATTR_UNUSED(session), "in http2-query-buffer-size"); return http2_submit_rst_stream(h2_session, h2_stream); } - if((h2_stream->qbuffer = sldns_buffer_new(qlen))) - http2_query_buffer_count += qlen; + http2_query_buffer_count += qlen; lock_basic_unlock(&http2_query_buffer_count_lock); + if(!(h2_stream->qbuffer = sldns_buffer_new(qlen))) { + lock_basic_lock(&http2_query_buffer_count_lock); + http2_query_buffer_count -= qlen; + lock_basic_unlock(&http2_query_buffer_count_lock); + } } if(!h2_stream->qbuffer || diff --git a/services/mesh.c b/services/mesh.c index c2afdbf82..ffb6d092a 100644 --- a/services/mesh.c +++ b/services/mesh.c @@ -551,7 +551,7 @@ void mesh_new_client(struct mesh_area* mesh, struct query_info* qinfo, goto servfail_mem; } } - if(rep->c->alpn_h2) { + if(rep->c->use_h2) { http2_stream_add_meshstate(rep->c->h2_stream, mesh, s); } /* add serve expired timer if required and not already there */ @@ -1210,7 +1210,7 @@ mesh_send_reply(struct mesh_state* m, int rcode, struct reply_info* rep, else secure = 0; if(!rep && rcode == LDNS_RCODE_NOERROR) rcode = LDNS_RCODE_SERVFAIL; - if(r->query_reply.c->alpn_h2) { + if(r->query_reply.c->use_h2) { r->query_reply.c->h2_stream = r->h2_stream; /* Mesh reply won't exist for long anymore. Make it impossible * for HTTP/2 stream to refer to mesh state, in case @@ -1498,7 +1498,7 @@ int mesh_state_add_reply(struct mesh_state* s, struct edns_data* edns, s->s.qinfo.qname_len); if(!r->qname) return 0; - if(rep->c->alpn_h2) + if(rep->c->use_h2) r->h2_stream = rep->c->h2_stream; /* Data related to local alias stored in 'qinfo' (if any) is ephemeral diff --git a/testcode/dohclient.c b/testcode/dohclient.c index 060d71ee4..adcc7d831 100644 --- a/testcode/dohclient.c +++ b/testcode/dohclient.c @@ -171,8 +171,9 @@ submit_query(struct http2_session* h2_session, struct sldns_buffer* buf) h2_stream->path = malloc(strlen( h2_session->endpoint)+strlen("?dns=")+qb64_size+1); if(!h2_stream->path) fatal_exit("out of memory"); - sprintf(h2_stream->path, "%s?dns=%s", h2_session->endpoint, - qb64); + snprintf(h2_stream->path, strlen(h2_session->endpoint)+ + strlen("?dns=")+qb64_size+1, "%s?dns=%s", + h2_session->endpoint, qb64); free(qb64); } @@ -328,6 +329,11 @@ static int http2_data_chunk_recv_cb(nghttp2_session* ATTR_UNUSED(session), return 0; } + if(sldns_buffer_remaining(h2_stream->buf) < len) { + log_err("received data chunck does not fit into buffer"); + return NGHTTP2_ERR_CALLBACK_FAILURE; + } + sldns_buffer_write(h2_stream->buf, data, len); return 0; @@ -575,5 +581,6 @@ int main(int argc, char** argv) int main(int ATTR_UNUSED(argc), char** ATTR_UNUSED(argv)) { printf("Compiled without nghttp2, cannot run test.\n"); + return 1; } #endif /* HAVE_NGHTTP2 */ diff --git a/testdata/doh_downstream.tdir/doh_downstream.test b/testdata/doh_downstream.tdir/doh_downstream.test index 98a54af26..78e2e84eb 100644 --- a/testdata/doh_downstream.tdir/doh_downstream.test +++ b/testdata/doh_downstream.tdir/doh_downstream.test @@ -323,17 +323,12 @@ if test "$?" -ne 0; then echo "Not OK" exit 1 fi -grep "a.example.com. IN A" outfile - -echo "" -echo "> query www5.example.net. www3.example.net. www.drop.net." -$PRE/dohclient -s 127.0.0.1 -p $UNBOUND_PORT www5.example.com. A IN www3.example.net A IN www.drop.net A IN >outfile 2>&1 -cat outfile -if test "$?" -ne 0; then - echo "exit status not OK" +num_ans=$(grep -B 3 "a.example.com. IN A" outfile | grep "rcode: NOERROR" | wc -l ) +if test "$num_ans" -ne 90; then + echo "number of answers not OK" echo "> cat logfiles" cat outfile - cat fwd.log + cat fwd.log cat unbound.log echo "Not OK" exit 1 diff --git a/testdata/doh_downstream_endpoint.tdir/doh_downstream_endpoint.test b/testdata/doh_downstream_endpoint.tdir/doh_downstream_endpoint.test index 0e25a47f6..d788e3667 100644 --- a/testdata/doh_downstream_endpoint.tdir/doh_downstream_endpoint.test +++ b/testdata/doh_downstream_endpoint.tdir/doh_downstream_endpoint.test @@ -33,7 +33,7 @@ else fi echo "OK" -echo "> query www.example.net. endpoint /dns-query" +echo "> query www.example.net. endpoint /abc" $PRE/dohclient -e /abc -s 127.0.0.1 -p $UNBOUND_PORT www.example.net. A IN >outfile 2>&1 cat outfile if test "$?" -ne 0; then diff --git a/testdata/doh_downstream_post.tdir/doh_downstream_post.test b/testdata/doh_downstream_post.tdir/doh_downstream_post.test index 4a49c8b51..d6a512ae3 100644 --- a/testdata/doh_downstream_post.tdir/doh_downstream_post.test +++ b/testdata/doh_downstream_post.tdir/doh_downstream_post.test @@ -323,17 +323,13 @@ if test "$?" -ne 0; then echo "Not OK" exit 1 fi -grep "a.example.com. IN A" outfile -echo "" -echo "> query www5.example.net. www3.example.net. www.drop.net." -$PRE/dohclient -P -s 127.0.0.1 -p $UNBOUND_PORT www5.example.com. A IN www3.example.net A IN www.drop.net A IN >outfile 2>&1 -cat outfile -if test "$?" -ne 0; then - echo "exit status not OK" +num_ans=$(grep -B 3 "a.example.com. IN A" outfile | grep "rcode: NOERROR" | wc -l ) +if test "$num_ans" -ne 90; then + echo "number of answers not OK" echo "> cat logfiles" cat outfile - cat fwd.log + cat fwd.log cat unbound.log echo "Not OK" exit 1 diff --git a/util/configparser.y b/util/configparser.y index 6ef42628c..ffc8813ff 100644 --- a/util/configparser.y +++ b/util/configparser.y @@ -982,8 +982,10 @@ server_http_endpoint: VAR_HTTP_ENDPOINT STRING_ARG free(cfg_parser->cfg->http_endpoint); if($2 && $2[0] != '/') { cfg_parser->cfg->http_endpoint = malloc(strlen($2)+2); + if(!cfg_parser->cfg->http_endpoint) + yyerror("out of memory"); cfg_parser->cfg->http_endpoint[0] = '/'; - memcpy(cfg_parser->cfg->http_endpoint+1, $2, + memmove(cfg_parser->cfg->http_endpoint+1, $2, strlen($2)+1); free($2); } else { diff --git a/util/netevent.c b/util/netevent.c index be3b740b5..2171e3e6d 100644 --- a/util/netevent.c +++ b/util/netevent.c @@ -1228,7 +1228,7 @@ ssl_handshake(struct comm_point* c) if(alpnlen == 2 && memcmp("h2", alpn, 2) == 0) { /* connection upgraded to HTTP2 */ c->tcp_do_toggle_rw = 0; - c->alpn_h2 = 1; + c->use_h2 = 1; } } @@ -2472,7 +2472,7 @@ comm_point_http_handle_read(int fd, struct comm_point* c) if(!c->tcp_is_reading) return 1; - if(c->alpn_h2) { + if(c->use_h2) { return comm_point_http2_handle_read(fd, c); } @@ -2766,7 +2766,7 @@ comm_point_http_handle_write(int fd, struct comm_point* c) if(c->tcp_is_reading) return 1; - if(c->alpn_h2) { + if(c->use_h2) { return comm_point_http2_handle_write(fd, c); } @@ -3160,7 +3160,7 @@ comm_point_create_http_handler(struct comm_base *base, free(c); return NULL; } - c->alpn_h2 = 0; + c->use_h2 = 0; #ifdef HAVE_NGHTTP2 if(!(c->h2_session = http2_session_create(c))) { log_err("could not create http2 session"); @@ -3676,7 +3676,7 @@ comm_point_send_reply(struct comm_reply *repinfo) #endif if(repinfo->c->tcp_req_info) { tcp_req_info_send_reply(repinfo->c->tcp_req_info); - } else if(repinfo->c->alpn_h2) { + } else if(repinfo->c->use_h2) { if(!http2_submit_dns_response(repinfo->c->h2_session)) { comm_point_drop_reply(repinfo); return; diff --git a/util/netevent.h b/util/netevent.h index 68f4c1297..6986f881b 100644 --- a/util/netevent.h +++ b/util/netevent.h @@ -236,8 +236,8 @@ struct comm_point { /* -------- HTTP/2 ------- */ /** http2 session */ struct http2_session* h2_session; - /** set to 1 if h2 is negatiated using alpn */ - int alpn_h2; + /** set to 1 if h2 is negotiated to be used (using alpn) */ + int use_h2; /** stream currently being handled */ struct http2_stream* h2_stream; /** maximum allowed query buffer size, per stream */ -- 2.47.3