Page MenuHomePhabricator

Make parse_name() properly fail when the query name is longer than 255 characters
AbandonedPublic

Authored by nakihito on Feb 10 2020, 20:24.

Details

Reviewers
jasonbcox
Group Reviewers
Restricted Owners Package(Owns No Changed Paths)
Restricted Project
Summary

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 D4418

Test Plan
ninja check-bitcoin-seeder

Diff Detail

Repository
rABC Bitcoin ABC
Branch
MaxLengthNameCheck
Lint
Lint OK
Unit
No Unit Test Coverage
Build Status
Buildable 9406
Build 16742: Default Diff Build & Tests
Build 16741: arc lint + arc unit

Event Timeline

nakihito created this revision.Feb 10 2020, 20:24
Owners added a reviewer: Restricted Owners Package.Feb 10 2020, 20:24
Herald added a reviewer: Restricted Project. · View Herald TranscriptFeb 10 2020, 20:24
nakihito retitled this revision from Make parse_name() fail when the query name is longer than 255 characters to Make parse_name() properly fail when the query name is longer than 255 characters.Feb 10 2020, 22:02
nakihito edited the summary of this revision. (Show Details)
jasonbcox requested changes to this revision.Feb 10 2020, 22:04
jasonbcox added a subscriber: jasonbcox.
jasonbcox added inline comments.
src/seeder/dns.cpp
81 ↗(On Diff #16228)

Just checking my understanding here: there needs to be at least 2 free units for the '.' and a following non-dot character?

Either way, a comment explaining how this value was chosen would be helpful, since it's not obvious at a glance.

src/seeder/test/seeder_tests.cpp
124 ↗(On Diff #16228)

Checking immediately beyond the boundary case would be more useful, otherwise this test will not fail for an off-by-one error.

This revision now requires changes to proceed.Feb 10 2020, 22:04
nakihito planned changes to this revision.Feb 10 2020, 23:28
nakihito added inline comments.
src/seeder/dns.cpp
81 ↗(On Diff #16228)

This is basically correct. The buffer is also null-terminated when returning successfully. I'll add a comment clarifying this.

nakihito updated this revision to Diff 16243.Feb 10 2020, 23:49

Added clarifying comment and changed to test to check immediately beyond the character limit.

jasonbcox accepted this revision.Feb 11 2020, 00:28
jasonbcox added inline comments.
src/seeder/dns.cpp
83

Fix the linting. should be fit on 2 lines

This revision is now accepted and ready to land.Feb 11 2020, 00:28
nakihito abandoned this revision.Feb 25 2020, 21:48

Dropped in favor of D5329.