From 12b6083c8f82db7e5db4c683cfe20151731ea851 Mon Sep 17 00:00:00 2001 From: Doug Davis Date: Wed, 15 Jul 2015 20:45:48 -0700 Subject: [PATCH] Remove panic in nat package on invalid hostport Closes #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 --- api/client/port.go | 6 +++- daemon/container_unix.go | 16 ++++++++-- daemon/daemon_unix.go | 8 ++++- daemon/list.go | 10 +++++-- integration-cli/docker_api_containers_test.go | 26 +++++++++++++++++ links/links_test.go | 26 ++++++++++------- pkg/nat/nat.go | 29 ++++++++++++++----- pkg/nat/nat_test.go | 11 ++++++- runconfig/compare_test.go | 20 ++++++++----- runconfig/merge_test.go | 8 ++--- runconfig/parse.go | 5 +++- 11 files changed, 128 insertions(+), 37 deletions(-) diff --git a/api/client/port.go b/api/client/port.go index 245bbfcc4c..09d6b5bd9f 100644 --- a/api/client/port.go +++ b/api/client/port.go @@ -48,7 +48,11 @@ func (cli *DockerCli) CmdPort(args ...string) error { proto = parts[1] } natPort := port + "/" + proto - if frontends, exists := c.NetworkSettings.Ports[nat.Port(port+"/"+proto)]; exists && frontends != nil { + newP, err := nat.NewPort(proto, port) + if err != nil { + return err + } + if frontends, exists := c.NetworkSettings.Ports[newP]; exists && frontends != nil { for _, frontend := range frontends { fmt.Fprintf(cli.out, "%s:%s\n", frontend.HostIp, frontend.HostPort) } diff --git a/daemon/container_unix.go b/daemon/container_unix.go index c5d3c01bb6..cc784609d2 100644 --- a/daemon/container_unix.go +++ b/daemon/container_unix.go @@ -521,7 +521,10 @@ func (container *Container) buildPortMapInfo(n libnetwork.Network, ep libnetwork if expData, ok := driverInfo[netlabel.ExposedPorts]; ok { if exposedPorts, ok := expData.([]types.TransportPort); ok { for _, tp := range exposedPorts { - natPort := nat.NewPort(tp.Proto.String(), strconv.Itoa(int(tp.Port))) + natPort, err := nat.NewPort(tp.Proto.String(), strconv.Itoa(int(tp.Port))) + if err != nil { + return nil, fmt.Errorf("Error parsing Port value(%s):%v", tp.Port, err) + } networkSettings.Ports[natPort] = nil } } @@ -534,7 +537,10 @@ func (container *Container) buildPortMapInfo(n libnetwork.Network, ep libnetwork if portMapping, ok := mapData.([]types.PortBinding); ok { for _, pp := range portMapping { - natPort := nat.NewPort(pp.Proto.String(), strconv.Itoa(int(pp.Port))) + natPort, err := nat.NewPort(pp.Proto.String(), strconv.Itoa(int(pp.Port))) + if err != nil { + return nil, err + } natBndg := nat.PortBinding{HostIp: pp.HostIP.String(), HostPort: strconv.Itoa(int(pp.HostPort))} networkSettings.Ports[natPort] = append(networkSettings.Ports[natPort], natBndg) } @@ -710,7 +716,11 @@ func (container *Container) buildCreateEndpointOptions() ([]libnetwork.EndpointO binding := bindings[port] for i := 0; i < len(binding); i++ { pbCopy := pb.GetCopy() - pbCopy.HostPort = uint16(nat.Port(binding[i].HostPort).Int()) + newP, err := nat.NewPort(nat.SplitProtoPort(binding[i].HostPort)) + if err != nil { + return nil, fmt.Errorf("Error parsing HostPort value(%s):%v", binding[i].HostPort, err) + } + pbCopy.HostPort = uint16(newP.Int()) pbCopy.HostIP = net.ParseIP(binding[i].HostIp) pbList = append(pbList, pbCopy) } diff --git a/daemon/daemon_unix.go b/daemon/daemon_unix.go index 7370b50db5..e8d5abbebd 100644 --- a/daemon/daemon_unix.go +++ b/daemon/daemon_unix.go @@ -132,7 +132,13 @@ func (daemon *Daemon) verifyContainerSettings(hostConfig *runconfig.HostConfig, for port := range hostConfig.PortBindings { _, portStr := nat.SplitProtoPort(string(port)) if _, err := nat.ParsePort(portStr); err != nil { - return warnings, fmt.Errorf("Invalid port specification: %s", portStr) + return warnings, fmt.Errorf("Invalid port specification: %q", portStr) + } + for _, pb := range hostConfig.PortBindings[port] { + _, err := nat.NewPort(nat.SplitProtoPort(pb.HostPort)) + if err != nil { + return warnings, fmt.Errorf("Invalid port specification: %q", pb.HostPort) + } } } if hostConfig.LxcConf.Len() > 0 && !strings.Contains(daemon.ExecutionDriver().Name(), "lxc") { diff --git a/daemon/list.go b/daemon/list.go index 9c7a1641bb..3e00023fcf 100644 --- a/daemon/list.go +++ b/daemon/list.go @@ -158,7 +158,10 @@ func (daemon *Daemon) Containers(config *ContainersConfig) ([]*types.Container, newC.Ports = []types.Port{} for port, bindings := range container.NetworkSettings.Ports { - p, _ := nat.ParsePort(port.Port()) + p, err := nat.ParsePort(port.Port()) + if err != nil { + return err + } if len(bindings) == 0 { newC.Ports = append(newC.Ports, types.Port{ PrivatePort: p, @@ -167,7 +170,10 @@ func (daemon *Daemon) Containers(config *ContainersConfig) ([]*types.Container, continue } for _, binding := range bindings { - h, _ := nat.ParsePort(binding.HostPort) + h, err := nat.ParsePort(binding.HostPort) + if err != nil { + return err + } newC.Ports = append(newC.Ports, types.Port{ PrivatePort: p, PublicPort: h, diff --git a/integration-cli/docker_api_containers_test.go b/integration-cli/docker_api_containers_test.go index 9236f88409..193c66fd7e 100644 --- a/integration-cli/docker_api_containers_test.go +++ b/integration-cli/docker_api_containers_test.go @@ -872,6 +872,32 @@ func (s *DockerSuite) TestContainerApiCommitWithLabelInConfig(c *check.C) { dockerCmd(c, "run", img.Id, "ls", "/test") } +func (s *DockerSuite) TestContainerApiBadPort(c *check.C) { + config := map[string]interface{}{ + "Image": "busybox", + "Cmd": []string{"/bin/sh", "-c", "echo test"}, + "PortBindings": map[string]interface{}{ + "8080/tcp": []map[string]interface{}{ + { + "HostIp": "", + "HostPort": "aa80", + }, + }, + }, + } + + jsonData := bytes.NewBuffer(nil) + json.NewEncoder(jsonData).Encode(config) + + status, b, err := sockRequest("POST", "/containers/create", config) + c.Assert(err, check.IsNil) + c.Assert(status, check.Equals, http.StatusInternalServerError) + + if strings.TrimSpace(string(b)) != `Invalid port specification: "aa80"` { + c.Fatalf("Incorrect error msg: %s", string(b)) + } +} + func (s *DockerSuite) TestContainerApiCreate(c *check.C) { config := map[string]interface{}{ "Image": "busybox", diff --git a/links/links_test.go b/links/links_test.go index 4786c2f686..21952c3d77 100644 --- a/links/links_test.go +++ b/links/links_test.go @@ -8,9 +8,15 @@ import ( "github.com/docker/docker/pkg/nat" ) +// Just to make life easier +func newPortNoError(proto, port string) nat.Port { + p, _ := nat.NewPort(proto, port) + return p +} + func TestLinkNaming(t *testing.T) { ports := make(nat.PortSet) - ports[nat.Port("6379/tcp")] = struct{}{} + ports[newPortNoError("tcp", "6379")] = struct{}{} link, err := NewLink("172.0.17.3", "172.0.17.2", "/db/docker-1", nil, ports) if err != nil { @@ -40,7 +46,7 @@ func TestLinkNaming(t *testing.T) { func TestLinkNew(t *testing.T) { ports := make(nat.PortSet) - ports[nat.Port("6379/tcp")] = struct{}{} + ports[newPortNoError("tcp", "6379")] = struct{}{} link, err := NewLink("172.0.17.3", "172.0.17.2", "/db/docker", nil, ports) if err != nil { @@ -63,7 +69,7 @@ func TestLinkNew(t *testing.T) { t.Fail() } for _, p := range link.Ports { - if p != nat.Port("6379/tcp") { + if p != newPortNoError("tcp", "6379") { t.Fail() } } @@ -71,7 +77,7 @@ func TestLinkNew(t *testing.T) { func TestLinkEnv(t *testing.T) { ports := make(nat.PortSet) - ports[nat.Port("6379/tcp")] = struct{}{} + ports[newPortNoError("tcp", "6379")] = struct{}{} link, err := NewLink("172.0.17.3", "172.0.17.2", "/db/docker", []string{"PASSWORD=gordon"}, ports) if err != nil { @@ -112,9 +118,9 @@ func TestLinkEnv(t *testing.T) { func TestLinkMultipleEnv(t *testing.T) { ports := make(nat.PortSet) - ports[nat.Port("6379/tcp")] = struct{}{} - ports[nat.Port("6380/tcp")] = struct{}{} - ports[nat.Port("6381/tcp")] = struct{}{} + ports[newPortNoError("tcp", "6379")] = struct{}{} + ports[newPortNoError("tcp", "6380")] = struct{}{} + ports[newPortNoError("tcp", "6381")] = struct{}{} link, err := NewLink("172.0.17.3", "172.0.17.2", "/db/docker", []string{"PASSWORD=gordon"}, ports) if err != nil { @@ -161,9 +167,9 @@ func TestLinkMultipleEnv(t *testing.T) { func TestLinkPortRangeEnv(t *testing.T) { ports := make(nat.PortSet) - ports[nat.Port("6379/tcp")] = struct{}{} - ports[nat.Port("6380/tcp")] = struct{}{} - ports[nat.Port("6381/tcp")] = struct{}{} + ports[newPortNoError("tcp", "6379")] = struct{}{} + ports[newPortNoError("tcp", "6380")] = struct{}{} + ports[newPortNoError("tcp", "6381")] = struct{}{} link, err := NewLink("172.0.17.3", "172.0.17.2", "/db/docker", []string{"PASSWORD=gordon"}, ports) if err != nil { diff --git a/pkg/nat/nat.go b/pkg/nat/nat.go index 2cec2e86ce..3805b2e853 100644 --- a/pkg/nat/nat.go +++ b/pkg/nat/nat.go @@ -29,8 +29,16 @@ type PortSet map[Port]struct{} // 80/tcp type Port string -func NewPort(proto, port string) Port { - return Port(fmt.Sprintf("%s/%s", port, proto)) +func NewPort(proto, port string) (Port, error) { + // Check for parsing issues on "port" now so we can avoid having + // to check it later on. + + portInt, err := ParsePort(port) + if err != nil { + return "", err + } + + return Port(fmt.Sprintf("%d/%s", portInt, proto)), nil } func ParsePort(rawPort string) (int, error) { @@ -55,11 +63,15 @@ func (p Port) Port() string { } func (p Port) Int() int { - port, err := ParsePort(p.Port()) - if err != nil { - panic(err) + portStr := p.Port() + if len(portStr) == 0 { + return 0 } - return port + + // We don't need to check for an error because we're going to + // assume that any error would have been found, and reported, in NewPort() + port, _ := strconv.ParseUint(portStr, 10, 16) + return int(port) } // Splits a port in the format of proto/port @@ -152,7 +164,10 @@ func ParsePortSpecs(ports []string) (map[Port]struct{}, map[Port][]PortBinding, if len(hostPort) > 0 { hostPort = strconv.FormatUint(startHostPort+i, 10) } - port := NewPort(strings.ToLower(proto), containerPort) + port, err := NewPort(strings.ToLower(proto), containerPort) + if err != nil { + return nil, nil, err + } if _, exists := exposedPorts[port]; !exists { exposedPorts[port] = struct{}{} } diff --git a/pkg/nat/nat_test.go b/pkg/nat/nat_test.go index 376857fd72..f17fda575a 100644 --- a/pkg/nat/nat_test.go +++ b/pkg/nat/nat_test.go @@ -42,7 +42,11 @@ func TestParsePort(t *testing.T) { } func TestPort(t *testing.T) { - p := NewPort("tcp", "1234") + p, err := NewPort("tcp", "1234") + + if err != nil { + t.Fatalf("tcp, 1234 had a parsing issue: %v", err) + } if string(p) != "1234/tcp" { t.Fatal("tcp, 1234 did not result in the string 1234/tcp") @@ -59,6 +63,11 @@ func TestPort(t *testing.T) { if p.Int() != 1234 { t.Fatal("port int value was not 1234") } + + p, err = NewPort("tcp", "asd1234") + if err == nil { + t.Fatal("tcp, asd1234 was supposed to fail") + } } func TestSplitProtoPort(t *testing.T) { diff --git a/runconfig/compare_test.go b/runconfig/compare_test.go index 0f4995bab5..e59c3b2fc8 100644 --- a/runconfig/compare_test.go +++ b/runconfig/compare_test.go @@ -6,17 +6,23 @@ import ( "github.com/docker/docker/pkg/nat" ) +// Just to make life easier +func newPortNoError(proto, port string) nat.Port { + p, _ := nat.NewPort(proto, port) + return p +} + func TestCompare(t *testing.T) { ports1 := make(nat.PortSet) - ports1[nat.Port("1111/tcp")] = struct{}{} - ports1[nat.Port("2222/tcp")] = struct{}{} + ports1[newPortNoError("tcp", "1111")] = struct{}{} + ports1[newPortNoError("tcp", "2222")] = struct{}{} ports2 := make(nat.PortSet) - ports2[nat.Port("3333/tcp")] = struct{}{} - ports2[nat.Port("4444/tcp")] = struct{}{} + ports2[newPortNoError("tcp", "3333")] = struct{}{} + ports2[newPortNoError("tcp", "4444")] = struct{}{} ports3 := make(nat.PortSet) - ports3[nat.Port("1111/tcp")] = struct{}{} - ports3[nat.Port("2222/tcp")] = struct{}{} - ports3[nat.Port("5555/tcp")] = struct{}{} + ports3[newPortNoError("tcp", "1111")] = struct{}{} + ports3[newPortNoError("tcp", "2222")] = struct{}{} + ports3[newPortNoError("tcp", "5555")] = struct{}{} volumes1 := make(map[string]struct{}) volumes1["/test1"] = struct{}{} volumes2 := make(map[string]struct{}) diff --git a/runconfig/merge_test.go b/runconfig/merge_test.go index 9612ab661f..6237ee9df0 100644 --- a/runconfig/merge_test.go +++ b/runconfig/merge_test.go @@ -11,8 +11,8 @@ func TestMerge(t *testing.T) { volumesImage["/test1"] = struct{}{} volumesImage["/test2"] = struct{}{} portsImage := make(nat.PortSet) - portsImage[nat.Port("1111/tcp")] = struct{}{} - portsImage[nat.Port("2222/tcp")] = struct{}{} + portsImage[newPortNoError("tcp", "1111")] = struct{}{} + portsImage[newPortNoError("tcp", "2222")] = struct{}{} configImage := &Config{ ExposedPorts: portsImage, Env: []string{"VAR1=1", "VAR2=2"}, @@ -20,8 +20,8 @@ func TestMerge(t *testing.T) { } portsUser := make(nat.PortSet) - portsUser[nat.Port("2222/tcp")] = struct{}{} - portsUser[nat.Port("3333/tcp")] = struct{}{} + portsUser[newPortNoError("tcp", "2222")] = struct{}{} + portsUser[newPortNoError("tcp", "3333")] = struct{}{} volumesUser := make(map[string]struct{}) volumesUser["/test3"] = struct{}{} configUser := &Config{ diff --git a/runconfig/parse.go b/runconfig/parse.go index f52fb45b8e..491bab7b10 100644 --- a/runconfig/parse.go +++ b/runconfig/parse.go @@ -267,7 +267,10 @@ func Parse(cmd *flag.FlagSet, args []string) (*Config, *HostConfig, *flag.FlagSe return nil, nil, cmd, fmt.Errorf("Invalid range format for --expose: %s, error: %s", e, err) } for i := start; i <= end; i++ { - p := nat.NewPort(proto, strconv.FormatUint(i, 10)) + p, err := nat.NewPort(proto, strconv.FormatUint(i, 10)) + if err != nil { + return nil, nil, cmd, err + } if _, exists := ports[p]; !exists { ports[p] = struct{}{} }