From 03ed19cf7163d658c2457ecebf9252ffc92750c9 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ond=C5=99ej=20Sur=C3=BD?= Date: Thu, 29 Feb 2024 22:26:29 +0100 Subject: [PATCH] Refactor the common buffer manipulation in rdataslab.c in macros The rdataslab.c was full of code like this: length = raw[0] * 256 + raw[1]; and count2 = *current2++ * 256; count2 += *current2++; Refactor code like this into peek_uint16() and get_uint16 macros to prevent code repetition and possible mistakes when copy and pasting the same code over and over. As a side note for an entertainment of a careful reader of the commit messages: The byte manipulation was changed from multiplication and addition to shift with or. The difference in the assembly looks like this: MUL and ADD: movzx eax, BYTE PTR [rdi] movzx edi, BYTE PTR [rdi+1] sal eax, 8 or edi, eax SHIFT and OR: movzx edi, WORD PTR [rdi] rol di, 8 movzx edi, di If the result and/or buffer is then being used after the macro call, there's more differences in favor of the SHIFT+OR solution. --- lib/dns/rdataslab.c | 153 +++++++++++++++++--------------------------- 1 file changed, 60 insertions(+), 93 deletions(-) diff --git a/lib/dns/rdataslab.c b/lib/dns/rdataslab.c index 941e60a318..6063c1be9c 100644 --- a/lib/dns/rdataslab.c +++ b/lib/dns/rdataslab.c @@ -89,6 +89,15 @@ struct xrdata { #endif /* if DNS_RDATASET_FIXED */ }; +#define peek_uint16(buffer) \ + ({ ((uint16_t) * (buffer) << 8) | *((buffer) + 1); }) +#define get_uint16(buffer) \ + ({ \ + uint16_t __ret = peek_uint16(buffer); \ + buffer += sizeof(uint16_t); \ + __ret; \ + }) + static void rdataset_disassociate(dns_rdataset_t *rdataset DNS__DB_FLARG); static isc_result_t @@ -381,25 +390,20 @@ free_rdatas: unsigned int dns_rdataslab_size(unsigned char *slab, unsigned int reservelen) { - unsigned int count, length; - unsigned char *current = NULL; - REQUIRE(slab != NULL); - current = slab + reservelen; - count = *current++ * 256; - count += *current++; + unsigned char *current = slab + reservelen; + uint16_t count = get_uint16(current); + #if DNS_RDATASET_FIXED current += (4 * count); #endif /* if DNS_RDATASET_FIXED */ - while (count > 0) { - count--; - length = *current++ * 256; - length += *current++; -#if DNS_RDATASET_FIXED - current += length + 2; -#else /* if DNS_RDATASET_FIXED */ + + while (count-- > 0) { + uint16_t length = get_uint16(current); current += length; +#if DNS_RDATASET_FIXED + current += 2; #endif /* if DNS_RDATASET_FIXED */ } @@ -408,26 +412,22 @@ dns_rdataslab_size(unsigned char *slab, unsigned int reservelen) { unsigned int dns_rdataslab_rdatasize(unsigned char *slab, unsigned int reservelen) { - unsigned int count, length, rdatalen = 0; - unsigned char *current = NULL; - REQUIRE(slab != NULL); - current = slab + reservelen; - count = *current++ * 256; - count += *current++; + uint16_t rdatalen = 0; + unsigned char *current = slab + reservelen; + uint16_t count = get_uint16(current); + #if DNS_RDATASET_FIXED current += (4 * count); #endif /* if DNS_RDATASET_FIXED */ - while (count > 0) { - count--; - length = *current++ * 256; - length += *current++; + + while (count-- > 0) { + uint16_t length = get_uint16(current); rdatalen += length; -#if DNS_RDATASET_FIXED - current += length + 2; -#else /* if DNS_RDATASET_FIXED */ current += length; +#if DNS_RDATASET_FIXED + current += 2; #endif /* if DNS_RDATASET_FIXED */ } @@ -436,14 +436,11 @@ dns_rdataslab_rdatasize(unsigned char *slab, unsigned int reservelen) { unsigned int dns_rdataslab_count(unsigned char *slab, unsigned int reservelen) { - unsigned int count; - unsigned char *current = NULL; - REQUIRE(slab != NULL); - current = slab + reservelen; - count = *current++ * 256; - count += *current++; + unsigned char *current = slab + reservelen; + uint16_t count = get_uint16(current); + return (count); } @@ -458,11 +455,8 @@ rdata_from_slab(unsigned char **current, dns_rdataclass_t rdclass, dns_rdatatype_t type, dns_rdata_t *rdata) { unsigned char *tcurrent = *current; isc_region_t region; - unsigned int length; bool offline = false; - - length = *tcurrent++ * 256; - length += *tcurrent++; + uint16_t length = get_uint16(tcurrent); if (type == dns_rdatatype_rrsig) { if ((*tcurrent & DNS_RDATASLAB_OFFLINE) != 0) { @@ -493,23 +487,19 @@ static bool rdata_in_slab(unsigned char *slab, unsigned int reservelen, dns_rdataclass_t rdclass, dns_rdatatype_t type, dns_rdata_t *rdata) { - unsigned int count, i; - unsigned char *current = NULL; - dns_rdata_t trdata = DNS_RDATA_INIT; - int n; + unsigned char *current = slab + reservelen; - current = slab + reservelen; - count = *current++ * 256; - count += *current++; + uint16_t count = get_uint16(current); #if DNS_RDATASET_FIXED current += (4 * count); #endif /* if DNS_RDATASET_FIXED */ - for (i = 0; i < count; i++) { + for (size_t i = 0; i < count; i++) { + dns_rdata_t trdata = DNS_RDATA_INIT; rdata_from_slab(¤t, rdclass, type, &trdata); - n = dns_rdata_compare(&trdata, rdata); + int n = dns_rdata_compare(&trdata, rdata); if (n == 0) { return (true); } @@ -552,15 +542,13 @@ dns_rdataslab_merge(unsigned char *oslab, unsigned char *nslab, REQUIRE(oslab != NULL && nslab != NULL); ocurrent = oslab + reservelen; - ocount = *ocurrent++ * 256; - ocount += *ocurrent++; + ocount = get_uint16(ocurrent); #if DNS_RDATASET_FIXED ocurrent += (4 * ocount); #endif /* if DNS_RDATASET_FIXED */ ostart = ocurrent; ncurrent = nslab + reservelen; - ncount = *ncurrent++ * 256; - ncount += *ncurrent++; + ncount = get_uint16(ncurrent); #if DNS_RDATASET_FIXED ncurrent += (4 * ncount); #endif /* if DNS_RDATASET_FIXED */ @@ -579,8 +567,7 @@ dns_rdataslab_merge(unsigned char *oslab, unsigned char *nslab, */ olength = 0; for (count = 0; count < ocount; count++) { - length = *ocurrent++ * 256; - length += *ocurrent++; + length = get_uint16(ocurrent); #if DNS_RDATASET_FIXED olength += length + 8; ocurrent += length + 2; @@ -679,7 +666,7 @@ dns_rdataslab_merge(unsigned char *oslab, unsigned char *nslab, ocurrent = ostart; INSIST(ocount != 0); #if DNS_RDATASET_FIXED - oorder = ocurrent[2] * 256 + ocurrent[3]; + oorder = peek_uint16(&ocurrent[2]); INSIST(oorder < ocount); #endif /* if DNS_RDATASET_FIXED */ rdata_from_slab(&ocurrent, rdclass, type, &ordata); @@ -693,7 +680,7 @@ dns_rdataslab_merge(unsigned char *oslab, unsigned char *nslab, do { dns_rdata_reset(&nrdata); #if DNS_RDATASET_FIXED - norder = ncurrent[2] * 256 + ncurrent[3]; + norder = peek_uint16(&ncurrent[2]); INSIST(norder < oncount); #endif /* if DNS_RDATASET_FIXED */ @@ -732,7 +719,7 @@ dns_rdataslab_merge(unsigned char *oslab, unsigned char *nslab, if (oadded < ocount) { dns_rdata_reset(&ordata); #if DNS_RDATASET_FIXED - oorder = ocurrent[2] * 256 + ocurrent[3]; + oorder = peek_uint16(&ocurrent[2]); INSIST(oorder < ocount); #endif /* if DNS_RDATASET_FIXED */ rdata_from_slab(&ocurrent, rdclass, type, @@ -760,8 +747,7 @@ dns_rdataslab_merge(unsigned char *oslab, unsigned char *nslab, do { dns_rdata_reset(&nrdata); #if DNS_RDATASET_FIXED - norder = ncurrent[2] * 256 + - ncurrent[3]; + norder = peek_uint16(&ncurrent[2]); INSIST(norder < oncount); #endif /* if DNS_RDATASET_FIXED */ rdata_from_slab(&ncurrent, rdclass, @@ -806,11 +792,9 @@ dns_rdataslab_subtract(unsigned char *mslab, unsigned char *sslab, REQUIRE(mslab != NULL && sslab != NULL); mcurrent = mslab + reservelen; - mcount = *mcurrent++ * 256; - mcount += *mcurrent++; + mcount = get_uint16(mcurrent); scurrent = sslab + reservelen; - scount = *scurrent++ * 256; - scount += *scurrent++; + scount = get_uint16(scurrent); INSIST(mcount > 0 && scount > 0); /* @@ -910,15 +894,14 @@ dns_rdataslab_subtract(unsigned char *mslab, unsigned char *sslab, * Copy the parts of mslab not in sslab. */ mcurrent = mslab + reservelen; - mcount = *mcurrent++ * 256; - mcount += *mcurrent++; + mcount = get_uint16(mcurrent); #if DNS_RDATASET_FIXED mcurrent += (4 * mcount); #endif /* if DNS_RDATASET_FIXED */ for (i = 0; i < mcount; i++) { unsigned char *mrdatabegin = mcurrent; #if DNS_RDATASET_FIXED - order = mcurrent[2] * 256 + mcurrent[3]; + order = peek_uint16(&mcurrent[2]); INSIST(order < mcount); #endif /* if DNS_RDATASET_FIXED */ rdata_from_slab(&mcurrent, rdclass, type, &mrdata); @@ -967,12 +950,10 @@ dns_rdataslab_equal(unsigned char *slab1, unsigned char *slab2, unsigned int length1, length2; current1 = slab1 + reservelen; - count1 = *current1++ * 256; - count1 += *current1++; + count1 = get_uint16(current1); current2 = slab2 + reservelen; - count2 = *current2++ * 256; - count2 += *current2++; + count2 = get_uint16(current2); if (count1 != count2) { return (false); @@ -983,12 +964,9 @@ dns_rdataslab_equal(unsigned char *slab1, unsigned char *slab2, current2 += (4 * count2); #endif /* if DNS_RDATASET_FIXED */ - while (count1 > 0) { - length1 = *current1++ * 256; - length1 += *current1++; - - length2 = *current2++ * 256; - length2 += *current2++; + while (count1-- > 0) { + length1 = get_uint16(current1); + length2 = get_uint16(current2); #if DNS_RDATASET_FIXED current1 += 2; @@ -1003,8 +981,6 @@ dns_rdataslab_equal(unsigned char *slab1, unsigned char *slab2, current1 += length1; current2 += length1; - - count1--; } return (true); } @@ -1019,12 +995,10 @@ dns_rdataslab_equalx(unsigned char *slab1, unsigned char *slab2, dns_rdata_t rdata2 = DNS_RDATA_INIT; current1 = slab1 + reservelen; - count1 = *current1++ * 256; - count1 += *current1++; + count1 = get_uint16(current1); current2 = slab2 + reservelen; - count2 = *current2++ * 256; - count2 += *current2++; + count2 = get_uint16(current2); if (count1 != count2) { return (false); @@ -1200,11 +1174,8 @@ rdataset_disassociate(dns_rdataset_t *rdataset DNS__DB_FLARG) { static isc_result_t rdataset_first(dns_rdataset_t *rdataset) { - unsigned char *raw = NULL; - unsigned int count; - - raw = rdataset->slab.raw; - count = raw[0] * 256 + raw[1]; + unsigned char *raw = rdataset->slab.raw; + uint16_t count = peek_uint16(raw); if (count == 0) { rdataset->slab.iter_pos = NULL; rdataset->slab.iter_count = 0; @@ -1232,11 +1203,7 @@ rdataset_first(dns_rdataset_t *rdataset) { static isc_result_t rdataset_next(dns_rdataset_t *rdataset) { - unsigned int count; - unsigned int length; - unsigned char *raw = NULL; - - count = rdataset->slab.iter_count; + uint16_t count = rdataset->slab.iter_count; if (count == 0) { rdataset->slab.iter_pos = NULL; return (ISC_R_NOMORE); @@ -1246,12 +1213,12 @@ rdataset_next(dns_rdataset_t *rdataset) { /* * Skip forward one record (length + 4) or one offset (4). */ - raw = rdataset->slab.iter_pos; + unsigned char *raw = rdataset->slab.iter_pos; #if DNS_RDATASET_FIXED if ((rdataset->attributes & DNS_RDATASETATTR_LOADORDER) == 0) #endif /* DNS_RDATASET_FIXED */ { - length = raw[0] * 256 + raw[1]; + uint16_t length = peek_uint16(raw); raw += length; } rdataset->slab.iter_pos = raw + DNS_RDATASET_ORDER + @@ -1284,7 +1251,7 @@ rdataset_current(dns_rdataset_t *rdataset, dns_rdata_t *rdata) { } #endif /* if DNS_RDATASET_FIXED */ - length = raw[0] * 256 + raw[1]; + length = peek_uint16(raw); raw += DNS_RDATASET_ORDER + DNS_RDATASET_LENGTH; @@ -1322,7 +1289,7 @@ rdataset_count(dns_rdataset_t *rdataset) { unsigned int count; raw = rdataset->slab.raw; - count = raw[0] * 256 + raw[1]; + count = get_uint16(raw); return (count); }