diff --git a/api/server/httputils/form.go b/api/server/httputils/form.go index b49d670651c8f..be7ec8756c4de 100644 --- a/api/server/httputils/form.go +++ b/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 { diff --git a/api/server/httputils/form_test.go b/api/server/httputils/form_test.go index a1543a1fed2f4..457af3a337b9b 100644 --- a/api/server/httputils/form_test.go +++ b/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 diff --git a/api/server/router/container/backend.go b/api/server/router/container/backend.go index e28b6d194aa20..4b495f04f9463 100644 --- a/api/server/router/container/backend.go +++ b/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 diff --git a/api/server/router/container/container_routes.go b/api/server/router/container/container_routes.go index 62e0efb20b598..a1bc9735d3db6 100644 --- a/api/server/router/container/container_routes.go +++ b/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 { diff --git a/api/server/router/container/exec.go b/api/server/router/container/exec.go index d8bbe3e77e2c3..fd2e403106b9e 100644 --- a/api/server/router/container/exec.go +++ b/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) } diff --git a/client/container_resize.go b/client/container_resize.go index 5cfd01d4798e3..6f1a8f5605cb7 100644 --- 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 index 76559ef928283..00960c17741f9 100644 --- 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(""))), diff --git a/daemon/resize.go b/daemon/resize.go index 3bd0e3b7bf190..bc2352b9cf437 100644 --- a/daemon/resize.go +++ b/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") } diff --git a/daemon/resize_test.go b/daemon/resize_test.go index ec3c710fda6fd..c916e706a2067 100644 --- a/daemon/resize_test.go +++ b/daemon/resize_test.go @@ -13,7 +13,11 @@ import ( // This test simply verify that when a wrong ID used, a specific error should be returned for exec resize. func TestExecResizeNoSuchExec(t *testing.T) { - n := "TestExecResize" + n := t.Name() + const ( + width = 24 + height = 8 + ) d := &Daemon{ execCommands: container.NewExecStore(), } @@ -25,7 +29,7 @@ func TestExecResizeNoSuchExec(t *testing.T) { Container: c, } d.registerExecCommand(c, ec) - err := d.ContainerExecResize("nil", 24, 8) + err := d.ContainerExecResize(context.TODO(), "no-such-exec", height, width) assert.ErrorContains(t, err, "No such exec instance") } @@ -42,9 +46,11 @@ func (p *execResizeMockProcess) Resize(ctx context.Context, width, height uint32 // This test is to make sure that when exec context is ready, resize should call ResizeTerminal via containerd client. func TestExecResize(t *testing.T) { - n := "TestExecResize" - width := 24 - height := 8 + n := t.Name() + const ( + width = 24 + height = 8 + ) mp := &execResizeMockProcess{} d := &Daemon{ execCommands: container.NewExecStore(), @@ -64,25 +70,28 @@ func TestExecResize(t *testing.T) { close(ec.Started) d.containers.Add(n, c) d.registerExecCommand(c, ec) - err := d.ContainerExecResize(n, height, width) + err := d.ContainerExecResize(context.TODO(), n, height, width) assert.NilError(t, err) assert.Equal(t, mp.Width, width) assert.Equal(t, mp.Height, height) } // This test is to make sure that when exec context is not ready, a timeout error should happen. +// // TODO: the expect running time for this test is 10s, which would be too long for unit test. func TestExecResizeTimeout(t *testing.T) { - n := "TestExecResize" - width := 24 - height := 8 + n := t.Name() + const ( + width = 24 + height = 8 + ) mp := &execResizeMockProcess{} d := &Daemon{ execCommands: container.NewExecStore(), containers: container.NewMemoryStore(), } c := &container.Container{ - ID: n, + ID: t.Name(), ExecCommands: container.NewExecStore(), State: &container.State{Running: true}, } @@ -94,6 +103,6 @@ func TestExecResizeTimeout(t *testing.T) { } d.containers.Add(n, c) d.registerExecCommand(c, ec) - err := d.ContainerExecResize(n, height, width) + err := d.ContainerExecResize(context.TODO(), n, height, width) assert.ErrorContains(t, err, "timeout waiting for exec session ready") } diff --git a/integration/container/exec_test.go b/integration/container/exec_test.go index 5f3fb7c4f3a30..e05bde8f5d208 100644 --- a/integration/container/exec_test.go +++ b/integration/container/exec_test.go @@ -159,35 +159,59 @@ func TestExecResize(t *testing.T) { doc: "unset height", height: valueNotSet, width: "100", - expErr: `strconv.Atoi: parsing "": invalid syntax`, + expErr: `invalid resize height "": invalid syntax`, }, { doc: "unset width", height: "100", width: valueNotSet, - expErr: `strconv.Atoi: parsing "": invalid syntax`, + expErr: `invalid resize width "": invalid syntax`, }, { doc: "empty height", width: "100", - expErr: `strconv.Atoi: parsing "": invalid syntax`, + expErr: `invalid resize height "": invalid syntax`, }, { doc: "empty width", height: "100", - expErr: `strconv.Atoi: parsing "": invalid syntax`, + expErr: `invalid resize width "": invalid syntax`, }, { doc: "non-numeric height", height: "not-a-number", width: "100", - expErr: `strconv.Atoi: parsing "not-a-number": invalid syntax`, + expErr: `invalid resize height "not-a-number": invalid syntax`, }, { doc: "non-numeric width", height: "100", width: "not-a-number", - expErr: `strconv.Atoi: parsing "not-a-number": invalid syntax`, + expErr: `invalid resize width "not-a-number": invalid syntax`, + }, + { + doc: "negative height", + height: "-100", + width: "100", + expErr: `invalid resize height "-100": value out of range`, + }, + { + doc: "negative width", + height: "100", + width: "-100", + expErr: `invalid resize width "-100": value out of range`, + }, + { + doc: "out of range height", + height: "4294967296", // math.MaxUint32+1 + width: "100", + expErr: `invalid resize height "4294967296": value out of range`, + }, + { + doc: "out of range width", + height: "100", + width: "4294967296", // math.MaxUint32+1 + expErr: `invalid resize width "4294967296": value out of range`, }, } for _, tc := range sizes { diff --git a/integration/container/resize_test.go b/integration/container/resize_test.go index 9f0ab60cd9f8f..7eaa9f5b957a4 100644 --- a/integration/container/resize_test.go +++ b/integration/container/resize_test.go @@ -48,35 +48,59 @@ func TestResize(t *testing.T) { doc: "unset height", height: valueNotSet, width: "100", - expErr: `strconv.Atoi: parsing "": invalid syntax`, + expErr: `invalid resize height "": invalid syntax`, }, { doc: "unset width", height: "100", width: valueNotSet, - expErr: `strconv.Atoi: parsing "": invalid syntax`, + expErr: `invalid resize width "": invalid syntax`, }, { doc: "empty height", width: "100", - expErr: `strconv.Atoi: parsing "": invalid syntax`, + expErr: `invalid resize height "": invalid syntax`, }, { doc: "empty width", height: "100", - expErr: `strconv.Atoi: parsing "": invalid syntax`, + expErr: `invalid resize width "": invalid syntax`, }, { doc: "non-numeric height", height: "not-a-number", width: "100", - expErr: `strconv.Atoi: parsing "not-a-number": invalid syntax`, + expErr: `invalid resize height "not-a-number": invalid syntax`, }, { doc: "non-numeric width", height: "100", width: "not-a-number", - expErr: `strconv.Atoi: parsing "not-a-number": invalid syntax`, + expErr: `invalid resize width "not-a-number": invalid syntax`, + }, + { + doc: "negative height", + height: "-100", + width: "100", + expErr: `invalid resize height "-100": value out of range`, + }, + { + doc: "negative width", + height: "100", + width: "-100", + expErr: `invalid resize width "-100": value out of range`, + }, + { + doc: "out of range height", + height: "4294967296", // math.MaxUint32+1 + width: "100", + expErr: `invalid resize height "4294967296": value out of range`, + }, + { + doc: "out of range width", + height: "100", + width: "4294967296", // math.MaxUint32+1 + expErr: `invalid resize width "4294967296": value out of range`, }, } for _, tc := range sizes {