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

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

Details

Reviewers

deadalnix
Fabien
jasonbcox

Group Reviewers

Restricted Owners Package	(Owns No Changed Paths)
Restricted Project

Commits

rSTAGING1c0566ce001c: Enforce maximum name length for parse_name() and add unit tests
rABC1c0566ce001c: Enforce maximum name length for parse_name() and add unit tests

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 9572
Build 17048: Default Diff Build & Tests
Build 17047: arc lint + arc unit

Event Timeline

nakihito created this revision.Feb 25 2020, 21:48

Build Bitcoin-ABC / Diffs / Diff Testing started.

nakihito mentioned this in D5241: Make parse_name() properly fail when the query name is longer than 255 characters.Feb 25 2020, 21:48

Build Bitcoin-ABC / Diffs / Diff Testing passed.

Harbormaster completed remote builds in B9568: Diff 16505.Feb 25 2020, 21:57

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.

Build Bitcoin-ABC / Diffs / Diff Testing started.

Build Bitcoin-ABC / Diffs / Diff Testing passed.

Harbormaster completed remote builds in B9572: Diff 16511.Feb 25 2020, 23:24

jasonbcox accepted this revision.Feb 26 2020, 00:51

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.

In D5329#129147, @Mengerian wrote:

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

Rebased.

Build Bitcoin-ABC / Diffs / Diff Testing started.

Build Bitcoin-ABC / Diffs / Diff Testing passed.

Harbormaster completed remote builds in B9646: Diff 16631.Feb 28 2020, 22:05

Closed by commit rABC1c0566ce001c: Enforce maximum name length for parse_name() and add unit tests (authored by nakihito). · Explain WhyMar 2 2020, 23:17

This revision was automatically updated to reflect the committed changes.

nakihito added a commit: rABC1c0566ce001c: Enforce maximum name length for parse_name() and add unit tests.

nakihito added a commit: rSTAGING1c0566ce001c: Enforce maximum name length for parse_name() and add unit tests.Apr 7 2020, 23:12