From 2d34cf0c16dd8fa71fb593e65ce4734cb61d9170 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Zbigniew=20J=C4=99drzejewski-Szmek?= Date: Wed, 27 Feb 2019 07:01:50 +0100 Subject: [PATCH 1/4] resolved: use a temporary Set to speed up dns question parsing This doesn't necessarily make things faster, because we still spend more time in dns_answer_add(), but it improves the compuational complexity of this part. If we even make dns_resource_key_equal_faster, this will become worthwhile. --- src/resolve/resolved-dns-packet.c | 21 ++++++++++++++++++++- src/resolve/resolved-dns-question.c | 22 +++++++++++++++------- src/resolve/resolved-dns-question.h | 1 + 3 files changed, 36 insertions(+), 8 deletions(-) diff --git a/src/resolve/resolved-dns-packet.c b/src/resolve/resolved-dns-packet.c index 572271be95..ff58a9d810 100644 --- a/src/resolve/resolved-dns-packet.c +++ b/src/resolve/resolved-dns-packet.c @@ -7,6 +7,7 @@ #include "alloc-util.h" #include "dns-domain.h" #include "resolved-dns-packet.h" +#include "set.h" #include "string-table.h" #include "strv.h" #include "unaligned.h" @@ -2133,6 +2134,17 @@ static int dns_packet_extract_question(DnsPacket *p, DnsQuestion **ret_question) if (!question) return -ENOMEM; + _cleanup_set_free_ Set *keys = NULL; /* references to keys are kept by Question */ + + keys = set_new(&dns_resource_key_hash_ops); + if (!keys) + return log_oom(); + + r = set_reserve(keys, n * 2); /* Higher multipliers give slightly higher efficiency through + * hash collisions, but the gains quickly drop of after 2. */ + if (r < 0) + return r; + for (i = 0; i < n; i++) { _cleanup_(dns_resource_key_unrefp) DnsResourceKey *key = NULL; bool cache_flush; @@ -2147,7 +2159,14 @@ static int dns_packet_extract_question(DnsPacket *p, DnsQuestion **ret_question) if (!dns_type_is_valid_query(key->type)) return -EBADMSG; - r = dns_question_add(question, key); + r = set_put(keys, key); + if (r < 0) + return r; + if (r == 0) + /* Already in the Question, let's skip */ + continue; + + r = dns_question_add_raw(question, key); if (r < 0) return r; } diff --git a/src/resolve/resolved-dns-question.c b/src/resolve/resolved-dns-question.c index 1ed9171564..60cd34bcfc 100644 --- a/src/resolve/resolved-dns-question.c +++ b/src/resolve/resolved-dns-question.c @@ -32,8 +32,20 @@ static DnsQuestion *dns_question_free(DnsQuestion *q) { DEFINE_TRIVIAL_REF_UNREF_FUNC(DnsQuestion, dns_question, dns_question_free); +int dns_question_add_raw(DnsQuestion *q, DnsResourceKey *key) { + /* Insert without checking for duplicates. */ + + assert(key); + assert(q); + + if (q->n_keys >= q->n_allocated) + return -ENOSPC; + + q->keys[q->n_keys++] = dns_resource_key_ref(key); + return 0; +} + int dns_question_add(DnsQuestion *q, DnsResourceKey *key) { - size_t i; int r; assert(key); @@ -41,7 +53,7 @@ int dns_question_add(DnsQuestion *q, DnsResourceKey *key) { if (!q) return -ENOSPC; - for (i = 0; i < q->n_keys; i++) { + for (size_t i = 0; i < q->n_keys; i++) { r = dns_resource_key_equal(q->keys[i], key); if (r < 0) return r; @@ -49,11 +61,7 @@ int dns_question_add(DnsQuestion *q, DnsResourceKey *key) { return 0; } - if (q->n_keys >= q->n_allocated) - return -ENOSPC; - - q->keys[q->n_keys++] = dns_resource_key_ref(key); - return 0; + return dns_question_add_raw(q, key); } int dns_question_matches_rr(DnsQuestion *q, DnsResourceRecord *rr, const char *search_domain) { diff --git a/src/resolve/resolved-dns-question.h b/src/resolve/resolved-dns-question.h index f513bf0328..0803f49b8b 100644 --- a/src/resolve/resolved-dns-question.h +++ b/src/resolve/resolved-dns-question.h @@ -22,6 +22,7 @@ int dns_question_new_address(DnsQuestion **ret, int family, const char *name, bo int dns_question_new_reverse(DnsQuestion **ret, int family, const union in_addr_union *a); int dns_question_new_service(DnsQuestion **ret, const char *service, const char *type, const char *domain, bool with_txt, bool convert_idna); +int dns_question_add_raw(DnsQuestion *q, DnsResourceKey *key); int dns_question_add(DnsQuestion *q, DnsResourceKey *key); int dns_question_matches_rr(DnsQuestion *q, DnsResourceRecord *rr, const char *search_domain); From 51969a5893f0344bf4fa2122ac30476ac30b9468 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Zbigniew=20J=C4=99drzejewski-Szmek?= Date: Wed, 27 Feb 2019 10:27:35 +0100 Subject: [PATCH 2/4] resolve: split the RR comparison function in two No functional change. --- src/resolve/resolved-dns-rr.c | 28 ++++++++++++++++++---------- src/resolve/resolved-dns-rr.h | 2 ++ 2 files changed, 20 insertions(+), 10 deletions(-) diff --git a/src/resolve/resolved-dns-rr.c b/src/resolve/resolved-dns-rr.c index 477d5170aa..4cbb9723e2 100644 --- a/src/resolve/resolved-dns-rr.c +++ b/src/resolve/resolved-dns-rr.c @@ -557,18 +557,10 @@ int dns_resource_record_new_address(DnsResourceRecord **ret, int family, const u ((a).field ## _size == (b).field ## _size && \ memcmp((a).field, (b).field, (a).field ## _size) == 0) -int dns_resource_record_equal(const DnsResourceRecord *a, const DnsResourceRecord *b) { +int dns_resource_record_payload_equal(const DnsResourceRecord *a, const DnsResourceRecord *b) { int r; - assert(a); - assert(b); - - if (a == b) - return 1; - - r = dns_resource_key_equal(a->key, b->key); - if (r <= 0) - return r; + /* Check if a and b are the same, but don't look at their keys */ if (a->unparseable != b->unparseable) return 0; @@ -692,6 +684,22 @@ int dns_resource_record_equal(const DnsResourceRecord *a, const DnsResourceRecor } } +int dns_resource_record_equal(const DnsResourceRecord *a, const DnsResourceRecord *b) { + int r; + + assert(a); + assert(b); + + if (a == b) + return 1; + + r = dns_resource_key_equal(a->key, b->key); + if (r <= 0) + return r; + + return dns_resource_record_payload_equal(a, b); +} + static char* format_location(uint32_t latitude, uint32_t longitude, uint32_t altitude, uint8_t size, uint8_t horiz_pre, uint8_t vert_pre) { char *s; diff --git a/src/resolve/resolved-dns-rr.h b/src/resolve/resolved-dns-rr.h index e02fa2d53b..3d27836651 100644 --- a/src/resolve/resolved-dns-rr.h +++ b/src/resolve/resolved-dns-rr.h @@ -308,6 +308,8 @@ DnsResourceRecord* dns_resource_record_unref(DnsResourceRecord *rr); int dns_resource_record_new_reverse(DnsResourceRecord **ret, int family, const union in_addr_union *address, const char *name); int dns_resource_record_new_address(DnsResourceRecord **ret, int family, const union in_addr_union *address, const char *name); int dns_resource_record_equal(const DnsResourceRecord *a, const DnsResourceRecord *b); +int dns_resource_record_payload_equal(const DnsResourceRecord *a, const DnsResourceRecord *b); + const char* dns_resource_record_to_string(DnsResourceRecord *rr); DnsResourceRecord *dns_resource_record_copy(DnsResourceRecord *rr); DEFINE_TRIVIAL_CLEANUP_FUNC(DnsResourceRecord*, dns_resource_record_unref); From dffb8277728b8052258bff49d7157fc4e326f88f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Zbigniew=20J=C4=99drzejewski-Szmek?= Date: Wed, 27 Feb 2019 10:37:40 +0100 Subject: [PATCH 3/4] resolved: when adding RR to an answer, avoid comparing keys twice We'd call dns_resource_record_equal(), which calls dns_resource_key_equal() internally, and then dns_resource_key_equal() a second time. Let's be a bit smarter, and call dns_resource_key_equal() only once. (before) dns_resource_key_hash_func_count=514 dns_resource_key_compare_func_count=275 dns_resource_key_equal_count=62371 4.13s user 0.01s system 99% cpu 4.153 total (after) dns_resource_key_hash_func_count=514 dns_resource_key_compare_func_count=276 dns_resource_key_equal_count=31337 2.13s user 0.01s system 99% cpu 2.139 total --- src/resolve/resolved-dns-answer.c | 51 +++++++++++++------------------ src/resolve/resolved-dns-packet.c | 2 +- 2 files changed, 23 insertions(+), 30 deletions(-) diff --git a/src/resolve/resolved-dns-answer.c b/src/resolve/resolved-dns-answer.c index d7252d3dac..44dc6552b4 100644 --- a/src/resolve/resolved-dns-answer.c +++ b/src/resolve/resolved-dns-answer.c @@ -87,40 +87,33 @@ int dns_answer_add(DnsAnswer *a, DnsResourceRecord *rr, int ifindex, DnsAnswerFl if (a->items[i].ifindex != ifindex) continue; - r = dns_resource_record_equal(a->items[i].rr, rr); - if (r < 0) - return r; - if (r > 0) { - /* Don't mix contradicting TTLs (see below) */ - if ((rr->ttl == 0) != (a->items[i].rr->ttl == 0)) - return -EINVAL; - - /* Entry already exists, keep the entry with - * the higher RR. */ - if (rr->ttl > a->items[i].rr->ttl) { - dns_resource_record_ref(rr); - dns_resource_record_unref(a->items[i].rr); - a->items[i].rr = rr; - } - - a->items[i].flags |= flags; - return 0; - } - r = dns_resource_key_equal(a->items[i].rr->key, rr->key); if (r < 0) return r; - if (r > 0) { - /* There's already an RR of the same RRset in - * place! Let's see if the TTLs more or less - * match. We don't really care if they match - * precisely, but we do care whether one is 0 - * and the other is not. See RFC 2181, Section - * 5.2. */ + if (r == 0) + continue; - if ((rr->ttl == 0) != (a->items[i].rr->ttl == 0)) - return -EINVAL; + /* There's already an RR of the same RRset in place! Let's see if the TTLs more or less + * match. We don't really care if they match precisely, but we do care whether one is 0 and + * the other is not. See RFC 2181, Section 5.2. */ + if ((rr->ttl == 0) != (a->items[i].rr->ttl == 0)) + return -EINVAL; + + r = dns_resource_record_payload_equal(a->items[i].rr, rr); + if (r < 0) + return r; + if (r == 0) + continue; + + /* Entry already exists, keep the entry with the higher RR. */ + if (rr->ttl > a->items[i].rr->ttl) { + dns_resource_record_ref(rr); + dns_resource_record_unref(a->items[i].rr); + a->items[i].rr = rr; } + + a->items[i].flags |= flags; + return 0; } return dns_answer_add_raw(a, rr, ifindex, flags); diff --git a/src/resolve/resolved-dns-packet.c b/src/resolve/resolved-dns-packet.c index ff58a9d810..278546da84 100644 --- a/src/resolve/resolved-dns-packet.c +++ b/src/resolve/resolved-dns-packet.c @@ -2258,7 +2258,7 @@ static int dns_packet_extract_answer(DnsPacket *p, DnsAnswer **ret_answer) { * be contained in questions, never in replies. Crappy * Belkin routers copy the OPT data for example, hence let's * detect this so that we downgrade early. */ - log_debug("OPT RR contained RFC6975 data, ignoring."); + log_debug("OPT RR contains RFC6975 data, ignoring."); bad_opt = true; continue; } From f27abfccd0e78b53069d0d19bcf40aa91f67d14d Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Zbigniew=20J=C4=99drzejewski-Szmek?= Date: Wed, 27 Feb 2019 13:09:37 +0100 Subject: [PATCH 4/4] fuzz-dns-packet: add test case with lots of labels https://bugs.chromium.org/p/oss-fuzz/issues/detail?id=13422 --- test/fuzz/fuzz-dns-packet/oss-fuzz-13422 | Bin 0 -> 36510 bytes 1 file changed, 0 insertions(+), 0 deletions(-) create mode 100644 test/fuzz/fuzz-dns-packet/oss-fuzz-13422 diff --git a/test/fuzz/fuzz-dns-packet/oss-fuzz-13422 b/test/fuzz/fuzz-dns-packet/oss-fuzz-13422 new file mode 100644 index 0000000000000000000000000000000000000000..439aec54e39683d952a81c3adcaccb633c2469df GIT binary patch literal 36510 zcmeI5TaO&Y6~|{C8!Y0NNT4VJM@C7IAkgZrzK$g#*6ZD;C=wDcQKTrdb4e`L*usX8 zmnb~)kgpKsi$wVQlUT4KW)yLhbFY5Q251MRAn$-&Hx9iuhE?2Sn;jYge99ILbvGO^w`H8^S zu$`w}mr%p)z?IJMXa2QW_|KZBjfW5_8(^KsHI5MS!#5w?U$`50Ke~CGjI+Yc_A7Ve zjmgQ$8fx{_to=vL(>LWID2OX0JbZWf;0Xxg4ztuCUs2YUBCwv!TKPqC_B(5?FOtY# zcCM#bb^1_ROZg5@Us136*0A|`V}8%oh9_UA4S!)1-}8Qcc{xGXq-}Wh9r(Tmxc~{+ zfg;~Lq|RNe*DP8+?LfwJ0r}24DCph^6o!1F5&P2z31_d^tt5B1w+paDr;EOHuD&S! zI(<pUyDap!(^Gmnxgk1~FeI@9Wd zD*+rw)98@mX)R7G&! zapcBP<$&&oBTscjax^dFtV-hBz2gCwPZrr?d^}$q2j~pk^v;8+t7iE$$>*W*X1C%+ z<%%j9$HyVK`Q6ET(>wPk5AWQ2F9i4gy$AF6-o5u>S)|i0-1V1M@-n&U4vOsfR(vbT zrne76fMtAp79GYjH=mHo2S(g4<9t5J5B8IBH7x^x)5*ih$)6rPj1Jwx5 z%EHJLnIM_k;&s7zOmAZiVRA&Ggq-22wZJ0E0Qc;68*A1F#u!jb6{;u)ojloM;X5O` zXsqee$dDccEJYR5s4`RmKt>bN24_&K1Sywi3z7$60GR}K7h&=sji8d@J>qR*rn==) z%v@YZUB`&HF(QCVE@lQG(v4ChY=$bh1oGgZ;dPjfPO1tIS!dX!0Eo&sOk@#OS4OJm4mL8(84NQP7lp_Sg2NcZ3|QZzx8xD)l*s0p_7St3LTzG`)WDhu0lZmg z8YVcJ4_t`^#bNTKkQx;ff5KDCE+dUhAMk~8A893-NEkQ`n?00D`Y&X{>Y7Cr!kudY z;WbKhs5f(o&JQoUG$^>Af!UjMZ4cf8;VPvo*;)>Kdtge8t zMizjI1tlCkEtgtjAR`aQI#Gqxn3{P(22<(>{Vi2kDL=ZHhYcxQMhysqi57y$fJgx; zp`)?J0zK6N;ZB%5#1pd1u-p(coNEHJAyOdFlqvw2)-ga*hUF1VV2%)~!t%)SBs_vO z0EM#Dfw&?_N9iE?3i1Zl03s7EH-k9LNd>X-o2$LFjSR0P6w+$Y0XPpEDa3�*X2W zk-_~cgM#IT?U5Oi_9MSxd8{y@GCUAIY;loAMuROTAf_4epta*LXT}f^F7mGoYaxLA zI3*Tg@(^Ll$zbY@D21>mks-y?7+8Zgnx*)7+_$hm*rBkJI3Yv-x83!AUjF(TDydm#G)CgQMHC#iYoytlyde_`!n| zNBoCLS}y!>=Ev8Y{5riAN3(K~-Wt#1%FVhI-+-i`P{esOt7hYJI``1ofILVh`_p4L ziV9)&g_5A&mP|8|;AtISowb)M3^JWOs)W*#3m#%$8|GOn`Rlc7z>#q@YKp3JK< zUBpLTHDwc$BFVkz?&Lu>i|5B4hc+PZPs+5KMsC5lS>m~{22$=pfhjtR9U_wLrF@-| z6Gkel!a=0{xHphWZ%CoN_@Xs?5@U#D|4?&l_9`mS@HpiQ;dNp(lqC|Fk3Ij?o}C;+ zh%=_2jdUPoRt0L0DablpD?Dq|8I#H)R+E@1HX!9Cu!h7Xey-Ra6R$jGnHX4$o)iNW z@W-R_oCjrx!>4O9ya z9uafErkbY~BsF!kBZ2Y;mFhJTRup`pM?P0PQZ9vzib&R&kVms`3$WX`tD?wwvJ3{5 zh~S)%mvIQM6DzMm67n1y>m*N7B0vV~?^1Nu$tsoo5EmO%qP$0RPc6X3hD~NjtPSu) zr(uj#dxcLZvLD2@*C~4po#0Vs&5jteQ$p5aNuBa4P*-SfctfTo3{t_wqEd-?U~ENE zgPSGl3)2a?Eh8f42dcYcMiGbVLl22=&k&<^~INQ#)6vx7S|3`F0=y)I4=iZ~g(xf`s(e!I5#NnC zL}|UEKi<|`FAr9G^_|&8b84VSUDz&ZD?r6=9H)_Y)SCuqoBU?jdOPGzR*?dzzoFAv z=dzwdQW3}^l^U#S6i67FkgC3tDeM zB6m91VMJE!F|fI!FpR=hcubI1HbMQ;h#Bgr28I&T`}N&6H(4R^mqI(kdOJ#Kdeko3j=DmE}YBMF&*L|+{00j_O=+0cxg*bwmmzLwIn0?1~-YQ zmF;3i$HqG4+?g7S=M4FTn6k%E-ogkI0;IBIBTi=PQJJxH*~rS}Hfk?}9vOZUNHSC6 z5ROm<`Ht;IF_;<1up~?5(F|EpLz0D#oI9-Y_u^XI6r{ESm2ZZ5q@BeY9n*pdl5DRI zt-{>YrXsdMLSodHv_^%0pycmsv@ybIqe;XpTecV5kRl}R6-dLO6vsu?!ui!SHd~ zHGr5*Zso<=ny=3zC{GA#hayVhP5$nmx@l3V-cDkjMCwMY2&C+sGAX07r!+#iGrbXe ztLz#}lv!uk($)o9N%FSc*fvI(1R@}9$QX56xfiS~bx5kfL3*kRB?D6HeHaqFk)_%t z8Dk@t@K% z#WpX#ZkjjD$TMyG?M&)m}x$hP-5EYE^-o7%o;FcOMaq*J5F5KA< zwLiW#A?&46rIU(l)c4H~`^daX}aLXfi~`lRuE2KJxf-_;K6Tb)<>y9MU#!Q&v-XbnaPPI(>+wGO z;)}m&ywx!i6#V4g?`yBeI^JH6HwujRyOUpySl>e?+xA*x%1VE`#8jg_D{TMw_?f*& zZ(i2-u1WNE5z%ch+a`WWK1){X`2Hu!U-s^dMox74`K8wCk{|r?k)+^ZJgA(&;{Jy`=7hr&Rs!$YcA;yO@G!+!pbq~(nJSv=v>mah;8z4ppO$V z+Eew8#?3_c#*C zxi9a$LVH9Vj~Y6cZ1aUO_Bw0Bfv2H!$#!3eLhHT+XN~*=5iK!;hC}C)R(8-L4`*R? zYFtO5yhB4Rz~yl*ok;fYy|e-GmYnU;&C;-G=2%bNp+cw7f+l6?ToNP?iQCY*q+h~0 z2s3mp*-jqP+o5xb*B7I)W{_v-TrzYn=^EeGDbe~ID_INSnsK{g=v*>%F7b*3D_I&k zmnfOnd1oEV58u!dBsu2Lxg>0txQQ@yF0t;Qs5YdVPt@*=n`F9AK6Eaz?!I$_Z0KA< zEv?Vauljrpol8_n96Fb5?%B4YN=%ZfTe{plbS~+q5_j@cL>fAmboT$-vZMwIs_$## zDHt=Ps+mF0pP)bK!aD zT!QEJHs)H^X6Rg^h8#oZl74HWzMn(q603X1(79yjToSglL+6sAbIH)Tq&bhZ*$JiR z|HpZH$P>^|