From a4a878dfdb264d659ae1d40a4512fb3bad531b13 Mon Sep 17 00:00:00 2001
From: Doug Davis <dug@us.ibm.com>
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 <dug@us.ibm.com>
---
 nat/nat.go      | 29 ++++++++++++++++++++++-------
 nat/nat_test.go | 11 ++++++++++-
 2 files changed, 32 insertions(+), 8 deletions(-)

diff --git a/nat/nat.go b/nat/nat.go
index 2cec2e8..3805b2e 100644
--- a/nat/nat.go
+++ b/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/nat/nat_test.go b/nat/nat_test.go
index 376857f..f17fda5 100644
--- a/nat/nat_test.go
+++ b/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) {