Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Limit $GENERATE range to 65535 steps #1020

Merged
merged 3 commits into from
Oct 3, 2019
Merged

Limit $GENERATE range to 65535 steps #1020

merged 3 commits into from
Oct 3, 2019

Conversation

miekg
Copy link
Owner

@miekg miekg commented Oct 3, 2019

Fixes #1019

@codecov-io
Copy link

codecov-io commented Oct 3, 2019 •
edited
Loading

Codecov Report

Merging #1020 into master will increase coverage by 0.49%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1020      +/-   ##
==========================================
+ Coverage   54.78%   55.27%   +0.49%     
==========================================
  Files          41       41              
  Lines        9867     9973     +106     
==========================================
+ Hits         5406     5513     +107     
+ Misses       3435     3431       -4     
- Partials     1026     1029       +3
Impacted Files Coverage Δ
scan.go 83.72% <ø> (+2.18%) ⬆️
generate.go 79.2% <100%> (+1.6%) ⬆️
msg.go 76.83% <0%> (-0.67%) ⬇️
server.go 67.95% <0%> (+0.27%) ⬆️
scan_rr.go 59.34% <0%> (+0.33%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 046ae4e...44b376e. Read the comment docs.

tmthrgd
tmthrgd requested changes Oct 3, 2019
View reviewed changes
generate_test.go Outdated Show resolved Hide resolved
generate.go Outdated Show resolved Hide resolved
@miekg
Limit $GENERATE range to 65535 steps
f0bed49 
Having these checks means all test in TestCrasherString() are not
reached because we bail out earlier - removed that test all together.

Fixes #1019

Signed-off-by: Miek Gieben <[email protected]>
@miekg
Copy link
Owner Author

miekg commented Oct 3, 2019

done. PTAL

chantra
chantra reviewed Oct 3, 2019
View reviewed changes
generate.go
@@ -49,14 +49,14 @@ func (zp *ZoneParser) generate(l lex) (RR, bool) {
if err != nil {
return zp.setParseError("bad stop in $GENERATE range", l)
}
if end < 0 || start < 0 || end < start {
if end < 0 || start < 0 || end < start || (end-start)/step > 65535 {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See my comment in #1019 (comment) to align this with BIND behaviour. TL;DR both start and stop must be positive integers between 0 and (2^31)-1

chantra
chantra reviewed Oct 3, 2019
View reviewed changes
generate_test.go Outdated
Comment on lines 215 to 218
{"$GENERATE 0-300103\"$$GENERATE 2-2", "dns: garbage after $GENERATE range: \"\\\"\" at line: 1:19"},
{"$GENERATE 0-5414137360", "dns: garbage after $GENERATE range: \"\\n\" at line: 1:22"},
{"$GENERATE 11522-3668518066406258", "dns: garbage after $GENERATE range: \"\\n\" at line: 1:38"},
{"$GENERATE 0-200\"(;00000000000000\n$$GENERATE 0-0", "dns: garbage after $GENERATE range: \"\\\"\" at line: 1:16"},
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

some of those test were generating different errors, like timeout (I believe the second test was a timeout). Let me find the original error they were triggering.

chantra
chantra reviewed Oct 3, 2019
View reviewed changes
generate_test.go
err string
}{
{"$GENERATE 0-300103\"$$GENERATE 2-2", "dns: garbage after $GENERATE range: \"\\\"\" at line: 1:19"},
{"$GENERATE 0-5414137360", "dns: garbage after $GENERATE range: \"\\n\" at line: 1:22"},
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

original error: program hanged (timeout 10 seconds)

@miekg
Copy link
Owner Author

miekg commented Oct 3, 2019 via email

chantra
chantra reviewed Oct 3, 2019
View reviewed changes
generate_test.go
in string
err string
}{
{"$GENERATE 0-300103\"$$GENERATE 2-2", "dns: garbage after $GENERATE range: \"\\\"\" at line: 1:19"},
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

runtime: goroutine stack exceeds 1000000000-byte limit fatal error: stack overflow

generate_test.go
}{
{"$GENERATE 0-300103\"$$GENERATE 2-2", "dns: garbage after $GENERATE range: \"\\\"\" at line: 1:19"},
{"$GENERATE 0-5414137360", "dns: garbage after $GENERATE range: \"\\n\" at line: 1:22"},
{"$GENERATE 11522-3668518066406258", "dns: garbage after $GENERATE range: \"\\n\" at line: 1:38"},
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

program hanged (timeout 10 seconds)

generate_test.go
{"$GENERATE 0-300103\"$$GENERATE 2-2", "dns: garbage after $GENERATE range: \"\\\"\" at line: 1:19"},
{"$GENERATE 0-5414137360", "dns: garbage after $GENERATE range: \"\\n\" at line: 1:22"},
{"$GENERATE 11522-3668518066406258", "dns: garbage after $GENERATE range: \"\\n\" at line: 1:38"},
{"$GENERATE 0-200\"(;00000000000000\n$$GENERATE 0-0", "dns: garbage after $GENERATE range: \"\\\"\" at line: 1:16"},
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

panic: runtime error: index out of range [2048] with length 2048

@miekg
Copy link
Owner Author

miekg commented Oct 3, 2019

@chantra happy to merge this as it now?

@chantra
Copy link
Contributor

chantra commented Oct 3, 2019

sure, I would be tempted to leave test case that catch garbage after the range/step token though :)

@miekg
bring back testcase
7c18208 
Signed-off-by: Miek Gieben <[email protected]>
@miekg
bring back crash test
44b376e 
Signed-off-by: Miek Gieben <[email protected]>
@miekg
Copy link
Owner Author

miekg commented Oct 3, 2019

ok, adding that back in

@miekg miekg merged commit 76b57d0 into master Oct 3, 2019
@miekg miekg deleted the limit-generate branch October 3, 2019 19:01
chantra added a commit to chantra/miekg-dns that referenced this pull request Oct 23, 2019
@chantra
[zone parser] disallow nested $GENERATE directive
fa22be0 
While the range number of GENERATE is now limited, one can pass
a line with 2 $GENERATE directive that will exponentially increase the
time spent generating RRs.
Limit to only one per line.
Fixes miekg#1020
@chantra chantra mentioned this pull request Oct 23, 2019
Merged
chantra added a commit to chantra/miekg-dns that referenced this pull request Oct 23, 2019
@chantra
[zone parser] disallow nested $GENERATE directive
7663045 
While the range number of GENERATE is now limited, one can pass
a line with 2 $GENERATE directive that will exponentially increase the
time spent generating RRs.
Limit to only one per line.
Fixes miekg#1020
chantra added a commit to chantra/miekg-dns that referenced this pull request Oct 23, 2019
@chantra
[zone parser] disallow nested $GENERATE directive
e9aa97b 
While the range number of GENERATE is now limited, one can pass
a line with 2 $GENERATE directive that will exponentially increase the
time spent generating RRs.
Limit to only one per line.
Fixes miekg#1020
miekg pushed a commit that referenced this pull request Oct 23, 2019
@chantra @miekg
[zone parser] disallow nested $GENERATE directive (#1033)
9b7437f 
While the range number of GENERATE is now limited, one can pass
a line with 2 $GENERATE directive that will exponentially increase the
time spent generating RRs.
Limit to only one per line.
Fixes #1020
aanm pushed a commit to cilium/dns that referenced this pull request Jul 29, 2022
@miekg @aanm
Limit $GENERATE range to 65535 steps (miekg#1020)
c6acdd9 
* Limit $GENERATE range to 65535 steps

Having these checks means all test in TestCrasherString() are not
reached because we bail out earlier - removed that test all together.

Fixes miekg#1019

Signed-off-by: Miek Gieben <[email protected]>

* bring back testcase

Signed-off-by: Miek Gieben <[email protected]>

* bring back crash test

Signed-off-by: Miek Gieben <[email protected]>
aanm pushed a commit to cilium/dns that referenced this pull request Jul 29, 2022
@chantra @aanm
[zone parser] disallow nested $GENERATE directive (miekg#1033)
00cecff 
While the range number of GENERATE is now limited, one can pass
a line with 2 $GENERATE directive that will exponentially increase the
time spent generating RRs.
Limit to only one per line.
Fixes miekg#1020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@chantra chantra chantra left review comments

@tmthrgd tmthrgd tmthrgd requested changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

limit $GENERATE range to 2^16?
4 participants
@miekg @codecov-io @chantra @tmthrgd