Skip to content

Commit

Permalink
Enforce maximum name length for parse_name() and add unit tests
Browse files Browse the repository at this point in the history
Summary:
Adds unit tests for the maximum length of a query name for the seeder's
`parse_name()` function.

Currently, parse_name() has a logical flaw that prevents testing above the
maximum query name length.  The length of a query name is the sum of its labels
plus 1 for each `.`.  The maximum length of a query name is 255 characters. This
is not strictly enforced in parse_name() and returns a misleading error when it
occurs. This patch fixes this and adds a unit test for it.

Depends on D5328

Test Plan:
  make check
  ninja check

Reviewers: O1 Bitcoin ABC, #bitcoin_abc, deadalnix, Fabien, jasonbcox

Reviewed By: O1 Bitcoin ABC, #bitcoin_abc, jasonbcox

Subscribers: Mengerian

Differential Revision: https://reviews.bitcoinabc.org/D5329
  • Loading branch information
Nico Guiton authored and ftrader committed Apr 24, 2020
1 parent 8bfa083 commit 041694e
Show file tree
Hide file tree
Showing 2 changed files with 21 additions and 1 deletion.
7 changes: 6 additions & 1 deletion src/seeder/dns.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -78,6 +78,11 @@ int parse_name(const uint8_t **inpos, const uint8_t *inend,
}
// add dot in output
if (!init) {
// The maximum size of a query name is 255. The buffer must have
// room for at least the '.' and a valid non-'.' character
if (bufused > 253) {
return -1;
}
if (bufused == bufsize - 1) {
return -2;
}
Expand All @@ -103,7 +108,7 @@ int parse_name(const uint8_t **inpos, const uint8_t *inend,
}
// copy label
while (octet) {
if (*inpos == inend) {
if (*inpos == inend || bufused > 254) {
return -1;
}
if (bufused == bufsize - 1) {
Expand Down
15 changes: 15 additions & 0 deletions src/seeder/test/dns_tests.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -119,4 +119,19 @@ BOOST_AUTO_TEST_CASE(parse_name_label_tests) {
CheckParseNameError("www." + maxLengthLabel + "a.com", -1);
}

BOOST_AUTO_TEST_CASE(parse_name_qname_length_tests) {
const std::string maxLengthLabel(MAX_LABEL_LENGTH, 'a');

// Check behavior for a name that is the maximum length
std::string maxLengthQName = maxLengthLabel + '.' + maxLengthLabel + '.' +
maxLengthLabel + '.' + maxLengthLabel;
BOOST_CHECK_EQUAL(maxLengthQName.size(), MAX_QUERY_NAME_LENGTH);
CheckParseName(maxLengthQName);

// Check that a query name that is too long causes an error
// Allocates an extra large buffer to guarantee an error is not caused by
// the buffer size
CheckParseNameError(maxLengthQName + "a", -1, 2 * maxLengthQName.size());
}

BOOST_AUTO_TEST_SUITE_END()

0 comments on commit 041694e

Please sign in to comment.