Visitar URL original
user: Fix GetAdditionalGroups panic on empty group name by vvoland · Pull Request #278 · moby/sys · GitHub
Skip to content

user: Fix GetAdditionalGroups panic on empty group name - #278

Closed
vvoland wants to merge 2 commits into
moby:mainfrom
vvoland:fix-panic
Closed

user: Fix GetAdditionalGroups panic on empty group name#278
vvoland wants to merge 2 commits into
moby:mainfrom
vvoland:fix-panic

Conversation

@vvoland

@vvoland vvoland commented Nov 5, 2026 •
edited by thaJeztah
Loading

Copy link
Copy Markdown
Contributor

Passing an empty string in additionalGroups caused a nil pointer dereference.

@vvoland
user: Fix GetAdditionalGroups panic on empty group name
6d53161 
Passing an empty string in additionalGroups caused a nil pointer
dereference.

Signed-off-by: Paweł Gronowski <pawel.gronowski@docker.com>
@vvoland vvoland self-assigned this Nov 5, 2026
thaJeztah
thaJeztah reviewed Nov 5, 2026
View reviewed changes
Comment thread user/user.go Outdated
Comment on lines 401 to 404
if groupArg == nil {
return nil, fmt.Errorf("unable to find group %s: %w", ag, ErrNoGroupEntries)
}
addtlGroups = append(addtlGroups, *groupArg)

@thaJeztah thaJeztah Nov 5, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wondering if we should either return an error from parseGroupArg(ag) if it's an empty string, or if we should gracefully handle an empty result here (additional groups probably shouldn't be required, so can be empty)?

cc @kolyshkin

@thaJeztah thaJeztah Nov 5, 2026 •
edited
Loading

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The overall problem looks to be basically the nil, nil return; although that's documented as part of the contract;

sys/user/user_utils.go

Lines 106 to 111 in 944ed87

// If name is empty, parseGroupArg returns "nil, nil" to indicate that no group
// argument was specified.
func parseGroupArg(name string) (*groupArg, error) {
if name == "" {
return nil, nil
}

sys/user/user_utils.go

Lines 75 to 80 in 944ed87

// If name is empty, parseUserArg returns "nil, nil" to indicate that no user
// argument was specified.
func parseUserArg(name string) (*userArg, error) {
if name == "" {
return nil, nil
}

@kolyshkin kolyshkin Nov 6, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah I think we should return an error from these two. Haven't checked all the users though

@vvoland vvoland Nov 9, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

@thaJeztah

thaJeztah commented Nov 10, 2026

Copy link
Copy Markdown
Member

Looks like that floating tag now bites us, @kolyshkin

   INFO zizmor: 🌈 zizmor v1.29.0
   INFO audit: zizmor: 🌈 completed ./.github/dependabot.yml
   INFO audit: zizmor: 🌈 completed ./.github/workflows/actionlint.yml
  Error: actionlint.yml:26: commit with no history in referenced repository: uses a commit that doesn't belong to the specified org/repo
   INFO audit: zizmor: 🌈 completed ./.github/workflows/test.yml
  Warning: actionlint.yml:26: action's hash pin has mismatched or missing version comment: points to commit 1a9278318c43
  Error: Process completed with exit code 14.

@thaJeztah

thaJeztah commented Nov 10, 2026

Copy link
Copy Markdown
Member

opened a PR to fix CI;

@thaJeztah thaJeztah mentioned this pull request Nov 10, 2026
Open
@kolyshkin

kolyshkin commented Nov 11, 2026

Copy link
Copy Markdown
Collaborator

Hmm, just noticed it intersects with PR #276, in particular 1135d9a. I'm fine either way

thaJeztah
thaJeztah reviewed Nov 11, 2026
View reviewed changes
Comment thread user/user.go Resolved
@thaJeztah

thaJeztah commented Nov 11, 2026

Copy link
Copy Markdown
Member

Hmm, just noticed it intersects with PR #276, in particular 1135d9a. I'm fine either way

Ah, yes, was wondering that (also didn't have time yet to look at your PR; I know I was going a bit back-and-forth on my PR, so curious what you ended up with).

I think it'd make sense to get this PR in first; it's fixing a regression, so probably should be the first to address before we look at the improvements in the other PR.

(That said, not sure if we'll release them separately, so perhaps doesn't really matter).

@vvoland
user: Reject empty names in parseUserArg and parseGroupArg
2588f62 
Both helpers now return an error for an empty name instead of
"nil, nil", and their callers check for an empty name first.
Behavior of GetExecUser and GetAdditionalGroups is unchanged.

Signed-off-by: Paweł Gronowski <pawel.gronowski@docker.com>
thaJeztah
thaJeztah approved these changes Nov 12, 2026
View reviewed changes

@thaJeztah thaJeztah left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@thaJeztah
thaJeztah requested a balanced review from Copilot November 12, 2026 13:30
Copilot AI reviewed Nov 12, 2026
View reviewed changes

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟢 Approval recommended

There are no active review issues for the change addressing the empty group-name panic.

No open issues 🎉

What changed in this PR

Fixes the GetAdditionalGroups panic on an empty group name by returning an error instead of a nil argument, while preserving optional user and group handling in GetExecUser.

Changes:

  • Reject empty names in the user and group argument parsers.
  • Skip parsing omitted user and group values in GetExecUser.
  • Add regression coverage for an empty additional group name.
File Description
user/​user.go Preserves handling of omitted user and group values.
user/​user_utils.go Returns errors for empty user and group arguments.
user/​user_test.go Tests rejection of an empty additional group name.

Review effort: Balanced

@kolyshkin

kolyshkin commented Nov 13, 2026

Copy link
Copy Markdown
Collaborator

(That said, not sure if we'll release them separately, so perhaps doesn't really matter).

OK now that I'm out with those nasty CVEs, I've redone #276; PTAL

@kolyshkin kolyshkin mentioned this pull request Nov 13, 2026
Open
@vvoland vvoland closed this Nov 13, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@kolyshkin kolyshkin kolyshkin left review comments
Copilot code review Copilot
Copilot review effort
Applies to this pull request for everyone.Learn more about Copilot code review.
Copilot left review comments
@thaJeztah thaJeztah thaJeztah approved these changes

Assignees

@vvoland vvoland

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@vvoland @thaJeztah @kolyshkin