From: Wouter Wijngaards Date: Mon, 2 Apr 2007 13:58:02 +0000 (+0000) Subject: Review of msgreply. X-Git-Tag: release-0.2~15 X-Git-Url: http://git.ipfire.org/cgi-bin/gitweb.cgi?a=commitdiff_plain;h=5bba40f64a963a5489598ba4ff6e64fcd6f9745b;p=thirdparty%2Funbound.git Review of msgreply. git-svn-id: file:///svn/unbound/trunk@212 be551aaa-1e26-0410-a405-d3ace91eadb9 --- diff --git a/daemon/worker.c b/daemon/worker.c index 2cad79000..0007e1618 100644 --- a/daemon/worker.c +++ b/daemon/worker.c @@ -161,7 +161,7 @@ worker_handle_reply(struct comm_point* c, void* arg, int error, memcpy(rep->reply, ldns_buffer_at(c->buffer, DNS_ID_AND_FLAGS), rep->replysize); reply_info_answer_iov(rep, w->query_id, w->query_flags, - &w->query_reply); + &w->query_reply, 0); req_release(w); /* store or update reply in the cache */ if(!(e = query_info_entrysetup(&w->qinfo, rep, w->query_hash))) { @@ -298,7 +298,7 @@ worker_handle_request(struct comm_point* c, void* arg, int error, log_info("answer from the cache"); memcpy(&id, ldns_buffer_begin(c->buffer), sizeof(uint16_t)); reply_info_answer_iov((struct reply_info*)e->data, id, - ldns_buffer_read_u16_at(c->buffer, 2), repinfo); + ldns_buffer_read_u16_at(c->buffer, 2), repinfo, 1); lock_rw_unlock(&e->lock); return 0; } @@ -316,6 +316,7 @@ worker_handle_request(struct comm_point* c, void* arg, int error, verbose(VERB_DETAIL, "worker: too many incoming requests " "active. dropping incoming query."); comm_point_drop_reply(repinfo); + query_info_clear(&qinfo); return 0; } worker->free_queries = w->next; diff --git a/doc/Changelog b/doc/Changelog index 120700d14..513a80ecf 100644 --- a/doc/Changelog +++ b/doc/Changelog @@ -2,6 +2,10 @@ - check sizes of udp received messages, not too short. - review changes. Some memmoves can be memcpys: 4byte aligned. set id correctly on cached answers. + - review changes msgreply.c, memleak on error condition. AA flag + clear on cached reply. Lowercase queries on hashing. + unit test on lowercasing. Test AA bit not set on cached reply. + Note that no TTLs are managed. 29 March 2007: Wouter - writev or sendmsg used when answering from cache. diff --git a/testcode/unitmain.c b/testcode/unitmain.c index 833af9265..f79d6e99f 100644 --- a/testcode/unitmain.c +++ b/testcode/unitmain.c @@ -147,6 +147,25 @@ msgreply_test() unit_assert( query_dname_len(dname_to_buf(buff, ".")) == 1 ); unit_assert( query_dname_len(dname_to_buf(buff, "bla.foo.")) == 9 ); unit_assert( query_dname_len(dname_to_buf(buff, "x.y.z.example.com.")) == 19 ); + + ldns_buffer_write_at(buff, 0, "\012abCDeaBCde\003cOm\000", 16); + query_dname_tolower(ldns_buffer_begin(buff), 16); + unit_assert( memcmp(ldns_buffer_begin(buff), + "\012abcdeabcde\003com\000", 16) == 0); + + ldns_buffer_write_at(buff, 0, "\001+\012abC{e-ZYXe\003NET\000", 18); + query_dname_tolower(ldns_buffer_begin(buff), 18); + unit_assert( memcmp(ldns_buffer_begin(buff), + "\001+\012abc{e-zyxe\003net\000", 18) == 0); + + ldns_buffer_write_at(buff, 0, "\000", 1); + query_dname_tolower(ldns_buffer_begin(buff), 1); + unit_assert( memcmp(ldns_buffer_begin(buff), "\000", 1) == 0); + + ldns_buffer_write_at(buff, 0, "\002NL\000", 4); + query_dname_tolower(ldns_buffer_begin(buff), 4); + unit_assert( memcmp(ldns_buffer_begin(buff), "\002nl\000", 4) == 0); + ldns_buffer_free(buff); } diff --git a/testdata/fwd_cached.rpl b/testdata/fwd_cached.rpl index 4e07ed014..9408730b0 100644 --- a/testdata/fwd_cached.rpl +++ b/testdata/fwd_cached.rpl @@ -6,6 +6,7 @@ SCENARIO_BEGIN Query receives answer from the cache STEP 1 QUERY ENTRY_BEGIN + REPLY RD SECTION QUESTION www.example.com. IN A ENTRY_END @@ -20,7 +21,8 @@ STEP 3 REPLY ENTRY_BEGIN MATCH opcode qtype qname ADJUST copy_id - REPLY QR RD RA NOERROR + ; authoritative answer + REPLY QR AA RD RA NOERROR SECTION QUESTION www.example.com. IN A SECTION ANSWER @@ -32,27 +34,40 @@ ENTRY_BEGIN ENTRY_END STEP 4 CHECK_ANSWER ENTRY_BEGIN - MATCH opcode qname qtype + MATCH all + ; first reply, have AA set. + REPLY QR AA RD RA SECTION QUESTION www.example.com. IN A SECTION ANSWER www.example.com. IN A 10.20.30.40 + SECTION AUTHORITY + www.example.com. IN NS ns.example.com. + SECTION ADDITIONAL + ns.example.com. IN A 10.20.30.50 ENTRY_END -; another query, same, so it must be answered frm the cache +; another query, same, so it must be answered from the cache STEP 5 QUERY ENTRY_BEGIN + REPLY RD SECTION QUESTION www.example.com. IN A ENTRY_END ; immediate answer without an OUT_QUERY happening (checked on exit) +; also, the answer does not have AA set STEP 6 CHECK_ANSWER ENTRY_BEGIN - MATCH opcode qname qtype + MATCH all + REPLY QR RD RA SECTION QUESTION www.example.com. IN A SECTION ANSWER www.example.com. IN A 10.20.30.40 + SECTION AUTHORITY + www.example.com. IN NS ns.example.com. + SECTION ADDITIONAL + ns.example.com. IN A 10.20.30.50 ENTRY_END SCENARIO_END diff --git a/util/data/msgreply.c b/util/data/msgreply.c index 405932cf9..17cb1907c 100644 --- a/util/data/msgreply.c +++ b/util/data/msgreply.c @@ -68,7 +68,8 @@ query_dname_len(ldns_buffer* query) } } -int query_info_parse(struct query_info* m, ldns_buffer* query) +int +query_info_parse(struct query_info* m, ldns_buffer* query) { uint8_t* q = ldns_buffer_begin(query); /* minimum size: header + \0 + qtype + qclass */ @@ -107,7 +108,9 @@ query_info_allocqname(struct query_info* m) if( (x) < (y) ) return -1; \ else if( (x) > (y) ) return +1; \ log_assert( (x) == (y) ); -int query_info_compare(void* m1, void* m2) + +int +query_info_compare(void* m1, void* m2) { struct query_info* msg1 = (struct query_info*)m1; struct query_info* msg2 = (struct query_info*)m2; @@ -115,8 +118,7 @@ int query_info_compare(void* m1, void* m2) /* from most different to least different for speed */ COMPARE_IT(msg1->qtype, msg2->qtype); COMPARE_IT(msg1->qnamesize, msg2->qnamesize); - mc = memcmp(msg1->qname, msg2->qname, msg1->qnamesize); - if(mc != 0) + if((mc = memcmp(msg1->qname, msg2->qname, msg1->qnamesize)) != 0) return mc; COMPARE_IT(msg1->has_cd, msg2->has_cd); COMPARE_IT(msg1->qclass, msg2->qclass); @@ -124,19 +126,22 @@ int query_info_compare(void* m1, void* m2) #undef COMPARE_IT } -void query_info_clear(struct query_info* m) +void +query_info_clear(struct query_info* m) { free(m->qname); m->qname = NULL; } -void reply_info_clear(struct reply_info* m) +void +reply_info_clear(struct reply_info* m) { free(m->reply); m->reply = NULL; } -size_t msgreply_sizefunc(void* k, void* d) +size_t +msgreply_sizefunc(void* k, void* d) { struct query_info* q = (struct query_info*)k; struct reply_info* r = (struct reply_info*)d; @@ -144,7 +149,8 @@ size_t msgreply_sizefunc(void* k, void* d) + r->replysize + q->qnamesize; } -void query_entry_delete(void *k, void* ATTR_UNUSED(arg)) +void +query_entry_delete(void *k, void* ATTR_UNUSED(arg)) { struct msgreply_entry* q = (struct msgreply_entry*)k; lock_rw_destroy(&q->entry.lock); @@ -152,24 +158,45 @@ void query_entry_delete(void *k, void* ATTR_UNUSED(arg)) free(q); } -void reply_info_delete(void* d, void* ATTR_UNUSED(arg)) +void +reply_info_delete(void* d, void* ATTR_UNUSED(arg)) { struct reply_info* r = (struct reply_info*)d; reply_info_clear(r); free(r); } -hashvalue_t query_info_hash(struct query_info *q) +void +query_dname_tolower(uint8_t* dname, size_t len) +{ + /* the dname is stored uncompressed */ + uint8_t labellen; + log_assert(len > 0); + labellen = *dname; + while(labellen) { + dname++; + while(labellen--) { + *dname = (uint8_t)tolower((int)*dname); + dname++; + } + labellen = *dname; + } +} + +hashvalue_t +query_info_hash(struct query_info *q) { hashvalue_t h = 0xab; h = hashlittle(&q->qtype, sizeof(q->qtype), h); h = hashlittle(&q->qclass, sizeof(q->qclass), h); h = hashlittle(&q->has_cd, sizeof(q->has_cd), h); + query_dname_tolower(q->qname, q->qnamesize); h = hashlittle(q->qname, q->qnamesize, h); return h; } -void reply_info_answer(struct reply_info* rep, uint16_t qflags, +void +reply_info_answer(struct reply_info* rep, uint16_t qflags, ldns_buffer* buffer) { uint16_t flags; @@ -184,26 +211,32 @@ void reply_info_answer(struct reply_info* rep, uint16_t qflags, void reply_info_answer_iov(struct reply_info* rep, uint16_t qid, - uint16_t qflags, struct comm_reply* comrep) + uint16_t qflags, struct comm_reply* comrep, int cached) { - uint16_t flags; - /* [0]=tcp, [1]=id, [2]=flags, [3]=message */ + /* [0]=reserved for tcplen, [1]=id, [2]=flags, [3]=message */ struct iovec iov[4]; iov[1].iov_base = &qid; iov[1].iov_len = sizeof(uint16_t); - flags = rep->flags | (qflags & 0x0100); /* copy RD bit */ - log_assert(flags & 0x8000); /* QR bit must be on in our replies */ - flags = htons(flags); - iov[2].iov_base = &flags; + if(!cached) { + /* original flags, copy RD bit from query. */ + qflags = rep->flags | (qflags & 0x0100); + } else { + /* remove AA bit, copy RD and CD bits from query. */ + qflags = (rep->flags & ~0x0400) | (qflags & 0x0110); + } + log_assert(qflags & 0x8000); /* QR bit must be on in our replies */ + qflags = htons(qflags); + iov[2].iov_base = &qflags; iov[2].iov_len = sizeof(uint16_t); iov[3].iov_base = rep->reply; iov[3].iov_len = rep->replysize; comm_point_send_reply_iov(comrep, iov, 4); } -struct msgreply_entry* query_info_entrysetup(struct query_info* q, - struct reply_info* r, hashvalue_t h) +struct msgreply_entry* +query_info_entrysetup(struct query_info* q, struct reply_info* r, + hashvalue_t h) { struct msgreply_entry* e = (struct msgreply_entry*)malloc( sizeof(struct msgreply_entry)); diff --git a/util/data/msgreply.h b/util/data/msgreply.h index 155358bb9..ecf94505c 100644 --- a/util/data/msgreply.h +++ b/util/data/msgreply.h @@ -88,9 +88,9 @@ struct msgreply_entry { /** * Parse wire query into a queryinfo structure, return 0 on parse error. - * initialises the (prealloced) queryinfo structure as well. sets reply to 0. + * initialises the (prealloced) queryinfo structure as well. * This query structure contains a pointer back info the buffer! - * This pointer avoids memory allocation. + * This pointer avoids memory allocation. allocqname does memory allocation. * @param m: the prealloced queryinfo structure to put query into. * must be unused, or _clear()ed. * @param query: the wireformat packet query. starts with ID. @@ -106,8 +106,8 @@ int query_info_parse(struct query_info* m, ldns_buffer* query); int query_info_allocqname(struct query_info* m); /** - * Compare two queryinfo structures, on query, - * The qname is _not_ sorted in canonical ordering. + * Compare two queryinfo structures, on query and type, class. + * It is _not_ sorted in canonical ordering. * @param m1: struct query_info* , void* here to ease use as function pointer. * @param m2: struct query_info* , void* here to ease use as function pointer. * @return: 0 = same, -1 m1 is smaller, +1 m1 is larger. @@ -118,9 +118,12 @@ int query_info_compare(void* m1, void* m2); void query_info_clear(struct query_info* m); /** helper routine, determine length of dname in buffer, no compression ptrs - * allowed, returns 0 on failure. */ + * allowed, returns 0 on parse failure. */ size_t query_dname_len(ldns_buffer* query); +/** lowercase query dname */ +void query_dname_tolower(uint8_t* dname, size_t len); + /** clear out reply info structure */ void reply_info_clear(struct reply_info* m); @@ -133,7 +136,7 @@ void query_entry_delete(void *q, void* arg); /** delete reply_info data structure */ void reply_info_delete(void* d, void* arg); -/** calculate hash value of query_info */ +/** calculate hash value of query_info, lowercases the qname. */ hashvalue_t query_info_hash(struct query_info *q); /** @@ -153,9 +156,11 @@ void reply_info_answer(struct reply_info* rep, uint16_t qflags, * @param qid: query id, in network byte order. * @param qflags: flags word from the query. * @param comrep: communication reply point. + * @param cached: set true if a cached reply (so no AA bit). + * set false for the first reply. */ void reply_info_answer_iov(struct reply_info* rep, uint16_t qid, - uint16_t qflags, struct comm_reply* comrep); + uint16_t qflags, struct comm_reply* comrep, int cached); /** * Setup query info entry