Bail early from UnpackDomainName when name is too long #839
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This drastically reduces the amount of garbage created in UnpackDomainName for certain malicious names. The wire formatted name "\x3Faaabbbcccdddeeefffggghhhiiijjjkkklllmmmnnnooopppqqqrrrssstttuuu\xC0\x00" would previously generate 1936B of garbage (36112B since maxCompressionPointers was raised) before returning the "too many compression pointers" error, while it now generates just 384B of garbage.
This better reflects what maxDomainNameWireOctets is actually measuring.
|
/cc @semihalev |
Codecov Report
@@ Coverage Diff @@
## master #839 +/- ##
==========================================
+ Coverage 57.87% 57.92% +0.04%
==========================================
Files 42 42
Lines 10820 10830 +10
==========================================
+ Hits 6262 6273 +11
Misses 3473 3473
+ Partials 1085 1084 -1
Continue to review full report at Codecov.
|
This can never be tripped as budget is always checked immediately after subtracting inside the loop.
semihalev
suggested changes
Nov 28, 2018
Closed
miekg
reviewed
Nov 28, 2018
|
/lgtm apart from the one nit |
|
[ Quoting <notifications@github.com> in "Re: [miekg/dns] Bail early from Unp..." ]
tmthrgd commented on this pull request.
> @@ -427,10 +427,7 @@ Loop:
off1 = off
}
if len(s) == 0 {
- s = []byte(".")
- } else if len(s) >= maxLen {
- // error if the name is too long, but don't throw it away
- return string(s), lenmsg, ErrLongDomain
@miekg Would you prefer a comment on `UnpackDomainName` or where the `if budget <= 0` return is?
function level doc about this edge case. Current doc is rather sparse.
|
miekg
approved these changes
Nov 28, 2018
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This PR drastically reduces the amount of garbage created in
UnpackDomainNamefor certain malicious names.The wire formatted name
"\x3Faaabbbcccdddeeefffggghhhiiijjjkkklllmmmnnnooopppqqqrrrssstttuuu\xC0\x00"would previously generate 1936B of garbage before #835, and 36112B of garbage since #835, before returning the
"too many compression pointers"error, while after this PR it will generate just 384B of garbage before returning the error.Related #835
Fixes #836