cli/command/service: fix panic when removing duplicate values

[winklemad]

- What I did

Fixed a panic in docker service create / docker service update when the same environment variable is passed more than once, and the same bug class in updateHosts.

Both (*serviceOptions).makeEnv and updateHosts removed elements from a slice with s = append(s[:i], s[i+1:]...) 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 there was a second problem feeding the first: the continue for the "no update required" case continued the inner loop instead of skipping the re-append, so an env var passed twice with the same value was stored twice — contradicting the // remove duplicates comment. A third occurrence with a different value then tried to remove both copies and ran off the end:

$ 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]

goroutine 1 [running]:
github.com/docker/cli/cli/command/service.(*serviceOptions).makeEnv(...)
	/go/src/github.com/docker/cli/cli/command/service/opts.go:696
github.com/docker/cli/cli/command/service.(*serviceOptions).ToService(...)
	/go/src/github.com/docker/cli/cli/command/service/opts.go:715

makeEnv is the first statement of ToService, so this happens before any API call — no daemon is needed to hit it, and the same applies to an --env-file that lists a variable twice.

updateHosts has the same defect when a hostname appears more than once in a single entry: --host-rm either leaves a copy behind or panics with slice bounds out of range. To be upfront about the impact: the CLI itself does not produce such entries, so reaching that one needs a spec written through the API or swarmkit directly. It is fixed here because it is literally the same bug two files apart, not because I can show it from the CLI.

- How I did it

Replaced both hand-rolled removal loops with slices.DeleteFunc, which removes every match in a single pass and cannot leave a stale index behind. In makeEnv the "already present, unchanged" check moved out to the top of the outer loop, which is where the original continue comment was clearly aiming — so an exact repeat is now skipped instead of appended twice. Behaviour is otherwise unchanged: vars are still processed in order and the last value for a key still wins.

I also swept the rest of the tree for the same pattern. The only other in-place removals are stats.remove (cli/command/container/stats_helpers.go) and ListOpts.Delete (opts/opts.go); neither removes inside a range over the slice it mutates, so both are correct and are left alone.

- How to verify it

Before this change, on master:

$ go build -o dockercli ./cmd/docker
$ ./dockercli service create --env A=1 --env A=1 --env A=2 --name repro nginx
panic: runtime error: slice bounds out of range [2:1]

(also reproducible with printf 'A=1\nA=1\nA=2\n' > /tmp/e && ./dockercli service create --env-file /tmp/e ...)

After it, the command proceeds normally and the service gets a single A=2.

makeEnv had no test coverage at all, so this adds TestMakeEnv covering the repeat, repeat-then-override, repeat-at-last-position and no-duplicate cases, plus TestUpdateHostsRemoveRepeatedHost for the --host-rm side. Both fail on master (the override case and the host case panic) and pass with the fix:

$ go test ./cli/command/service/... 
ok  	github.com/docker/cli/cli/command/service
ok  	github.com/docker/cli/cli/command/service/internal/genericresource
ok  	github.com/docker/cli/cli/command/service/progress

The existing TestUpdateHosts* tests are untouched and still pass, and golangci-lint run ./cli/command/service/... is clean.

- Human readable description for the release notes

I left the changelog block below empty on purpose: check-changelog fails a PR that fills it without an impact/ label, and labels are maintainer-applied. If you'd like it in the release notes, the line I'd suggest is:

Fix docker service create and docker service update panicking when the same environment variable is passed more than once.

Happy to move that into the block once a label is applied.

[Copilot]

Pull request overview

Fixes a panic in service env/hosts option processing by replacing unsafe in-place slice deletions during range iteration with safe slice filtering, and adds regression tests to cover the duplicate-input cases.

Changes:

• Update updateHosts to remove hostnames using slices.DeleteFunc to avoid index invalidation and missed removals.
• Update (*serviceOptions).makeEnv to safely de-duplicate env vars while preserving “last value wins” semantics and skipping exact repeats.
• Add focused tests covering repeated env vars and repeated host entries to prevent regressions.

Reviewed changes

Copilot reviewed 4 out of 4 changed files in this pull request and generated no comments.

File	Description
cli/command/service/update.go	Switch host removal logic to slices.DeleteFunc to avoid slice-mutation-during-range panics and ensure all duplicates are removed.
cli/command/service/update_test.go	Add regression test ensuring --host-rm removes repeated hostnames within a single hosts entry.
cli/command/service/opts.go	Rework env de-duplication to avoid unsafe slice mutation during iteration and correctly skip exact duplicates.
cli/command/service/opts_test.go	Add TestMakeEnv to cover no-dup, exact-repeat, repeat-then-override, and ordering cases.

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[codecov-commenter]

Codecov Report

✅ All modified and coverable lines are covered by tests.

📢 Thoughts on this report? Let us know!