Page MenuHomePhabricator

Enforce maximum name length for parse_name() and add unit tests
ClosedPublic

Authored by nakihito on Feb 25 2020, 21:48.

Details

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

Diff Detail

Repository
rABC Bitcoin ABC
Branch
ParseNameUnitTest-5
Lint
Lint Passed
Unit
No Test Coverage
Build Status
Buildable 9646
Build 17192: Default Diff Build & Tests
Build 17191: arc lint + arc unit

Event Timeline

jasonbcox requested changes to this revision.Feb 25 2020, 22:57
jasonbcox added inline comments.
src/seeder/test/seeder_tests.cpp
115 ↗(On Diff #16505)

The this-and-that style naming is a hint that two things are being tested. The test may look organized now, but it is likely to degrade as new cases are added.

129 ↗(On Diff #16505)

I can see why you're reusing maxLengthLabel here, but consider our discussion from earlier today where duplicating code in tests shouldn't necessarily be frowned upon. That coupled with https://reviews.bitcoinabc.org/D5328#128936 easily makes these cases not dependent on the above code.

This revision now requires changes to proceed.Feb 25 2020, 22:57

Separated qname length tests from label tests.

This revision is now accepted and ready to land.Feb 26 2020, 00:51

Just a small comment/quibble: The name of the Diff seems misleading to me, it makes it sound like it's just adding a test, not changing behavior. But isn't it actually changing how the code work also?

Maybe a better name would be "Add maximum name length for parse_name()", or something similar.

Just a small comment/quibble: The name of the Diff seems misleading to me, it makes it sound like it's just adding a test, not changing behavior. But isn't it actually changing how the code work also?

Maybe a better name would be "Add maximum name length for parse_name()", or something similar.

Yes and no. So the dns protocol only actually allows names up to 255 characters in length. So even though the function doesn't enforce this, it should never actually occur. I'll go ahead and change the title since this could cause confusion if someone were to be looking back through the history.

nakihito retitled this revision from Add maximum name length unit test for parse_name() to Enforce maximum name length for parse_name() and add unit tests.Feb 26 2020, 21:50