From b335522ce74a26eaefb28c9594431152a83ba3db Mon Sep 17 00:00:00 2001 From: Brad House Date: Thu, 14 Nov 2024 09:06:26 -0500 Subject: [PATCH] Empty TXT records were not being preserved (#922) When parsing, preserve empty TXT records. Fixes #921 Fix-By: Brad House (@bradh352) --- src/lib/ares__buf.c | 46 +++++++++++++++++---------------- src/lib/ares_dns_multistring.c | 11 ++++++++ test/ares-test-parse-txt.cc | 47 +++++++++++++++++++++++++++++----- 3 files changed, 75 insertions(+), 29 deletions(-) diff --git a/src/lib/ares__buf.c b/src/lib/ares__buf.c index c3d52430..997e54cd 100644 --- a/src/lib/ares__buf.c +++ b/src/lib/ares__buf.c @@ -971,36 +971,38 @@ ares_status_t ares__buf_parse_dns_abinstr(ares__buf_t *buf, break; /* LCOV_EXCL_LINE: DefensiveCoding */ } - if (len) { - /* When used by the _str() parser, it really needs to be validated to - * be a valid printable ascii string. Do that here */ - if (validate_printable && ares__buf_len(buf) >= len) { - size_t mylen; - const char *data = (const char *)ares__buf_peek(buf, &mylen); - if (!ares__str_isprint(data, len)) { - status = ARES_EBADSTR; - break; - } + + /* When used by the _str() parser, it really needs to be validated to + * be a valid printable ascii string. Do that here */ + if (len && validate_printable && ares__buf_len(buf) >= len) { + size_t mylen; + const char *data = (const char *)ares__buf_peek(buf, &mylen); + if (!ares__str_isprint(data, len)) { + status = ARES_EBADSTR; + break; } + } - if (strs != NULL) { - unsigned char *data = NULL; + if (strs != NULL) { + unsigned char *data = NULL; + if (len) { status = ares__buf_fetch_bytes_dup(buf, len, ARES_TRUE, &data); if (status != ARES_SUCCESS) { break; } - status = ares__dns_multistring_add_own(*strs, data, len); - if (status != ARES_SUCCESS) { - ares_free(data); - break; - } - } else { - status = ares__buf_consume(buf, len); - if (status != ARES_SUCCESS) { - break; - } + } + status = ares__dns_multistring_add_own(*strs, data, len); + if (status != ARES_SUCCESS) { + ares_free(data); + break; + } + } else { + status = ares__buf_consume(buf, len); + if (status != ARES_SUCCESS) { + break; } } + } if (status != ARES_SUCCESS && strs != NULL) { diff --git a/src/lib/ares_dns_multistring.c b/src/lib/ares_dns_multistring.c index b81c46c0..b51e4bf6 100644 --- a/src/lib/ares_dns_multistring.c +++ b/src/lib/ares_dns_multistring.c @@ -138,6 +138,17 @@ ares_status_t ares__dns_multistring_add_own(ares__dns_multistring_t *strs, strs->alloc = newalloc; } + /* Issue #921, ares_dns_multistring_get() doesn't have a way to indicate + * success or fail on a zero-length string which is actually valid. So we + * are going to allocate a 1-byte buffer to use as a placeholder in this + * case */ + if (str == NULL) { + str = ares_malloc_zero(1); + if (str == NULL) { + return ARES_ENOMEM; + } + } + strs->strs[strs->cnt].data = str; strs->strs[strs->cnt].len = len; strs->cnt++; diff --git a/test/ares-test-parse-txt.cc b/test/ares-test-parse-txt.cc index 173d0bde..07b77adb 100644 --- a/test/ares-test-parse-txt.cc +++ b/test/ares-test-parse-txt.cc @@ -38,7 +38,7 @@ TEST_F(LibraryTest, ParseTxtReplyOK) { std::string expected2a = "txt2a"; std::string expected2b("ABC\0ABC", 7); pkt.set_qid(0x1234).set_response().set_aa() - .add_question(new DNSQuestion("example.com", T_MX)) + .add_question(new DNSQuestion("example.com", T_TXT)) .add_answer(new DNSTxtRR("example.com", 100, {expected1})) .add_answer(new DNSTxtRR("example.com", 100, {expected2a, expected2b})); std::vector data = pkt.data(); @@ -68,7 +68,7 @@ TEST_F(LibraryTest, ParseTxtExtReplyOK) { std::string expected2a = "txt2a"; std::string expected2b("ABC\0ABC", 7); pkt.set_qid(0x1234).set_response().set_aa() - .add_question(new DNSQuestion("example.com", T_MX)) + .add_question(new DNSQuestion("example.com", T_TXT)) .add_answer(new DNSTxtRR("example.com", 100, {expected1})) .add_answer(new DNSTxtRR("example.com", 100, {expected2a, expected2b})); std::vector data = pkt.data(); @@ -95,6 +95,39 @@ TEST_F(LibraryTest, ParseTxtExtReplyOK) { ares_free_data(txt); } +TEST_F(LibraryTest, ParseTxtEmpty) { + DNSPacket pkt; + std::string expected1 = ""; + pkt.set_qid(0x1234).set_response().set_aa() + .add_question(new DNSQuestion("example.com", T_TXT)) + .add_answer(new DNSTxtRR("example.com", 100, {expected1})); + std::vector data = pkt.data(); + + ares_dns_record_t *dnsrec = NULL; + ares_dns_rr_t *rr = NULL; + EXPECT_EQ(ARES_SUCCESS, ares_dns_parse(data.data(), data.size(), 0, &dnsrec)); + EXPECT_EQ(1, ares_dns_record_rr_cnt(dnsrec, ARES_SECTION_ANSWER)); + rr = ares_dns_record_rr_get(dnsrec, ARES_SECTION_ANSWER, 0); + ASSERT_NE(nullptr, rr); + EXPECT_EQ(ARES_REC_TYPE_TXT, ares_dns_rr_get_type(rr)); + + size_t txtdata_len; + const unsigned char *txtdata; + + /* Using array methodology */ + EXPECT_EQ(1, ares_dns_rr_get_abin_cnt(rr, ARES_RR_TXT_DATA)); + txtdata = ares_dns_rr_get_abin(rr, ARES_RR_TXT_DATA, 0, &txtdata_len); + EXPECT_EQ(txtdata_len, 0); + EXPECT_NE(nullptr, txtdata); + + /* Using combined methodology */ + txtdata = ares_dns_rr_get_bin(rr, ARES_RR_TXT_DATA, &txtdata_len); + EXPECT_EQ(txtdata_len, 0); + EXPECT_NE(nullptr, txtdata); + + ares_dns_record_destroy(dnsrec); dnsrec = NULL; +} + TEST_F(LibraryTest, ParseTxtMalformedReply1) { std::vector data = { 0x12, 0x34, // qid @@ -213,7 +246,7 @@ TEST_F(LibraryTest, ParseTxtReplyErrors) { std::string expected2a = "txt2a"; std::string expected2b = "txt2b"; pkt.set_qid(0x1234).set_response().set_aa() - .add_question(new DNSQuestion("example.com", T_MX)) + .add_question(new DNSQuestion("example.com", T_TXT)) .add_answer(new DNSTxtRR("example.com", 100, {expected1})) .add_answer(new DNSTxtRR("example.com", 100, {expected1})) .add_answer(new DNSTxtRR("example.com", 100, {expected2a, expected2b})); @@ -227,7 +260,7 @@ TEST_F(LibraryTest, ParseTxtReplyErrors) { txt = nullptr; EXPECT_EQ(ARES_EBADRESP, ares_parse_txt_reply(data.data(), (int)data.size(), &txt)); EXPECT_EQ(nullptr, txt); - pkt.add_question(new DNSQuestion("example.com", T_MX)); + pkt.add_question(new DNSQuestion("example.com", T_TXT)); #ifdef DISABLED // Question != answer @@ -240,13 +273,13 @@ TEST_F(LibraryTest, ParseTxtReplyErrors) { #endif // Two questions. - pkt.add_question(new DNSQuestion("example.com", T_MX)); + pkt.add_question(new DNSQuestion("example.com", T_TXT)); data = pkt.data(); txt = nullptr; EXPECT_EQ(ARES_EBADRESP, ares_parse_txt_reply(data.data(), (int)data.size(), &txt)); EXPECT_EQ(nullptr, txt); pkt.questions_.clear(); - pkt.add_question(new DNSQuestion("example.com", T_MX)); + pkt.add_question(new DNSQuestion("example.com", T_TXT)); // No answer. pkt.answers_.clear(); @@ -274,7 +307,7 @@ TEST_F(LibraryTest, ParseTxtReplyAllocFail) { std::string expected2a = "txt2a"; std::string expected2b = "txt2b"; pkt.set_qid(0x1234).set_response().set_aa() - .add_question(new DNSQuestion("example.com", T_MX)) + .add_question(new DNSQuestion("example.com", T_TXT)) .add_answer(new DNSCnameRR("example.com", 300, "c.example.com")) .add_answer(new DNSTxtRR("c.example.com", 100, {expected1})) .add_answer(new DNSTxtRR("c.example.com", 100, {expected1}))