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 <dug@us.ibm.com>

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)
 			}

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)
 		}

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") {

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,

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",

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 {

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{}{}
 			}

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) {

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{})

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{

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{}{}
 			}