client: ContainerResize, ContainerExecResize: don't overflow width/he…

…ight Mostly theoretical, but let's be correct here. It's worth noting that the API (backend) accepts uint32, but container.ResizeOptions uses uint (uint64). We could decide to add checks for this on the client side, or to change the type (but that would be a breaking change). Signed-off-by: Sebastiaan van Stijn <[email protected]>
thaJeztah · Oct 17, 2024 · abed0e1 · abed0e1
1 parent ed44a05
commit abed0e1
Show file tree

Hide file tree

Showing 2 changed files with 86 additions and 34 deletions.
diff --git a/client/container_resize.go b/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)

diff --git a/client/container_resize_test.go b/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(""))),