LGTM

LGTM

Looks like this introduced a change in behaviour moby/moby#41178 (comment)

If the image does not have an /etc/passwd file (e.g. the hello-world image), it now produces an error (perhaps it was a bug before? not sure). In the link above, it appears to fail on the default (root; 0:0 user)

If /etc/passwd is missing the intended behaviour that is numeric users should work as normal (and the default 0:0 should still work). But I'm a little confused how this caused the regression -- while errors wouldn't be returned before (which was a bug), you shouldn't get errors from the scanner if the file doesn't exist -- ENOENT is handled elsewhere in libcontainer/user...

@cyphar I didn't spend much time on digging deeper; I was looking at that PR, and this one touched the code that could be related, so I tried applying it and got the same failure. From your comment there, looks like the actual issue might be #2529 ?

@thaJeztah #2529 Can solve the problem that the privileged container cannot start. moby/moby#41178 (comment)

But the unable to find user 0: invalid argument error still exists.

root@9c15fcc51480:~# docker run --rm --privileged alpine whoami
root
root@9c15fcc51480:~# docker run --rm hello-world
docker: Error response from daemon: unable to find user 0: invalid argument.

Okay, I figured out the issue. It's because Docker is incorrectly using the API, and this PR just happened to expose this. The EINVAL is coming from the fact that a nil *os.File is being passed to GetExecUser but the checks are related to nil interfaces (which aren't the same thing in Go -- yay!). This patch to Docker should fix it:

From b6196eaebe884303e0f1198c1eebc50312868d44 Mon Sep 17 00:00:00 2001
From: Aleksa Sarai <[email protected]>
Date: Wed, 29 Jul 2020 12:43:43 +1000
Subject: [PATCH] oci: correctly use user.GetExecUser interface

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]>
---
 daemon/oci_linux.go | 9 ++++++++-
 1 file changed, 8 insertions(+), 1 deletion(-)

diff --git a/daemon/oci_linux.go b/daemon/oci_linux.go
index 44ac97d1d363..47970593d376 100644
--- a/daemon/oci_linux.go
+++ b/daemon/oci_linux.go
@@ -176,7 +176,14 @@ func readUserFile(c *container.Container, p string) (io.ReadCloser, error) {
 	if err != nil {
 		return nil, err
 	}
-	return os.Open(fp)
+	fh, err := os.Open(fp)
+	if err != nil {
+		// This is needed because a nil *os.File is different to a nil
+		// io.ReadCloser and this causes GetExecUser to not detect that the
+		// container file is missing.
+		return nil, err
+	}
+	return fh, nil
 }
 
 func getUser(c *container.Container, username string) (uint32, uint32, []uint32, error) {
-- 
2.27.0

Please add that patch to your PR @tao12345666333 (copy it to a file and apply it using git am < patch-file).

OK, Thanks!

Tested locally, it works well.

@cyphar wow... TIL. thanks!

Nice catch 👍