Page MenuHomePhabricator
Differential D4418 Diff 14020 src/seeder/test/seeder_tests.cpp

Changeset View

Standalone View

View Options

src/seeder/test/seeder_tests.cpp

// Copyright (c) 2019 The Bitcoin developers // Copyright (c) 2019 The Bitcoin developers
// Distributed under the MIT software license, see the accompanying // Distributed under the MIT software license, see the accompanying
// file COPYING or http://www.opensource.org/licenses/mit-license.php. // file COPYING or http://www.opensource.org/licenses/mit-license.php.
#define BOOST_TEST_MODULE Bitcoin Seeder Test Suite #define BOOST_TEST_MODULE Bitcoin Seeder Test Suite
#include <seeder/dns.h>
#include <boost/test/unit_test.hpp> #include <boost/test/unit_test.hpp>
#include <array>
#include <iomanip>
#include <iostream>
#include <sstream>
#include <string>
BOOST_AUTO_TEST_SUITE(seeder) BOOST_AUTO_TEST_SUITE(seeder)
const int BUFFER_LENGTH = 512;
const int QNAME_BUFFER_LENGTH = 256;
const int SIZE_OF_DNS_MESSAGE_HEADER = 12;
const int SIZE_OF_QTYPE = 2;
const int SIZE_OF_QCLASS = 2;
jasonbcoxUnsubmitted
Not Done
Inline Actions

It seemed a little suspicious to me that these constants would be defined in the test. But after looking at the existing seeder source, it's clear that these magic values are mostly hardcoded everywhere. So while this is the right approach for now (to test existing behavior), I expect at least some of these constants to eventually move to more appropriate locations.

jasonbcox: It seemed a little suspicious to me that these constants would be defined in the test. But…
// Builds dummy DNS query message
std::array<uint8_t, BUFFER_LENGTH> CreateDNSMessage(const std::string &qname) {
std::stringstream querryhex;
querryhex.clear();
querryhex << std::hex << std::setfill('0');
// ID
querryhex << std::setw(2) << static_cast<uint8_t>(0);
// QR to RD
querryhex << std::setw(1) << static_cast<uint8_t>(128);
// RA to RCODE
querryhex << std::setw(1) << static_cast<uint8_t>(0);
// QDCOUNT
querryhex << std::setw(2) << static_cast<uint8_t>(1);
// ANCOUNT
querryhex << std::setw(2) << static_cast<uint8_t>(0);
// NSCOUNT
querryhex << std::setw(2) << static_cast<uint8_t>(0);
// ARCOUNT
querryhex << std::setw(2) << static_cast<uint8_t>(0);
deadalnixUnsubmitted
Not Done
Inline Actions

Why all these static casts?

deadalnix: Why all these static casts?
deadalnixUnsubmitted
Not Done
Inline Actions

On a second note, why not just push strings instead of this?

deadalnix: On a second note, why not just push strings instead of this?
size_t i = 0;
size_t lastperiod = 0;
while (i < qname.size()) {
if (qname[i] == '.') {
querryhex << std::setw(1) << static_cast<uint8_t>(i - lastperiod);
while (lastperiod < i) {
querryhex << std::setw(1)
<< static_cast<uint8_t>(qname[lastperiod]);
lastperiod++;
}
lastperiod = i + 1;
}
i++;
}
querryhex << std::setw(1) << static_cast<uint8_t>(i - lastperiod);
while (lastperiod < i) {
querryhex << std::setw(1) << static_cast<uint8_t>(qname[lastperiod]);
lastperiod++;
}
// End of name field
querryhex << std::setw(1) << static_cast<uint8_t>(0);
// QTYPE = A query
querryhex << std::setw(2) << static_cast<uint8_t>(1);
// QCLASS = IN
querryhex << std::setw(2) << static_cast<uint8_t>(1);
std::array<uint8_t, BUFFER_LENGTH> messageBuffer;
messageBuffer.fill(0);
querryhex >> std::hex >> messageBuffer.data();
return messageBuffer;
}
BOOST_AUTO_TEST_CASE(parse_name_simple) { BOOST_AUTO_TEST_CASE(parse_name_simple) {
BOOST_CHECK_EQUAL(true, true); const std::string messageQName = "www.mydomain.com";
std::array<uint8_t, BUFFER_LENGTH> inbuf;
inbuf.fill(0);
inbuf = CreateDNSMessage(messageQName);
std::array<char, QNAME_BUFFER_LENGTH> qname;
qname.fill(0);
size_t insize = QNAME_BUFFER_LENGTH;
const uint8_t *inpos = inbuf.data() + SIZE_OF_DNS_MESSAGE_HEADER;
// +1 for the last octet ending the field name
const uint8_t *inend = inbuf.data() + SIZE_OF_DNS_MESSAGE_HEADER +
messageQName.size() + 1 + SIZE_OF_QTYPE +
SIZE_OF_QCLASS;
int ret = parse_name(&inpos, inend, inbuf.data(), qname.data(), insize);
BOOST_CHECK_EQUAL(ret, 0);
BOOST_CHECK_EQUAL(qname.data(), messageQName);
// Test for insufficient output buffer size
qname.fill(0);
inpos = inbuf.data() + SIZE_OF_DNS_MESSAGE_HEADER;
ret = parse_name(&inpos, inend, inbuf.data(), qname.data(),
messageQName.size());
BOOST_CHECK_EQUAL(ret, -2);
BOOST_CHECK_EQUAL(qname.data(),
messageQName.substr(0, messageQName.size() - 1));
// Test for premature end of input buffer
qname.fill(0);
inpos = inbuf.data() + SIZE_OF_DNS_MESSAGE_HEADER;
ret = parse_name(&inpos, inpos + 2, inbuf.data(), qname.data(), insize);
BOOST_CHECK_EQUAL(ret, -1);
BOOST_CHECK_EQUAL(qname.data(), messageQName.substr(0, 1));
// Test for when name field is too long
std::string longMessageQName = "www.";
for (size_t i = 0; i < 65; i++) {
longMessageQName += 'a';
}
longMessageQName += ".com";
qname.fill(0);
std::array<uint8_t, BUFFER_LENGTH> longInbuf;
longInbuf.fill(0);
longInbuf = CreateDNSMessage(longMessageQName);
const uint8_t *longInpos = longInbuf.data() + SIZE_OF_DNS_MESSAGE_HEADER;
const uint8_t *longInend = longInbuf.data() + SIZE_OF_DNS_MESSAGE_HEADER +
longMessageQName.size() + 1 + SIZE_OF_QTYPE +
SIZE_OF_QCLASS;
ret = parse_name(&longInpos, longInend, longInbuf.data(), qname.data(),
insize);
BOOST_CHECK_EQUAL(ret, -1);
deadalnixUnsubmitted
Not Done
Inline Actions

Honestly, this test is very hard to follow. The variable names are not meaningful (qname? inbuf?). There are magic numbers everywhere. Almost everything is mutated thourough the test such as it means different things at different points in time.

The whole thing is convoluted for no apparent reason. For instance, what does this achieve:

std::array<uint8_t, BUFFER_LENGTH> longInbuf;
longInbuf.fill(0);
longInbuf = CreateDNSMessage(longMessageQName);

over

std::array<uint8_t, BUFFER_LENGTH> longInbuf = CreateDNSMessage(longMessageQName);

?

I think this code would benefit a lot from a few passes of self review.

deadalnix: Honestly, this test is very hard to follow. The variable names are not meaningful (qname? inbuf?
} }
BOOST_AUTO_TEST_SUITE_END() BOOST_AUTO_TEST_SUITE_END()