Skip to content

oci: correctly use user.GetExecUser interface#41288

Merged
cpuguy83 merged 1 commit into
moby:masterfrom
thaJeztah:fix_getexecuser
Jul 29, 2020
Merged

oci: correctly use user.GetExecUser interface#41288
cpuguy83 merged 1 commit into
moby:masterfrom
thaJeztah:fix_getexecuser

Conversation

@thaJeztah

Copy link
Copy Markdown
Member

extracting this from #41178 as it's an existing bug. This patch was provided by @cyphar on opencontainers/runc#2275 (comment)

A nil interface in Go is not the same as a nil pointer that satisfies
the interface. libcontainer/user has special handling for missing
/etc/{passwd,group} files but this is all based on nil interface checks,
which were broken by Docker's usage of the API.

When combined with some recent changes in runc that made read errors
actually be returned to the caller, this results in spurrious -EINVAL
errors when we should detect the situation as "there is no passwd file".

- Description for the changelog

A nil interface in Go is not the same as a nil pointer that satisfies
the interface. libcontainer/user has special handling for missing
/etc/{passwd,group} files but this is all based on nil interface checks,
which were broken by Docker's usage of the API.

When combined with some recent changes in runc that made read errors
actually be returned to the caller, this results in spurrious -EINVAL
errors when we should detect the situation as "there is no passwd file".

Signed-off-by: Aleksa Sarai <[email protected]>
@thaJeztah

Copy link
Copy Markdown
Member Author

@kolyshkin @AkihiroSuda @tiborvass ptal

@thaJeztah

Copy link
Copy Markdown
Member Author

Also opened #41290 to refactor this somewhat

@kolyshkin kolyshkin left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

LGTM (and wow!)

@cpuguy83 cpuguy83 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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants