From 8717af7168e05632c834ba32826be52d21d6064b Mon Sep 17 00:00:00 2001 From: Madan Kumar Date: Sun, 2 Aug 2026 07:13:16 +0530 Subject: [PATCH] cli/command/service: fix panic when removing duplicate values Both makeEnv and updateHosts removed elements from a slice while ranging over that same slice. The range expression is evaluated once, so after a removal the loop keeps using the original length: it reads stale elements that shifted down, skips a live element, and can slice past the end of the shrunken slice, which panics. In makeEnv, the "no update required" continue also applied to the inner loop instead of skipping the re-append, so an env-var passed twice with the same value was stored twice. A third occurrence with a different value then tried to remove both entries and panicked: docker service create --env A=1 --env A=1 --env A=2 --name repro nginx panic: runtime error: slice bounds out of range [2:1] The same happens with an env-file that lists a variable twice, and the panic occurs before any API call, so no daemon is needed to hit it. updateHosts has the same problem when a hostname is listed more than once in a single entry: --host-rm either leaves a copy behind or panics with "slice bounds out of range". That needs a spec written through the API or swarmkit directly, as the CLI does not produce such entries itself, so it is less likely to be hit in practice. Use slices.DeleteFunc for both, which removes every match in one pass, and add tests for makeEnv, which had no coverage. Signed-off-by: Madan Kumar --- cli/command/service/opts.go | 15 +++++----- cli/command/service/opts_test.go | 46 ++++++++++++++++++++++++++++++ cli/command/service/update.go | 8 ++---- cli/command/service/update_test.go | 15 ++++++++++ 4 files changed, 71 insertions(+), 13 deletions(-) diff --git a/cli/command/service/opts.go b/cli/command/service/opts.go index ba2735ace259..da83f8108498 100644 --- a/cli/command/service/opts.go +++ b/cli/command/service/opts.go @@ -8,6 +8,7 @@ import ( "errors" "fmt" "net/netip" + "slices" "sort" "strconv" "strings" @@ -687,15 +688,13 @@ func (options *serviceOptions) makeEnv() ([]string, error) { } currentEnv := make([]string, 0, len(envVariables)) for _, env := range envVariables { // need to process each var, in order - k, _, _ := strings.Cut(env, "=") - for i, current := range currentEnv { // remove duplicates - if current == env { - continue // no update required, may hide this behind flag to preserve order of envVariables - } - if strings.HasPrefix(current, k+"=") { - currentEnv = append(currentEnv[:i], currentEnv[i+1:]...) - } + if slices.Contains(currentEnv, env) { + continue // no update required, may hide this behind flag to preserve order of envVariables } + k, _, _ := strings.Cut(env, "=") + currentEnv = slices.DeleteFunc(currentEnv, func(current string) bool { // remove duplicates + return strings.HasPrefix(current, k+"=") + }) currentEnv = append(currentEnv, env) } diff --git a/cli/command/service/opts_test.go b/cli/command/service/opts_test.go index d11d9a2e1aa9..5ab11e262b36 100644 --- a/cli/command/service/opts_test.go +++ b/cli/command/service/opts_test.go @@ -373,3 +373,49 @@ func TestToServiceSysCtls(t *testing.T) { assert.NilError(t, err) assert.Check(t, is.DeepEqual(service.TaskTemplate.ContainerSpec.Sysctls, expected)) } + +func TestMakeEnv(t *testing.T) { + tests := []struct { + doc string + env []string + expected []string + }{ + { + doc: "no duplicates", + env: []string{"one=1", "two=2"}, + expected: []string{"one=1", "two=2"}, + }, + { + doc: "same variable repeated", + env: []string{"one=1", "one=1"}, + expected: []string{"one=1"}, + }, + { + doc: "same variable repeated, then overridden", + env: []string{"one=1", "one=1", "one=2"}, + expected: []string{"one=2"}, + }, + { + doc: "repeated variable last", + env: []string{"one=1", "two=2", "two=2"}, + expected: []string{"one=1", "two=2"}, + }, + { + doc: "last value wins", + env: []string{"one=1", "two=2", "one=3"}, + expected: []string{"two=2", "one=3"}, + }, + } + + for _, tc := range tests { + t.Run(tc.doc, func(t *testing.T) { + o := newServiceOptions() + for _, env := range tc.env { + assert.NilError(t, o.env.Set(env)) + } + actual, err := o.makeEnv() + assert.NilError(t, err) + assert.Check(t, is.DeepEqual(tc.expected, actual)) + }) + } +} diff --git a/cli/command/service/update.go b/cli/command/service/update.go index 62f5376ae29a..13af4bd746cf 100644 --- a/cli/command/service/update.go +++ b/cli/command/service/update.go @@ -1213,11 +1213,9 @@ func updateHosts(flags *pflag.FlagSet, hosts *[]string) error { if rm.IPAddr != "" && rm.IPAddr != ip { continue } - for i, h := range hostNames { - if h == rm.Host { - hostNames = append(hostNames[:i], hostNames[i+1:]...) - } - } + hostNames = slices.DeleteFunc(hostNames, func(h string) bool { + return h == rm.Host + }) } if len(hostNames) > 0 { newHosts = append(newHosts, fmt.Sprintf("%s %s", ip, strings.Join(hostNames, " "))) diff --git a/cli/command/service/update_test.go b/cli/command/service/update_test.go index bb346b2e6fb9..6158820951b1 100644 --- a/cli/command/service/update_test.go +++ b/cli/command/service/update_test.go @@ -1727,3 +1727,18 @@ func TestUpdateUlimits(t *testing.T) { }) } } + +func TestUpdateHostsRemoveRepeatedHost(t *testing.T) { + flags := newUpdateCommand(nil).Flags() + flags.Set("host-rm", "host1") + + //nolint:dupword // ignore "Duplicate words (host1) found" + hosts := []string{"127.0.0.1 host1 host1 host2", "127.0.0.2 host2 host1 host1"} + + err := updateHosts(flags, &hosts) + assert.NilError(t, err) + + // All occurrences of `host1` should be removed, also if the same host + // is listed multiple times in the same entry. + assert.Check(t, is.DeepEqual([]string{"127.0.0.1 host2", "127.0.0.2 host2"}, hosts)) +}