Remove panic in nat package on invalid hostport#14682
Merged
calavera merged 1 commit intomoby:masterfrom Jul 21, 2015
Merged
Conversation
duglin
force-pushed
the
Issue14621
branch
3 times, most recently
from
c1a45bd to
17ddc9f
Compare
Member
|
LGTM |
Member
There was a problem hiding this comment.
I don't think we need this, and is a bit dangerous to to add functions just to support a test case.
Tests can just do p, _ just as this function is, or add a test helper called newPort that does this.
Contributor
Author
There was a problem hiding this comment.
I like the idea of adding a test util func - will do that.
Closes moby#14621 This one grew to be much more than I expected so here's the story... :-) - when a bad port string (e.g. xxx80) is passed into container.create() via the API it wasn't being checked until we tried to start the container. - While starting the container we trid to parse 'xxx80' in nat.Int() and would panic on the strconv.ParseUint(). We should (almost) never panic. - In trying to remove the panic I decided to make it so that we, instead, checked the string during the NewPort() constructor. This means that I had to change all casts from 'string' to 'Port' to use NewPort() instead. Which is a good thing anyway, people shouldn't assume they know the internal format of types like that, in general. - This meant I had to go and add error checks on all calls to NewPort(). To avoid changing the testcases too much I create newPortNoError() **JUST** for the testcase uses where we know the port string is ok. - After all of that I then went back and added a check during container.create() to check the port string so we'll report the error as soon as we get the data. - If, somehow, the bad string does get into the metadata we will generate an error during container.start() but I can't test for that because the container.create() catches it now. But I did add a testcase for that. Signed-off-by: Doug Davis <[email protected]>
Contributor
Author
|
Updated per @cpuguy83 's suggestion |
Contributor
Author
Contributor
|
LGTM |
calavera
added a commit
that referenced
this pull request
Jul 21, 2015
Remove panic in nat package on invalid hostport
austinvazquez
pushed a commit
to austinvazquez/moby
that referenced
this pull request
Aug 8, 2025
Remove panic in nat package on invalid hostport
This file contains hidden or 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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
5 participants
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.
Closes #14621
This one grew to be much more than I expected so here's the story... :-)
via the API it wasn't being checked until we tried to start the container.
and would panic on the strconv.ParseUint(). We should (almost) never panic.
checked the string during the NewPort() constructor. This means that
I had to change all casts from 'string' to 'Port' to use NewPort() instead.
Which is a good thing anyway, people shouldn't assume they know the
internal format of types like that, in general.
To avoid changing the testcases too much I create newPortNoError() JUST
for the testcase uses where we know the port string is ok.
to check the port string so we'll report the error as soon as we get the
data.
an error during container.start() but I can't test for that because
the container.create() catches it now. But I did add a testcase for that.
Signed-off-by: Doug Davis [email protected]