From: Wouter Wijngaards Date: Wed, 18 Apr 2007 13:57:01 +0000 (+0000) Subject: review changes. X-Git-Tag: release-0.3~53 X-Git-Url: http://git.ipfire.org/cgi-bin/gitweb.cgi?a=commitdiff_plain;h=561385c35a76c1dedfccb6277a60e658cb78a6d1;p=thirdparty%2Funbound.git review changes. git-svn-id: file:///svn/unbound/trunk@250 be551aaa-1e26-0410-a405-d3ace91eadb9 --- diff --git a/doc/Changelog b/doc/Changelog index a4399f453..13b6fee30 100644 --- a/doc/Changelog +++ b/doc/Changelog @@ -1,3 +1,7 @@ +18 April 2007: Wouter + - review of msgparse code. + - smaller test cases. + 17 April 2007: Wouter - copy and decompress dnames. - store calculated hash value too. diff --git a/doc/TODO b/doc/TODO index 3c2385bfb..143766f1e 100644 --- a/doc/TODO +++ b/doc/TODO @@ -11,3 +11,4 @@ o possible optimization, so that precious id number resource is not depleted by parsing of messages. Delay malloc, as above, or try to reverse release special id numbers, and if you release the next_id number for the thread it reuses that id number. +o calculate rdata size during parse step, instead of delay until copy. diff --git a/testcode/unitmain.c b/testcode/unitmain.c index 7719d8751..26ef8da5e 100644 --- a/testcode/unitmain.c +++ b/testcode/unitmain.c @@ -211,13 +211,11 @@ main(int argc, char* argv[]) } printf("Start of %s unit test.\n", PACKAGE_STRING); checklock_start(); - /* net_test(); alloc_test(); msgreply_test(); lruhash_test(); slabhash_test(); - */ msgparse_test(); checklock_stop(); printf("%d tests succeeded\n", testcount); diff --git a/testcode/unitmsgparse.c b/testcode/unitmsgparse.c index 376ad1fd3..4b6a577ad 100644 --- a/testcode/unitmsgparse.c +++ b/testcode/unitmsgparse.c @@ -293,7 +293,7 @@ testfromfile(ldns_buffer* pkt, struct alloc_cache* alloc, ldns_buffer* out, perror("fname"); return; } - while(fgets(buf, sizeof(buf), in)) { + while(fgets(buf, (int)sizeof(buf), in)) { if(buf[0] == ';') /* comment */ continue; if(strlen(buf) < 10) /* skip pcat line numbers. */ @@ -320,6 +320,8 @@ void msgparse_test() printf("testmsgparse\n"); simpletest(pkt, &alloc, out); testfromfile(pkt, &alloc, out, "testdata/test_packets.1"); + testfromfile(pkt, &alloc, out, "testdata/test_packets.2"); + testfromfile(pkt, &alloc, out, "testdata/test_packets.3"); /* cleanup */ alloc_clear(&alloc); diff --git a/util/data/dname.c b/util/data/dname.c index 005dc543e..7c2c7da89 100644 --- a/util/data/dname.c +++ b/util/data/dname.c @@ -41,6 +41,7 @@ #include "config.h" #include "util/data/dname.h" +#include "util/data/msgparse.h" #include "util/log.h" #include "util/storage/lookup3.h" @@ -54,7 +55,7 @@ query_dname_len(ldns_buffer* query) if(ldns_buffer_remaining(query) < 1) return 0; /* parse error, need label len */ labellen = ldns_buffer_read_u8(query); - if(labellen & 0xC0) + if(labellen&0xc0) return 0; /* no compression allowed in queries */ len += labellen + 1; if(len > LDNS_MAX_DOMAINLEN) @@ -152,12 +153,12 @@ pkt_dname_len(ldns_buffer* pkt) if(ldns_buffer_remaining(pkt) < 1) return 0; labellen = ldns_buffer_read_u8(pkt); - if( (labellen & 0xc0) == 0xc0 ) { + if(LABEL_IS_PTR(labellen)) { /* compression ptr */ - uint16_t ptr = (labellen & 0x3f) << 8; + uint16_t ptr; if(ldns_buffer_remaining(pkt) < 1) return 0; - ptr |= ldns_buffer_read_u8(pkt); + ptr = PTR_OFFSET(labellen, ldns_buffer_read_u8(pkt)); if(loopcheck(loop, ptr)) return 0; /* loop! */ if(ldns_buffer_limit(pkt) <= ptr) @@ -196,13 +197,13 @@ dname_pkt_compare(ldns_buffer* pkt, uint8_t* d1, uint8_t* d2) len2 = *d2++; while( len1 != 0 || len2 != 0 ) { /* resolve ptrs */ - if( (len1 & 0xc0) == 0xc0) { - d1 = ldns_buffer_at(pkt, (len1&0x3f)<<8 | *d1); + if(LABEL_IS_PTR(len1)) { + d1 = ldns_buffer_at(pkt, PTR_OFFSET(len1, *d1)); len1 = *d1++; continue; } - if( (len2 & 0xc0) == 0xc0) { - d2 = ldns_buffer_at(pkt, (len2&0x3f)<<8 | *d2); + if(LABEL_IS_PTR(len2)) { + d2 = ldns_buffer_at(pkt, PTR_OFFSET(len2, *d2)); len2 = *d2++; continue; } @@ -261,9 +262,9 @@ dname_pkt_hash(ldns_buffer* pkt, uint8_t* dname, hashvalue_t h) /* preserve case of query, make hash label by label */ lablen = *dname++; while(lablen) { - if((lablen & 0xc0) == 0xc0) { + if(LABEL_IS_PTR(lablen)) { /* follow pointer */ - dname = ldns_buffer_at(pkt, (lablen&0x3f)<<8 | *dname); + dname = ldns_buffer_at(pkt, PTR_OFFSET(lablen, *dname)); lablen = *dname++; continue; } @@ -285,9 +286,9 @@ void dname_pkt_copy(ldns_buffer* pkt, uint8_t* to, uint8_t* dname) uint8_t lablen; lablen = *dname++; while(lablen) { - if((lablen & 0xc0) == 0xc0) { + if(LABEL_IS_PTR(lablen)) { /* follow pointer */ - dname = ldns_buffer_at(pkt, (lablen&0x3f)<<8 | *dname); + dname = ldns_buffer_at(pkt, PTR_OFFSET(lablen, *dname)); lablen = *dname++; continue; } diff --git a/util/data/msgparse.c b/util/data/msgparse.c index 2b0aecd78..509155423 100644 --- a/util/data/msgparse.c +++ b/util/data/msgparse.c @@ -47,33 +47,17 @@ /** smart comparison of (compressed, valid) dnames from packet. */ static int smart_compare(ldns_buffer* pkt, uint8_t* dnow, - uint8_t *dprfirst, uint8_t* dprlast) + uint8_t* dprfirst, uint8_t* dprlast) { - if( (*dnow & 0xc0) == 0xc0) { + if(LABEL_IS_PTR(*dnow)) { /* ptr points to a previous dname */ - uint8_t* p = ldns_buffer_at(pkt, (dnow[0]&0x3f)<<8 | dnow[1]); + uint8_t* p = ldns_buffer_at(pkt, PTR_OFFSET(dnow[0], dnow[1])); if( p == dprfirst || p == dprlast ) return 0; /* prev dname is also a ptr, both ptrs are the same. */ - /* if( (*dprfirst & 0xc0) == 0xc0 && - dprfirst[0] == dnow[0] && dprfirst[1] == dnow[1]) - return 0; */ - if( (*dprlast & 0xc0) == 0xc0 && + if(LABEL_IS_PTR(*dprlast) && dprlast[0] == dnow[0] && dprlast[1] == dnow[1]) return 0; - /* checks for prev dnames pointing forwards in the packet - } else { - if( (*dprfirst & 0xc0) == 0xc0 ) { - if(ldns_buffer_at(pkt, (dprfirst[0]&0x3f)<<8 | - dprfirst[1]) == dnow) - return 0; - } - if( (*dprlast & 0xc0) == 0xc0 ) { - if(ldns_buffer_at(pkt, (dprlast[0]&0x3f)<<8 | - dprlast[1]) == dnow) - return 0; - } - */ } return dname_pkt_compare(pkt, dnow, dprlast); } @@ -104,7 +88,7 @@ nsec_at_apex(ldns_buffer* pkt) /* nsec type bitmap contains items */ uint8_t win, blen, bits; /* need: windownum, bitmap len, firstbyte */ - if(ldns_buffer_position(pkt)+3 <= pos+rdatalen) { + if(ldns_buffer_position(pkt)+3 > pos+rdatalen) { ldns_buffer_set_position(pkt, pos); return 0; /* malformed nsec */ } @@ -277,43 +261,143 @@ new_rrset(struct msg_parse* msg, uint8_t* dname, size_t dnamelen, p->rrset_class = dclass; p->flags = rrset_flags; p->rr_count = 0; + p->size = 0; p->rr_first = 0; p->rr_last = 0; return p; } +size_t +get_rdf_size(ldns_rdf_type rdf) +{ + switch(rdf) { + case LDNS_RDF_TYPE_CLASS: + case LDNS_RDF_TYPE_ALG: + case LDNS_RDF_TYPE_INT8: + return 1; + break; + case LDNS_RDF_TYPE_INT16: + case LDNS_RDF_TYPE_TYPE: + case LDNS_RDF_TYPE_CERT_ALG: + return 2; + break; + case LDNS_RDF_TYPE_INT32: + case LDNS_RDF_TYPE_TIME: + case LDNS_RDF_TYPE_A: + case LDNS_RDF_TYPE_PERIOD: + return 4; + break; + case LDNS_RDF_TYPE_TSIGTIME: + return 6; + break; + case LDNS_RDF_TYPE_AAAA: + return 16; + break; + default: + log_assert(false); /* add type above */ + /* only types that appear before a domain * + * name are needed. rest is simply copied. */ + } + return 0; +} + +/** calculate the size of one rr */ +static int +calc_size(ldns_buffer* pkt, uint16_t type, struct rr_parse* rr) +{ + const ldns_rr_descriptor* desc; + uint16_t pkt_len; /* length of rr inside the packet */ + rr->size = sizeof(uint16_t); /* the rdatalen */ + ldns_buffer_skip(pkt, 4); /* skip ttl */ + pkt_len = ldns_buffer_read_u16(pkt); + if(ldns_buffer_remaining(pkt) < pkt_len) + return 0; + desc = ldns_rr_descript(type); + if(pkt_len > 0 && desc->_dname_count > 0) { + int count = (int)desc->_dname_count; + int rdf = 0; + size_t len; + size_t oldpos; + /* skip first part. */ + while(pkt_len > 0 && count) { + switch(desc->_wireformat[rdf]) { + case LDNS_RDF_TYPE_DNAME: + /* decompress every domain name */ + oldpos = ldns_buffer_position(pkt); + if((len = pkt_dname_len(pkt)) == 0) + return 0; /* malformed dname */ + if(ldns_buffer_position(pkt)-oldpos > pkt_len) + return 0; /* dname exceeds rdata */ + pkt_len -= ldns_buffer_position(pkt)-oldpos; + rr->size += len; + count--; + len = 0; + break; + case LDNS_RDF_TYPE_STR: + if(pkt_len < 1) + return 0; /* len byte exceeds rdata */ + len = ldns_buffer_current(pkt)[0] + 1; + break; + default: + len = get_rdf_size(desc->_wireformat[rdf]); + } + if(len) { + if(pkt_len < len) + return 0; /* exceeds rdata */ + pkt_len -= len; + ldns_buffer_skip(pkt, (ssize_t)len); + rr->size += len; + } + rdf++; + } + } + /* remaining rdata */ + rr->size += pkt_len; + ldns_buffer_skip(pkt, (ssize_t)pkt_len); + return 1; +} + + /** Add rr (from packet here) to rrset, skips rr */ static int add_rr_to_rrset(struct rrset_parse* rrset, ldns_buffer* pkt, region_type* region, ldns_pkt_section section) { uint16_t rdatalen; + struct rr_parse* rr; /* check section of rrset. */ if(rrset->section != section) { - /* silently drop it */ + /* silently drop it - it is a security problem, since + * trust in rr data depends on the section it is in. + * the less trustworthy part is discarded. */ verbose(VERB_DETAIL, "Packet contains rrset data in " "multiple sections, dropped last part."); - } else { - /* create rr */ - struct rr_parse* rr = region_alloc(region, sizeof(*rr)); - if(!rr) return LDNS_RCODE_SERVFAIL; - rr->ttl_data = ldns_buffer_current(pkt); - rr->next = 0; - if(rrset->rr_last) - rrset->rr_last->next = rr; - else rrset->rr_first = rr; - rrset->rr_last = rr; - rrset->rr_count++; - } + /* forwards */ + if(ldns_buffer_remaining(pkt) < 6) /* ttl + rdatalen */ + return LDNS_RCODE_FORMERR; + ldns_buffer_skip(pkt, 4); /* ttl */ + rdatalen = ldns_buffer_read_u16(pkt); + if(ldns_buffer_remaining(pkt) < rdatalen) + return LDNS_RCODE_FORMERR; + ldns_buffer_skip(pkt, (ssize_t)rdatalen); + return 0; + } + /* create rr */ + if(!(rr = (struct rr_parse*)region_alloc(region, sizeof(*rr)))) + return LDNS_RCODE_SERVFAIL; + rr->ttl_data = ldns_buffer_current(pkt); + rr->next = 0; + if(rrset->rr_last) + rrset->rr_last->next = rr; + else rrset->rr_first = rr; + rrset->rr_last = rr; + rrset->rr_count++; - /* forwards */ - if(ldns_buffer_remaining(pkt) < 6) /* ttl + rdatalen */ + /* calc decompressed size */ + if(!calc_size(pkt, ntohs(rrset->type), rr)) return LDNS_RCODE_FORMERR; - ldns_buffer_skip(pkt, 4); /* ttl */ - rdatalen = ldns_buffer_read_u16(pkt); - if(ldns_buffer_remaining(pkt) < rdatalen) - return LDNS_RCODE_FORMERR; - ldns_buffer_skip(pkt, (ssize_t)rdatalen); + rrset->size += rr->size; + return 0; } @@ -357,21 +441,20 @@ parse_section(ldns_buffer* pkt, struct msg_parse* msg, region_type* region, ldns_buffer_read(pkt, &dclass, sizeof(dclass)); /* see if it is part of an existing RR set */ - if((rrset = find_rrset(msg, pkt, dname, dnamelen, type, dclass, + if(!(rrset = find_rrset(msg, pkt, dname, dnamelen, type, dclass, &hash, &rrset_flags, &prev_dname_f, &prev_dname_l, &prev_dnamelen, &prev_type, &prev_dclass, - &rrset_prev)) != 0) { - /* check if it fits the existing rrset */ - /* add to rrset. */ - } else { + &rrset_prev))) { /* it is a new RR set. hash&flags already calculated.*/ (*num_rrsets)++; rrset = new_rrset(msg, dname, dnamelen, type, dclass, hash, rrset_flags, section, region); - if(!rrset) return LDNS_RCODE_SERVFAIL; + if(!rrset) + return LDNS_RCODE_SERVFAIL; rrset_prev = rrset; } - if((r=add_rr_to_rrset(rrset, pkt, region, section))) + /* add to rrset. */ + if((r=add_rr_to_rrset(rrset, pkt, region, section)) != 0) return r; } return 0; diff --git a/util/data/msgparse.h b/util/data/msgparse.h index bb69d20cb..1cbd6c836 100644 --- a/util/data/msgparse.h +++ b/util/data/msgparse.h @@ -117,12 +117,14 @@ struct rrset_parse { size_t dname_len; /** type, network order. */ uint16_t type; - /** class, network order. name so that it is not a c++ keyword. */ + /** class, network order. var name so that it is not a c++ keyword. */ uint16_t rrset_class; /** the flags for the rrset, like for packedrrset */ uint32_t flags; /** number of RRs in the rr list */ size_t rr_count; + /** sum of RR rdata sizes */ + size_t size; /** linked list of RRs in this rrset. */ struct rr_parse* rr_first; /** last in list of RRs in this rrset. */ @@ -145,6 +147,20 @@ struct rr_parse { struct rr_parse* next; }; +/** Check if label length is first octet of a compression pointer, pass u8. */ +#define LABEL_IS_PTR(x) ( ((x)&0xc0) == 0xc0 ) +/** Calculate destination offset of a compression pointer. pass first and + * second octets of the compression pointer. */ +#define PTR_OFFSET(x, y) ( ((x)&0x3f)<<8 | (y) ) + +/** + * Obtain size in the packet of an rr type, that is before dname type. + * Do TYPE_DNAME, and type STR, yourself. Gives size for most regular types. + * @param rdf: the rdf type from the descriptor. + * @return: size in octets. 0 on failure. + */ +size_t get_rdf_size(ldns_rdf_type rdf); + /** * Parse the packet. * @param pkt: packet, position at call must be at start of packet. diff --git a/util/data/msgreply.c b/util/data/msgreply.c index 457415562..a0c791d98 100644 --- a/util/data/msgreply.c +++ b/util/data/msgreply.c @@ -55,7 +55,8 @@ static uint8_t* copy_uncompr(uint8_t* dname, size_t len) { uint8_t* p = (uint8_t*)malloc(len); - if(!p) return 0; + if(!p) + return 0; memmove(p, dname, len); return p; } @@ -79,7 +80,7 @@ static int parse_create_repinfo(struct msg_parse* msg, struct reply_info** rep) { /* rrset_count-1 because the first ref is part of the struct. */ - *rep = malloc(sizeof(struct reply_info) + + *rep = (struct reply_info*)malloc(sizeof(struct reply_info) + sizeof(struct rrset_ref) * (msg->rrset_count-1) + sizeof(struct ub_packed_rrset_key*) * msg->rrset_count); if(!*rep) return 0; @@ -118,119 +119,6 @@ parse_alloc_rrset_keys(struct msg_parse* msg, struct reply_info* rep, return 1; } -/** - * Obtain size in the packet of an rr type, that is before dname type. - * Do TYPE_DNAME, and type STR, yourself. - * @param rdf: the rdf type from the descriptor. - * @return: size in octets. 0 on failure. - */ -static size_t -get_rdf_size(ldns_rdf_type rdf) -{ - switch(rdf) { - case LDNS_RDF_TYPE_CLASS: - case LDNS_RDF_TYPE_ALG: - case LDNS_RDF_TYPE_INT8: - return 1; - break; - case LDNS_RDF_TYPE_INT16: - case LDNS_RDF_TYPE_TYPE: - case LDNS_RDF_TYPE_CERT_ALG: - return 2; - break; - case LDNS_RDF_TYPE_INT32: - case LDNS_RDF_TYPE_TIME: - case LDNS_RDF_TYPE_A: - case LDNS_RDF_TYPE_PERIOD: - return 4; - break; - case LDNS_RDF_TYPE_TSIGTIME: - return 6; - break; - case LDNS_RDF_TYPE_AAAA: - return 16; - break; - default: - log_assert(false); /* add type above */ - /* only types that appear before a domain * - * name are needed. rest is simply copied. */ - } - return 0; -} - -/** calculate the size of one rr */ -static int -calc_size(ldns_buffer* pkt, uint16_t type, struct rr_parse* rr) -{ - const ldns_rr_descriptor* desc; - uint16_t pkt_len; /* length of rr inside the packet */ - rr->size = sizeof(uint16_t); /* the rdatalen */ - ldns_buffer_set_position(pkt, (size_t)(rr->ttl_data - - ldns_buffer_begin(pkt) + 4)); /* skip ttl */ - pkt_len = ldns_buffer_read_u16(pkt); - if(ldns_buffer_remaining(pkt) < pkt_len) - return 0; - desc = ldns_rr_descript(type); - if(pkt_len > 0 && desc->_dname_count > 0) { - int count = (int)desc->_dname_count; - int rdf = 0; - size_t len; - size_t oldpos; - /* skip first part. */ - while(pkt_len > 0 && count) { - switch(desc->_wireformat[rdf]) { - case LDNS_RDF_TYPE_DNAME: - /* decompress every domain name */ - oldpos = ldns_buffer_position(pkt); - if((len = pkt_dname_len(pkt)) == 0) - return 0; /* malformed dname */ - if(ldns_buffer_position(pkt)-oldpos > pkt_len) - return 0; /* dname exceeds rdata */ - pkt_len -= ldns_buffer_position(pkt)-oldpos; - rr->size += len; - count--; - len = 0; - break; - case LDNS_RDF_TYPE_STR: - if(pkt_len < 1) - return 0; /* len byte exceeds rdata */ - len = ldns_buffer_current(pkt)[0] + 1; - break; - default: - len = get_rdf_size(desc->_wireformat[rdf]); - } - if(len) { - if(pkt_len < len) - return 0; /* exceeds rdata */ - pkt_len -= len; - ldns_buffer_skip(pkt, (ssize_t)len); - rr->size += len; - } - rdf++; - } - } - /* remaining rdata */ - rr->size += pkt_len; - return 1; -} - -/** calculate size of rrs in rrset, 0 on parse failure */ -static int -parse_rr_size(ldns_buffer* pkt, struct rrset_parse* pset, size_t* allocsize) -{ - struct rr_parse* p = pset->rr_first; - *allocsize = 0; - /* size of rrs */ - while(p) { - if(!calc_size(pkt, ntohs(pset->type), p)) - return 0; - *allocsize += p->size; - p = p->next; - } - /* TODO calc size of rrsig */ - return 1; -} - /** do the rdata copy */ static int rdata_copy(ldns_buffer* pkt, struct rrset_parse* pset, @@ -241,12 +129,11 @@ rdata_copy(ldns_buffer* pkt, struct rrset_parse* pset, const ldns_rr_descriptor* desc; ldns_buffer_set_position(pkt, (size_t) (rr->ttl_data - ldns_buffer_begin(pkt))); - if(ldns_buffer_remaining(pkt) < 6) - return 0; + log_assert(ldns_buffer_remaining(pkt) >= 6 /* ttl + rdatalen */); ttl = ldns_buffer_read_u32(pkt); if(ttl < data->ttl) data->ttl = ttl; - /* insert decompressed size into memory rdata len */ + /* insert decompressed size into rdata len stored in memory */ pkt_len = htons(rr->size); memmove(to, &pkt_len, sizeof(uint16_t)); to += 2; @@ -331,13 +218,10 @@ static int parse_create_rrset(ldns_buffer* pkt, struct rrset_parse* pset, struct packed_rrset_data** data) { - /* calculate sizes of rr rdata */ - size_t allocsize; - if(!parse_rr_size(pkt, pset, &allocsize)) - return LDNS_RCODE_FORMERR; /* allocate */ *data = malloc(sizeof(struct packed_rrset_data) + pset->rr_count* - (sizeof(size_t)+sizeof(uint8_t*)+sizeof(uint32_t)) + allocsize); + (sizeof(size_t)+sizeof(uint8_t*)+sizeof(uint32_t)) + + pset->size); if(!*data) return LDNS_RCODE_SERVFAIL; /* copy & decompress */ @@ -366,7 +250,8 @@ parse_copy_decompress(ldns_buffer* pkt, struct msg_parse* msg, for(i=0; irrset_count; i++) { rep->rrsets[i]->rk.flags = pset->flags; rep->rrsets[i]->rk.dname_len = pset->dname_len; - rep->rrsets[i]->rk.dname = malloc(pset->dname_len + 4); + rep->rrsets[i]->rk.dname = (uint8_t*)malloc( + pset->dname_len + 4 /* size of type and class */ ); if(!rep->rrsets[i]->rk.dname) return LDNS_RCODE_SERVFAIL; /** copy & decompress dname */ @@ -416,9 +301,11 @@ int reply_info_parse(ldns_buffer* pkt, struct alloc_cache* alloc, qinf->qname = NULL; *rep = NULL; - msg = region_alloc(region, sizeof(*msg)); - if(!msg) - goto malloc_error; + if(!(msg = region_alloc(region, sizeof(*msg)))) { + region_free_all(region); + region_destroy(region); + return LDNS_RCODE_SERVFAIL; + } memset(msg, 0, sizeof(*msg)); log_assert(ldns_buffer_position(pkt) == 0); @@ -430,7 +317,6 @@ int reply_info_parse(ldns_buffer* pkt, struct alloc_cache* alloc, /* parse OK, allocate return structures */ /* this also performs dname decompression */ - *rep = NULL; if((ret = parse_create_msg(pkt, msg, alloc, qinf, rep)) != 0) { query_info_clear(qinf); reply_info_parsedelete(*rep, alloc); @@ -444,10 +330,6 @@ int reply_info_parse(ldns_buffer* pkt, struct alloc_cache* alloc, region_free_all(region); region_destroy(region); return 0; -malloc_error: - region_free_all(region); - region_destroy(region); - return LDNS_RCODE_SERVFAIL; } void