Skip to content

fix docker cp -a failing to access / in container (run getent with a noop stdin)#45720

Merged
thaJeztah merged 1 commit intomoby:masterfrom
ndeloof:copy_uidgid
Jun 13, 2023
Merged

fix docker cp -a failing to access / in container (run getent with a noop stdin)#45720
thaJeztah merged 1 commit intomoby:masterfrom
ndeloof:copy_uidgid

Conversation

@ndeloof
Copy link
Copy Markdown
Contributor

@ndeloof ndeloof commented Jun 9, 2023

fixes #45719 (actually, just a workaround)

- What I did
configure command to run getent to use a noop readCloser as stdin to prevent ENOENT failure to open /dev/null

- How I did it

- How to verify it
test case included

- Description for the changelog

- A picture of a cute animal (not mandatory but encouraged)
image

@ndeloof ndeloof changed the title test case fix docker cp -a failing to access / in container Jun 9, 2023
@ndeloof ndeloof marked this pull request as ready for review June 9, 2023 14:42
Copy link
Copy Markdown

@glours glours left a comment

Choose a reason for hiding this comment

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

LGTM

@cpuguy83
Copy link
Copy Markdown
Member

cpuguy83 commented Jun 9, 2023

Seems like a strange thing:

$ getent passwd 1000 < /dev/null
cpuguy83:x:1000:1000:Ubuntu:/home/cpuguy83:/bin/bash

Also doing the same in go works just fine as well.

@neersighted
Copy link
Copy Markdown
Member

I'm not convinced this is right at all either -- if nil Stdin is breaking something, I think that represents a regression in the Go runtime and not something to work around in Moby.

@ndeloof
Copy link
Copy Markdown
Contributor Author

ndeloof commented Jun 9, 2023

@neersighted I totally agree with you, this is at most a temporary workaround. Maybe this deserve a FIXME note
I have no explanation why os.Open(os.DevNull) fails in this specific scenario with:

fs.PathError {
Op = {string} "open"
Path = {string} "/dev/null"
Err = {error | syscall.Errno} github.com/docker/docker/vendor/github.com/cilium/ebpf/internal/unix.ENOENT (2)
}

but I haven't found any bug reported to the golang team related to such an issue, and have no idea how to investigate further.

@rumpl
Copy link
Copy Markdown
Member

rumpl commented Jun 9, 2023

During copy the container filesystem is mounted and the archive code gets a view of that filesystem, /dev is not mounted there, that's why you get this error.

I tried adding a simple

if err := mount.Mount("/dev", dest, "tmpfs", "rbind"); err != nil {
	return err
}

But then the copy fails with getent unable to find entry "1000" in passwd database, which is the same error I get when I run cp -a on a 23.x version.

@neersighted
Copy link
Copy Markdown
Member

It appears this isn't a regression then, but a subtle breaking change where the Go runtime depends on the /dev/null device node being available. I wonder if it's worth the trouble to provide some basic device nodes during copy -- likely this is more pragmatic, but it seems possible that we could run into similar issues in the future.

@ndeloof
Copy link
Copy Markdown
Contributor Author

ndeloof commented Jun 10, 2023

according to #45720 (comment) this is actually a reasonable fix, but need to add some comment to explain the reason for this. I'll update my PR on Monday

Signed-off-by: Nicolas De Loof <nicolas.deloof@gmail.com>
}
out, err := exec.Command(getentCmd, database, key).CombinedOutput()
command := exec.Command(getentCmd, database, key)
// we run getent within container filesystem, but without /dev so /dev/null is not available for exec to mock stdin
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.

'close' might be more accurate than 'mock'; but this is fine.

@cpuguy83 cpuguy83 added status/4-merge area/daemon Core Engine kind/bug Bugs are bugs. The cause may or may not be known at triage time so debugging may be needed. kind/bugfix PR's that fix bugs and removed kind/bug Bugs are bugs. The cause may or may not be known at triage time so debugging may be needed. labels Jun 12, 2023
@thaJeztah thaJeztah added this to the 25.0.0 milestone Jun 13, 2023
Copy link
Copy Markdown
Member

@thaJeztah thaJeztah left a comment

Choose a reason for hiding this comment

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

LGTM

Comment on lines +72 to +74
// empty content
dstDir, _ := makeEmptyArchive(t)
err := apiclient.CopyToContainer(ctx, cid, dstDir, bytes.NewReader([]byte("")), types.CopyToContainerOptions{})
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.

Not a blocker; we probably could've used subtests for these

// empty content
dstDir, _ := makeEmptyArchive(t)
err := apiclient.CopyToContainer(ctx, cid, dstDir, bytes.NewReader([]byte("")), types.CopyToContainerOptions{})
assert.NilError(t, err)
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.

Not a blocker; using assert.Check() allows the test to continue (assert.NilError() does a t.Fatal(), which in this case means we won't test the other cases if an earlier one fails)

Suggested change
assert.NilError(t, err)
assert.Check(t, err)

@thaJeztah thaJeztah merged commit a978888 into moby:master Jun 13, 2023
@thaJeztah thaJeztah changed the title fix docker cp -a failing to access / in container fix docker cp -a failing to access / in container (run getent with a noop stdin) Jun 14, 2023
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.

docker cp -a broken in 24.0

6 participants