Skip to content
This repository was archived by the owner on Oct 13, 2023. It is now read-only.

Conversation

@thaJeztah
Copy link
Member

@thaJeztah thaJeztah commented Jun 18, 2019

backport of moby#39357 for 18.09

Another attempt at fixing moby#39348
Fixes moby#39348
Previous attempt at moby#39351

Before 7a7357d, archive.TarResourceRebase was being used to copy files
and folders from the container. That function splits the source path
into a dirname + basename pair to support copying a file:
if you wanted to tar dir/file it would tar from dir the file file
(as part of the IncludedFiles option).

However, that path splitting logic was kept for folders as well, which
resulted in weird inputs to archive.TarWithOptions:
if you wanted to tar dir1/dir2 it would tar from dir1 the directory
dir2 (as part of IncludedFiles option).

Although it was weird, it worked fine until we started chrooting into
the container rootfs when doing a docker cp with container source set
to / (cf 3029e76 (moby#39292)).

The fix is to only do the path splitting logic if the source is a file.

Unfortunately, 7a7357d added support for LCOW by duplicating some of
this subtle logic. Ideally we would need to do more refactoring of the
archive codebase to properly encapsulate these behaviors behind well-
documented APIs.

This fix does not do that. Instead, it fixes the issue inline.

Signed-off-by: Tibor Vass [email protected]

I added a couple of more tests than the actual issue needs, just to make sure there are no other regressions compared to before the cve fix (3029e76).

Huge thanks to @cpuguy83 ❤️who worked tirelessly with me to understand the code and make this PR.

cpuguy83 and others added 4 commits June 18, 2019 14:43
CID=$(docker create alpine)
docker cp $CID:/ out

Signed-off-by: Brian Goff <[email protected]>
Signed-off-by: Tibor Vass <[email protected]>
(cherry picked from commit 6db9f1c)
Signed-off-by: Sebastiaan van Stijn <[email protected]>
Signed-off-by: Tibor Vass <[email protected]>
(cherry picked from commit 02f1eb8)
Signed-off-by: Sebastiaan van Stijn <[email protected]>
Before 7a7357d, archive.TarResourceRebase was being used to copy files
and folders from the container. That function splits the source path
into a dirname + basename pair to support copying a file:
if you wanted to tar `dir/file` it would tar from `dir` the file `file`
(as part of the IncludedFiles option).

However, that path splitting logic was kept for folders as well, which
resulted in weird inputs to archive.TarWithOptions:
if you wanted to tar `dir1/dir2` it would tar from `dir1` the directory
`dir2` (as part of IncludedFiles option).

Although it was weird, it worked fine until we started chrooting into
the container rootfs when doing a `docker cp` with container source set
to `/` (cf 3029e76).

The fix is to only do the path splitting logic if the source is a file.

Unfortunately, 7a7357d added support for LCOW by duplicating some of
this subtle logic. Ideally we would need to do more refactoring of the
archive codebase to properly encapsulate these behaviors behind well-
documented APIs.

This fix does not do that. Instead, it fixes the issue inline.

Signed-off-by: Tibor Vass <[email protected]>
(cherry picked from commit 171538c)
Signed-off-by: Sebastiaan van Stijn <[email protected]>
Previously, getWalkRoot("/", "foo") would return "//foo"
Now it returns "/foo"

Signed-off-by: Tibor Vass <[email protected]>
(cherry picked from commit 7410f1a)
Signed-off-by: Sebastiaan van Stijn <[email protected]>
@thaJeztah thaJeztah added this to the 18.09.7 milestone Jun 18, 2019
@thaJeztah
Copy link
Member Author

ping @tiborvass @kolyshkin @andrewhsu PTAL

@cpuguy83
Copy link

integration/container/copy_test.go:108:26:warning: cannot use ctx (variable of type context.Context) as *testing.T value in argument to container.Create (gosimple)

@thaJeztah
Copy link
Member Author

ah, booh

For reference on why this is needed:
docker-archive#280 (comment)

Signed-off-by: Tibor Vass <[email protected]>
(cherry picked from commit 8f4b96f)
Signed-off-by: Sebastiaan van Stijn <[email protected]>
@thaJeztah
Copy link
Member Author

cherry-picked 8f4b96f

@thaJeztah
Copy link
Member Author

only failures are DockerSuite.TestRunInteractiveWithRestartPolicy on experimental https://jenkins.dockerproject.org/job/Docker-PRs-experimental/45728/console

tracked through moby#39352

17:30:53 FAIL: docker_cli_run_test.go:1792: DockerSuite.TestRunInteractiveWithRestartPolicy
17:30:53 
17:30:53 assertion failed: 
17:30:53 Command:  /usr/local/cli/docker run -i --name test-inter-restart --restart=always busybox sh
17:30:53 ExitCode: 0
17:30:53 Error:    <nil>
17:30:53 Stdout:   
17:30:53 Stderr:   
17:30:53 
17:30:53 Failures:
17:30:53 ExitCode was 0 expected 11

and on Janky https://jenkins.dockerproject.org/job/Docker-PRs/54598/console

17:44:53 FAIL: docker_cli_start_test.go:190: DockerSuite.TestStartReturnCorrectExitCode
17:44:53 
17:44:53 assertion failed: expected an error, got nil

@andrewhsu
Copy link

@kolyshkin
Copy link

kolyshkin commented Jun 19, 2019

kicked the janky ci as I haven't seen this earlier:

17:44:53 FAIL: docker_cli_start_test.go:190: DockerSuite.TestStartReturnCorrectExitCode

and it looks like another manifestation of that elusive flakiness we have it TestRunInteractiveWithRestartPolicy

@andrewhsu
Copy link

Looks like latest job run has success:

...
PASS: docker_cli_run_test.go:1792: DockerSuite.TestRunInteractiveWithRestartPolicy	11.104s
...
PASS: docker_cli_start_test.go:190: DockerSuite.TestStartReturnCorrectExitCode	2.289s
...

Copy link

@andrewhsu andrewhsu left a comment

Choose a reason for hiding this comment

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

LGTM

@andrewhsu andrewhsu merged commit c513a4c into docker-archive:18.09 Jun 20, 2019
@thaJeztah thaJeztah deleted the 18.09_backport_cp_slash_fix branch June 20, 2019 06:51
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants