Skip to content

Commit 12b6083

Browse files
author
Doug Davis
committed
Remove panic in nat package on invalid hostport
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]>
1 parent 48a01a3 commit 12b6083

11 files changed

Lines changed: 128 additions & 37 deletions

File tree

‎api/client/port.go‎

Lines changed: 5 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -48,7 +48,11 @@ func (cli *DockerCli) CmdPort(args ...string) error {
4848
proto = parts[1]
4949
}
5050
natPort := port + "/" + proto
51-
if frontends, exists := c.NetworkSettings.Ports[nat.Port(port+"/"+proto)]; exists && frontends != nil {
51+
newP, err := nat.NewPort(proto, port)
52+
if err != nil {
53+
return err
54+
}
55+
if frontends, exists := c.NetworkSettings.Ports[newP]; exists && frontends != nil {
5256
for _, frontend := range frontends {
5357
fmt.Fprintf(cli.out, "%s:%s\n", frontend.HostIp, frontend.HostPort)
5458
}

‎daemon/container_unix.go‎

Lines changed: 13 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -521,7 +521,10 @@ func (container *Container) buildPortMapInfo(n libnetwork.Network, ep libnetwork
521521
if expData, ok := driverInfo[netlabel.ExposedPorts]; ok {
522522
if exposedPorts, ok := expData.([]types.TransportPort); ok {
523523
for _, tp := range exposedPorts {
524-
natPort := nat.NewPort(tp.Proto.String(), strconv.Itoa(int(tp.Port)))
524+
natPort, err := nat.NewPort(tp.Proto.String(), strconv.Itoa(int(tp.Port)))
525+
if err != nil {
526+
return nil, fmt.Errorf("Error parsing Port value(%s):%v", tp.Port, err)
527+
}
525528
networkSettings.Ports[natPort] = nil
526529
}
527530
}
@@ -534,7 +537,10 @@ func (container *Container) buildPortMapInfo(n libnetwork.Network, ep libnetwork
534537

535538
if portMapping, ok := mapData.([]types.PortBinding); ok {
536539
for _, pp := range portMapping {
537-
natPort := nat.NewPort(pp.Proto.String(), strconv.Itoa(int(pp.Port)))
540+
natPort, err := nat.NewPort(pp.Proto.String(), strconv.Itoa(int(pp.Port)))
541+
if err != nil {
542+
return nil, err
543+
}
538544
natBndg := nat.PortBinding{HostIp: pp.HostIP.String(), HostPort: strconv.Itoa(int(pp.HostPort))}
539545
networkSettings.Ports[natPort] = append(networkSettings.Ports[natPort], natBndg)
540546
}
@@ -710,7 +716,11 @@ func (container *Container) buildCreateEndpointOptions() ([]libnetwork.EndpointO
710716
binding := bindings[port]
711717
for i := 0; i < len(binding); i++ {
712718
pbCopy := pb.GetCopy()
713-
pbCopy.HostPort = uint16(nat.Port(binding[i].HostPort).Int())
719+
newP, err := nat.NewPort(nat.SplitProtoPort(binding[i].HostPort))
720+
if err != nil {
721+
return nil, fmt.Errorf("Error parsing HostPort value(%s):%v", binding[i].HostPort, err)
722+
}
723+
pbCopy.HostPort = uint16(newP.Int())
714724
pbCopy.HostIP = net.ParseIP(binding[i].HostIp)
715725
pbList = append(pbList, pbCopy)
716726
}

‎daemon/daemon_unix.go‎

Lines changed: 7 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -132,7 +132,13 @@ func (daemon *Daemon) verifyContainerSettings(hostConfig *runconfig.HostConfig,
132132
for port := range hostConfig.PortBindings {
133133
_, portStr := nat.SplitProtoPort(string(port))
134134
if _, err := nat.ParsePort(portStr); err != nil {
135-
return warnings, fmt.Errorf("Invalid port specification: %s", portStr)
135+
return warnings, fmt.Errorf("Invalid port specification: %q", portStr)
136+
}
137+
for _, pb := range hostConfig.PortBindings[port] {
138+
_, err := nat.NewPort(nat.SplitProtoPort(pb.HostPort))
139+
if err != nil {
140+
return warnings, fmt.Errorf("Invalid port specification: %q", pb.HostPort)
141+
}
136142
}
137143
}
138144
if hostConfig.LxcConf.Len() > 0 && !strings.Contains(daemon.ExecutionDriver().Name(), "lxc") {

‎daemon/list.go‎

Lines changed: 8 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -158,7 +158,10 @@ func (daemon *Daemon) Containers(config *ContainersConfig) ([]*types.Container,
158158

159159
newC.Ports = []types.Port{}
160160
for port, bindings := range container.NetworkSettings.Ports {
161-
p, _ := nat.ParsePort(port.Port())
161+
p, err := nat.ParsePort(port.Port())
162+
if err != nil {
163+
return err
164+
}
162165
if len(bindings) == 0 {
163166
newC.Ports = append(newC.Ports, types.Port{
164167
PrivatePort: p,
@@ -167,7 +170,10 @@ func (daemon *Daemon) Containers(config *ContainersConfig) ([]*types.Container,
167170
continue
168171
}
169172
for _, binding := range bindings {
170-
h, _ := nat.ParsePort(binding.HostPort)
173+
h, err := nat.ParsePort(binding.HostPort)
174+
if err != nil {
175+
return err
176+
}
171177
newC.Ports = append(newC.Ports, types.Port{
172178
PrivatePort: p,
173179
PublicPort: h,

‎integration-cli/docker_api_containers_test.go‎

Lines changed: 26 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -872,6 +872,32 @@ func (s *DockerSuite) TestContainerApiCommitWithLabelInConfig(c *check.C) {
872872
dockerCmd(c, "run", img.Id, "ls", "/test")
873873
}
874874

875+
func (s *DockerSuite) TestContainerApiBadPort(c *check.C) {
876+
config := map[string]interface{}{
877+
"Image": "busybox",
878+
"Cmd": []string{"/bin/sh", "-c", "echo test"},
879+
"PortBindings": map[string]interface{}{
880+
"8080/tcp": []map[string]interface{}{
881+
{
882+
"HostIp": "",
883+
"HostPort": "aa80",
884+
},
885+
},
886+
},
887+
}
888+
889+
jsonData := bytes.NewBuffer(nil)
890+
json.NewEncoder(jsonData).Encode(config)
891+
892+
status, b, err := sockRequest("POST", "/containers/create", config)
893+
c.Assert(err, check.IsNil)
894+
c.Assert(status, check.Equals, http.StatusInternalServerError)
895+
896+
if strings.TrimSpace(string(b)) != `Invalid port specification: "aa80"` {
897+
c.Fatalf("Incorrect error msg: %s", string(b))
898+
}
899+
}
900+
875901
func (s *DockerSuite) TestContainerApiCreate(c *check.C) {
876902
config := map[string]interface{}{
877903
"Image": "busybox",

‎links/links_test.go‎

Lines changed: 16 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -8,9 +8,15 @@ import (
88
"github.com/docker/docker/pkg/nat"
99
)
1010

11+
// Just to make life easier
12+
func newPortNoError(proto, port string) nat.Port {
13+
p, _ := nat.NewPort(proto, port)
14+
return p
15+
}
16+
1117
func TestLinkNaming(t *testing.T) {
1218
ports := make(nat.PortSet)
13-
ports[nat.Port("6379/tcp")] = struct{}{}
19+
ports[newPortNoError("tcp", "6379")] = struct{}{}
1420

1521
link, err := NewLink("172.0.17.3", "172.0.17.2", "/db/docker-1", nil, ports)
1622
if err != nil {
@@ -40,7 +46,7 @@ func TestLinkNaming(t *testing.T) {
4046

4147
func TestLinkNew(t *testing.T) {
4248
ports := make(nat.PortSet)
43-
ports[nat.Port("6379/tcp")] = struct{}{}
49+
ports[newPortNoError("tcp", "6379")] = struct{}{}
4450

4551
link, err := NewLink("172.0.17.3", "172.0.17.2", "/db/docker", nil, ports)
4652
if err != nil {
@@ -63,15 +69,15 @@ func TestLinkNew(t *testing.T) {
6369
t.Fail()
6470
}
6571
for _, p := range link.Ports {
66-
if p != nat.Port("6379/tcp") {
72+
if p != newPortNoError("tcp", "6379") {
6773
t.Fail()
6874
}
6975
}
7076
}
7177

7278
func TestLinkEnv(t *testing.T) {
7379
ports := make(nat.PortSet)
74-
ports[nat.Port("6379/tcp")] = struct{}{}
80+
ports[newPortNoError("tcp", "6379")] = struct{}{}
7581

7682
link, err := NewLink("172.0.17.3", "172.0.17.2", "/db/docker", []string{"PASSWORD=gordon"}, ports)
7783
if err != nil {
@@ -112,9 +118,9 @@ func TestLinkEnv(t *testing.T) {
112118

113119
func TestLinkMultipleEnv(t *testing.T) {
114120
ports := make(nat.PortSet)
115-
ports[nat.Port("6379/tcp")] = struct{}{}
116-
ports[nat.Port("6380/tcp")] = struct{}{}
117-
ports[nat.Port("6381/tcp")] = struct{}{}
121+
ports[newPortNoError("tcp", "6379")] = struct{}{}
122+
ports[newPortNoError("tcp", "6380")] = struct{}{}
123+
ports[newPortNoError("tcp", "6381")] = struct{}{}
118124

119125
link, err := NewLink("172.0.17.3", "172.0.17.2", "/db/docker", []string{"PASSWORD=gordon"}, ports)
120126
if err != nil {
@@ -161,9 +167,9 @@ func TestLinkMultipleEnv(t *testing.T) {
161167

162168
func TestLinkPortRangeEnv(t *testing.T) {
163169
ports := make(nat.PortSet)
164-
ports[nat.Port("6379/tcp")] = struct{}{}
165-
ports[nat.Port("6380/tcp")] = struct{}{}
166-
ports[nat.Port("6381/tcp")] = struct{}{}
170+
ports[newPortNoError("tcp", "6379")] = struct{}{}
171+
ports[newPortNoError("tcp", "6380")] = struct{}{}
172+
ports[newPortNoError("tcp", "6381")] = struct{}{}
167173

168174
link, err := NewLink("172.0.17.3", "172.0.17.2", "/db/docker", []string{"PASSWORD=gordon"}, ports)
169175
if err != nil {

‎pkg/nat/nat.go‎

Lines changed: 22 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -29,8 +29,16 @@ type PortSet map[Port]struct{}
2929
// 80/tcp
3030
type Port string
3131

32-
func NewPort(proto, port string) Port {
33-
return Port(fmt.Sprintf("%s/%s", port, proto))
32+
func NewPort(proto, port string) (Port, error) {
33+
// Check for parsing issues on "port" now so we can avoid having
34+
// to check it later on.
35+
36+
portInt, err := ParsePort(port)
37+
if err != nil {
38+
return "", err
39+
}
40+
41+
return Port(fmt.Sprintf("%d/%s", portInt, proto)), nil
3442
}
3543

3644
func ParsePort(rawPort string) (int, error) {
@@ -55,11 +63,15 @@ func (p Port) Port() string {
5563
}
5664

5765
func (p Port) Int() int {
58-
port, err := ParsePort(p.Port())
59-
if err != nil {
60-
panic(err)
66+
portStr := p.Port()
67+
if len(portStr) == 0 {
68+
return 0
6169
}
62-
return port
70+
71+
// We don't need to check for an error because we're going to
72+
// assume that any error would have been found, and reported, in NewPort()
73+
port, _ := strconv.ParseUint(portStr, 10, 16)
74+
return int(port)
6375
}
6476

6577
// Splits a port in the format of proto/port
@@ -152,7 +164,10 @@ func ParsePortSpecs(ports []string) (map[Port]struct{}, map[Port][]PortBinding,
152164
if len(hostPort) > 0 {
153165
hostPort = strconv.FormatUint(startHostPort+i, 10)
154166
}
155-
port := NewPort(strings.ToLower(proto), containerPort)
167+
port, err := NewPort(strings.ToLower(proto), containerPort)
168+
if err != nil {
169+
return nil, nil, err
170+
}
156171
if _, exists := exposedPorts[port]; !exists {
157172
exposedPorts[port] = struct{}{}
158173
}

‎pkg/nat/nat_test.go‎

Lines changed: 10 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -42,7 +42,11 @@ func TestParsePort(t *testing.T) {
4242
}
4343

4444
func TestPort(t *testing.T) {
45-
p := NewPort("tcp", "1234")
45+
p, err := NewPort("tcp", "1234")
46+
47+
if err != nil {
48+
t.Fatalf("tcp, 1234 had a parsing issue: %v", err)
49+
}
4650

4751
if string(p) != "1234/tcp" {
4852
t.Fatal("tcp, 1234 did not result in the string 1234/tcp")
@@ -59,6 +63,11 @@ func TestPort(t *testing.T) {
5963
if p.Int() != 1234 {
6064
t.Fatal("port int value was not 1234")
6165
}
66+
67+
p, err = NewPort("tcp", "asd1234")
68+
if err == nil {
69+
t.Fatal("tcp, asd1234 was supposed to fail")
70+
}
6271
}
6372

6473
func TestSplitProtoPort(t *testing.T) {

‎runconfig/compare_test.go‎

Lines changed: 13 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -6,17 +6,23 @@ import (
66
"github.com/docker/docker/pkg/nat"
77
)
88

9+
// Just to make life easier
10+
func newPortNoError(proto, port string) nat.Port {
11+
p, _ := nat.NewPort(proto, port)
12+
return p
13+
}
14+
915
func TestCompare(t *testing.T) {
1016
ports1 := make(nat.PortSet)
11-
ports1[nat.Port("1111/tcp")] = struct{}{}
12-
ports1[nat.Port("2222/tcp")] = struct{}{}
17+
ports1[newPortNoError("tcp", "1111")] = struct{}{}
18+
ports1[newPortNoError("tcp", "2222")] = struct{}{}
1319
ports2 := make(nat.PortSet)
14-
ports2[nat.Port("3333/tcp")] = struct{}{}
15-
ports2[nat.Port("4444/tcp")] = struct{}{}
20+
ports2[newPortNoError("tcp", "3333")] = struct{}{}
21+
ports2[newPortNoError("tcp", "4444")] = struct{}{}
1622
ports3 := make(nat.PortSet)
17-
ports3[nat.Port("1111/tcp")] = struct{}{}
18-
ports3[nat.Port("2222/tcp")] = struct{}{}
19-
ports3[nat.Port("5555/tcp")] = struct{}{}
23+
ports3[newPortNoError("tcp", "1111")] = struct{}{}
24+
ports3[newPortNoError("tcp", "2222")] = struct{}{}
25+
ports3[newPortNoError("tcp", "5555")] = struct{}{}
2026
volumes1 := make(map[string]struct{})
2127
volumes1["/test1"] = struct{}{}
2228
volumes2 := make(map[string]struct{})

‎runconfig/merge_test.go‎

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -11,17 +11,17 @@ func TestMerge(t *testing.T) {
1111
volumesImage["/test1"] = struct{}{}
1212
volumesImage["/test2"] = struct{}{}
1313
portsImage := make(nat.PortSet)
14-
portsImage[nat.Port("1111/tcp")] = struct{}{}
15-
portsImage[nat.Port("2222/tcp")] = struct{}{}
14+
portsImage[newPortNoError("tcp", "1111")] = struct{}{}
15+
portsImage[newPortNoError("tcp", "2222")] = struct{}{}
1616
configImage := &Config{
1717
ExposedPorts: portsImage,
1818
Env: []string{"VAR1=1", "VAR2=2"},
1919
Volumes: volumesImage,
2020
}
2121

2222
portsUser := make(nat.PortSet)
23-
portsUser[nat.Port("2222/tcp")] = struct{}{}
24-
portsUser[nat.Port("3333/tcp")] = struct{}{}
23+
portsUser[newPortNoError("tcp", "2222")] = struct{}{}
24+
portsUser[newPortNoError("tcp", "3333")] = struct{}{}
2525
volumesUser := make(map[string]struct{})
2626
volumesUser["/test3"] = struct{}{}
2727
configUser := &Config{

0 commit comments

Comments
 (0)