Merge pull request moby#48679 from thaJeztah/resize_uint

api: container, exec resize: improve errors for invalid width/height

api/server/httputils/form.go

@@ -29,6 +29,29 @@ func BoolValueOrDefault(r *http.Request, k string, d bool) bool {
 	return BoolValue(r, k)
 }
 
+// Uint32Value parses a form value into an uint32 type. It returns an error
+// if the field is not set, empty, incorrectly formatted, or out of range.
+func Uint32Value(r *http.Request, field string) (uint32, error) {
+	// strconv.ParseUint returns an "strconv.ErrSyntax" for negative values,
+	// not an "out of range". Strip the prefix before parsing, and use it
+	// later to detect valid, but negative values.
+	v, isNeg := strings.CutPrefix(r.Form.Get(field), "-")
+	if v == "" || v[0] == '+' {
+		// Fast-path for invalid values.
+		return 0, strconv.ErrSyntax
+	}
+
+	i, err := strconv.ParseUint(v, 10, 32)
+	if err != nil {
+		// Unwrap to remove the 'strconv.ParseUint: parsing "some-invalid-value":' prefix.
+		return 0, errors.Unwrap(err)
+	}
+	if isNeg {
+		return 0, strconv.ErrRange
+	}
+	return uint32(i), nil
+}
+
 // Int64ValueOrZero parses a form value into an int64 type.
 // It returns 0 if the parsing fails.
 func Int64ValueOrZero(r *http.Request, k string) int64 {

api/server/httputils/form_test.go

@@ -1,8 +1,10 @@
 package httputils // import "github.com/docker/docker/api/server/httputils"
 
 import (
+	"math"
 	"net/http"
 	"net/url"
+	"strconv"
 	"testing"
 
 	"github.com/docker/docker/errdefs"
@@ -109,6 +111,58 @@ func TestInt64ValueOrDefaultWithError(t *testing.T) {
 	}
 }
 
+func TestUint32Value(t *testing.T) {
+	const valueNotSet = "unset"
+
+	tests := []struct {
+		value       string
+		expected    uint32
+		expectedErr error
+	}{
+		{
+			value:    "0",
+			expected: 0,
+		},
+		{
+			value:    strconv.FormatUint(math.MaxUint32, 10),
+			expected: math.MaxUint32,
+		},
+		{
+			value:       valueNotSet,
+			expectedErr: strconv.ErrSyntax,
+		},
+		{
+			value:       "",
+			expectedErr: strconv.ErrSyntax,
+		},
+		{
+			value:       "-1",
+			expectedErr: strconv.ErrRange,
+		},
+		{
+			value:       "4294967296", // MaxUint32+1
+			expectedErr: strconv.ErrRange,
+		},
+		{
+			value:       "not-a-number",
+			expectedErr: strconv.ErrSyntax,
+		},
+	}
+	for _, tc := range tests {
+		tc := tc
+		t.Run(tc.value, func(t *testing.T) {
+			r, _ := http.NewRequest(http.MethodPost, "", nil)
+			r.Form = url.Values{}
+			if tc.value != valueNotSet {
+				r.Form.Set("field", tc.value)
+			}
+			out, err := Uint32Value(r, "field")
+			assert.Check(t, is.Equal(tc.expected, out))
+			assert.Check(t, is.ErrorIs(err, tc.expectedErr))
+		})
+	}
+}
+
 func TestDecodePlatform(t *testing.T) {
 	tests := []struct {
 		doc          string

api/server/router/container/backend.go

@@ -15,7 +15,7 @@ import (
 type execBackend interface {
 	ContainerExecCreate(name string, options *container.ExecOptions) (string, error)
 	ContainerExecInspect(id string) (*backend.ExecInspect, error)
-	ContainerExecResize(name string, height, width int) error
+	ContainerExecResize(ctx context.Context, name string, height, width uint32) error
 	ContainerExecStart(ctx context.Context, name string, options backend.ExecStartConfig) error
 	ExecExists(name string) (bool, error)
 }
@@ -34,7 +34,7 @@ type stateBackend interface {
 	ContainerKill(name string, signal string) error
 	ContainerPause(name string) error
 	ContainerRename(oldName, newName string) error
-	ContainerResize(name string, height, width int) error
+	ContainerResize(ctx context.Context, name string, height, width uint32) error
 	ContainerRestart(ctx context.Context, name string, options container.StopOptions) error
 	ContainerRm(name string, config *backend.ContainerRmConfig) error
 	ContainerStart(ctx context.Context, name string, checkpoint string, checkpointDir string) error

api/server/router/container/container_routes.go

@@ -899,16 +899,16 @@ func (c *containerRouter) postContainersResize(ctx context.Context, w http.Respo
 		return err
 	}
 
-	height, err := strconv.Atoi(r.Form.Get("h"))
+	height, err := httputils.Uint32Value(r, "h")
 	if err != nil {
-		return errdefs.InvalidParameter(err)
+		return errdefs.InvalidParameter(errors.Wrapf(err, "invalid resize height %q", r.Form.Get("h")))
 	}
-	width, err := strconv.Atoi(r.Form.Get("w"))
+	width, err := httputils.Uint32Value(r, "w")
 	if err != nil {
-		return errdefs.InvalidParameter(err)
+		return errdefs.InvalidParameter(errors.Wrapf(err, "invalid resize width %q", r.Form.Get("w")))
 	}
 
-	return c.backend.ContainerResize(vars["name"], height, width)
+	return c.backend.ContainerResize(ctx, vars["name"], height, width)
 }
 
 func (c *containerRouter) postContainersAttach(ctx context.Context, w http.ResponseWriter, r *http.Request, vars map[string]string) error {

api/server/router/container/exec.go

@@ -5,7 +5,6 @@ import (
 	"fmt"
 	"io"
 	"net/http"
-	"strconv"
 
 	"github.com/containerd/log"
 	"github.com/docker/docker/api/server/httputils"
@@ -15,6 +14,7 @@ import (
 	"github.com/docker/docker/api/types/versions"
 	"github.com/docker/docker/errdefs"
 	"github.com/docker/docker/pkg/stdcopy"
+	"github.com/pkg/errors"
 )
 
 func (c *containerRouter) getExecByID(ctx context.Context, w http.ResponseWriter, r *http.Request, vars map[string]string) error {
@@ -158,14 +158,14 @@ func (c *containerRouter) postContainerExecResize(ctx context.Context, w http.Re
 	if err := httputils.ParseForm(r); err != nil {
 		return err
 	}
-	height, err := strconv.Atoi(r.Form.Get("h"))
+	height, err := httputils.Uint32Value(r, "h")
 	if err != nil {
-		return errdefs.InvalidParameter(err)
+		return errdefs.InvalidParameter(errors.Wrapf(err, "invalid resize height %q", r.Form.Get("h")))
 	}
-	width, err := strconv.Atoi(r.Form.Get("w"))
+	width, err := httputils.Uint32Value(r, "w")
 	if err != nil {
-		return errdefs.InvalidParameter(err)
+		return errdefs.InvalidParameter(errors.Wrapf(err, "invalid resize width %q", r.Form.Get("w")))
 	}
 
-	return c.backend.ContainerExecResize(vars["name"], height, width)
+	return c.backend.ContainerExecResize(ctx, vars["name"], height, width)
 }

client/container_resize.go

@@ -19,9 +19,10 @@ func (cli *Client) ContainerExecResize(ctx context.Context, execID string, optio
 }
 
 func (cli *Client) resize(ctx context.Context, basePath string, height, width uint) error {
+	// FIXME(thaJeztah): the API / backend accepts uint32, but container.ResizeOptions uses uint.
 	query := url.Values{}
-	query.Set("h", strconv.Itoa(int(height)))
-	query.Set("w", strconv.Itoa(int(width)))
+	query.Set("h", strconv.FormatUint(uint64(height), 10))
+	query.Set("w", strconv.FormatUint(uint64(width), 10))
 
 	resp, err := cli.post(ctx, basePath+"/resize", query, nil, nil)
 	ensureReaderClosed(resp)

client/container_resize_test.go

@@ -3,10 +3,9 @@ package client // import "github.com/docker/docker/client"
 import (
 	"bytes"
 	"context"
-	"fmt"
 	"io"
+	"math"
 	"net/http"
-	"strings"
 	"testing"
 
 	"github.com/docker/docker/api/types/container"
@@ -32,47 +31,99 @@ func TestContainerExecResizeError(t *testing.T) {
 }
 
 func TestContainerResize(t *testing.T) {
-	client := &Client{
-		client: newMockClient(resizeTransport("/containers/container_id/resize")),
-	}
+	const expectedURL = "/containers/container_id/resize"
 
-	err := client.ContainerResize(context.Background(), "container_id", container.ResizeOptions{
-		Height: 500,
-		Width:  600,
-	})
-	if err != nil {
-		t.Fatal(err)
+	tests := []struct {
+		doc                           string
+		opts                          container.ResizeOptions
+		expectedHeight, expectedWidth string
+	}{
+		{
+			doc:            "zero width height", // valid, but not very useful
+			opts:           container.ResizeOptions{},
+			expectedWidth:  "0",
+			expectedHeight: "0",
+		},
+		{
+			doc: "valid resize",
+			opts: container.ResizeOptions{
+				Height: 500,
+				Width:  600,
+			},
+			expectedHeight: "500",
+			expectedWidth:  "600",
+		},
+		{
+			doc: "larger than maxint64",
+			opts: container.ResizeOptions{
+				Height: math.MaxInt64 + 1,
+				Width:  math.MaxInt64 + 2,
+			},
+			expectedHeight: "9223372036854775808",
+			expectedWidth:  "9223372036854775809",
+		},
+	}
+	for _, tc := range tests {
+		t.Run(tc.doc, func(t *testing.T) {
+			client := &Client{
+				client: newMockClient(resizeTransport(t, expectedURL, tc.expectedHeight, tc.expectedWidth)),
+			}
+			err := client.ContainerResize(context.Background(), "container_id", tc.opts)
+			assert.Check(t, err)
+		})
 	}
 }
 
 func TestContainerExecResize(t *testing.T) {
-	client := &Client{
-		client: newMockClient(resizeTransport("/exec/exec_id/resize")),
+	const expectedURL = "/exec/exec_id/resize"
+	tests := []struct {
+		doc                           string
+		opts                          container.ResizeOptions
+		expectedHeight, expectedWidth string
+	}{
+		{
+			doc:            "zero width height", // valid, but not very useful
+			opts:           container.ResizeOptions{},
+			expectedWidth:  "0",
+			expectedHeight: "0",
+		},
+		{
+			doc: "valid resize",
+			opts: container.ResizeOptions{
+				Height: 500,
+				Width:  600,
+			},
+			expectedHeight: "500",
+			expectedWidth:  "600",
+		},
+		{
+			doc: "larger than maxint64",
+			opts: container.ResizeOptions{
+				Height: math.MaxInt64 + 1,
+				Width:  math.MaxInt64 + 2,
+			},
+			expectedHeight: "9223372036854775808",
+			expectedWidth:  "9223372036854775809",
+		},
 	}
-
-	err := client.ContainerExecResize(context.Background(), "exec_id", container.ResizeOptions{
-		Height: 500,
-		Width:  600,
-	})
-	if err != nil {
-		t.Fatal(err)
+	for _, tc := range tests {
+		t.Run(tc.doc, func(t *testing.T) {
+			client := &Client{
+				client: newMockClient(resizeTransport(t, expectedURL, tc.expectedHeight, tc.expectedWidth)),
+			}
+			err := client.ContainerExecResize(context.Background(), "exec_id", tc.opts)
+			assert.Check(t, err)
+		})
 	}
 }
 
-func resizeTransport(expectedURL string) func(req *http.Request) (*http.Response, error) {
+func resizeTransport(t *testing.T, expectedURL, expectedHeight, expectedWidth string) func(req *http.Request) (*http.Response, error) {
 	return func(req *http.Request) (*http.Response, error) {
-		if !strings.HasPrefix(req.URL.Path, expectedURL) {
-			return nil, fmt.Errorf("Expected URL '%s', got '%s'", expectedURL, req.URL)
-		}
+		assert.Check(t, is.Equal(req.URL.Path, expectedURL))
+
 		query := req.URL.Query()
-		h := query.Get("h")
-		if h != "500" {
-			return nil, fmt.Errorf("h not set in URL query properly. Expected '500', got %s", h)
-		}
-		w := query.Get("w")
-		if w != "600" {
-			return nil, fmt.Errorf("w not set in URL query properly. Expected '600', got %s", w)
-		}
+		assert.Check(t, is.Equal(query.Get("h"), expectedHeight))
+		assert.Check(t, is.Equal(query.Get("w"), expectedWidth))
 		return &http.Response{
 			StatusCode: http.StatusOK,
 			Body:       io.NopCloser(bytes.NewReader([]byte(""))),

daemon/resize.go

@@ -12,7 +12,7 @@ import (
 
 // ContainerResize changes the size of the TTY of the process running
 // in the container with the given name to the given height and width.
-func (daemon *Daemon) ContainerResize(name string, height, width int) error {
+func (daemon *Daemon) ContainerResize(ctx context.Context, name string, height, width uint32) error {
 	container, err := daemon.GetContainer(name)
 	if err != nil {
 		return err
@@ -25,10 +25,10 @@ func (daemon *Daemon) ContainerResize(name string, height, width int) error {
 		return err
 	}
 
-	if err = tsk.Resize(context.Background(), uint32(width), uint32(height)); err == nil {
+	if err = tsk.Resize(context.WithoutCancel(ctx), width, height); err == nil {
 		daemon.LogContainerEventWithAttributes(container, events.ActionResize, map[string]string{
-			"height": strconv.Itoa(height),
-			"width":  strconv.Itoa(width),
+			"height": strconv.FormatUint(uint64(height), 10),
+			"width":  strconv.FormatUint(uint64(width), 10),
 		})
 	}
 	return err
@@ -37,7 +37,7 @@ func (daemon *Daemon) ContainerResize(name string, height, width int) error {
 // ContainerExecResize changes the size of the TTY of the process
 // running in the exec with the given name to the given height and
 // width.
-func (daemon *Daemon) ContainerExecResize(name string, height, width int) error {
+func (daemon *Daemon) ContainerExecResize(ctx context.Context, name string, height, width uint32) error {
 	ec, err := daemon.getExecConfig(name)
 	if err != nil {
 		return err
@@ -54,7 +54,7 @@ func (daemon *Daemon) ContainerExecResize(name string, height, width int) error
 		if ec.Process == nil {
 			return errdefs.InvalidParameter(errors.New("exec process is not started"))
 		}
-		return ec.Process.Resize(context.Background(), uint32(width), uint32(height))
+		return ec.Process.Resize(context.WithoutCancel(ctx), width, height)
 	case <-timeout.C:
 		return errors.New("timeout waiting for exec session ready")
 	}