review changes.

git-svn-id: file:///svn/unbound/trunk@250 be551aaa-1e26-0410-a405-d3ace91eadb9

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.

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.

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);

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);

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;
 		}

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;

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.

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; i<rep->rrset_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