Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

pkg/archive:CopyTo(): fix for long dest filename #38634

Merged
merged 1 commit into from
Jan 30, 2019

Conversation

kolyshkin
Copy link
Contributor

@kolyshkin kolyshkin commented Jan 25, 2019

relates to #35739

As reported in docker/for-linux/issues/484, since Docker 18.06
docker cp with a destination file name fails with the following error:

archive/tar: cannot encode header: Format specifies USTAR; and USTAR cannot encode Name="a_very_xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx_long_filename_that_is_101_characters"

The problem is caused by changes in Go 1.10 archive/tar, which
mis-guesses the tar stream format as USTAR (rather than PAX),
which, in turn, leads to inability to specify file names
longer than 100 characters.

This tar stream is sent by TarWithOptions() (which, since we switched to
Go 1.10, explicitly sets format=PAX for every file, see FileInfoHeader(),
and before Go 1.10 it was PAX by default). Unfortunately, the receiving
side, RebaseArchiveEntries(), which calls tar.Next(), mistakenly guesses
header format as USTAR, which leads to the above error.

The fix is easy: set the format to PAX in RebaseArchiveEntries()
where we read the tar stream and change the file name.

A unit test is added to prevent future regressions.

NOTE this code is not used by dockerd, but rather but docker cli
(also possibly other clients), so this needs to be re-vendored
to cli in order to take effect.

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

As reported in docker/for-linux/issues/484, since Docker 18.06
docker cp with a destination file name fails with the following error:

> archive/tar: cannot encode header: Format specifies USTAR; and USTAR cannot encode Name="a_very_xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx_long_filename_that_is_101_characters"

The problem is caused by changes in Go 1.10 archive/tar, which
mis-guesses the tar stream format as USTAR (rather than PAX),
which, in turn, leads to inability to specify file names
longer than 100 characters.

This tar stream is sent by TarWithOptions() (which, since we switched to
Go 1.10, explicitly sets format=PAX for every file, see FileInfoHeader(),
and before Go 1.10 it was PAX by default). Unfortunately, the receiving
side, RebaseArchiveEntries(), which calls tar.Next(), mistakenly guesses
header format as USTAR, which leads to the above error.

The fix is easy: set the format to PAX in RebaseArchiveEntries()
where we read the tar stream and change the file name.

A unit test is added to prevent future regressions.

NOTE this code is not used by dockerd, but rather but docker cli
(also possibly other clients), so this needs to be re-vendored
to cli in order to take effect.

Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
@kolyshkin
Copy link
Contributor Author

kolyshkin commented Jan 25, 2019

janky failure

01:23:29.240 FAIL: docker_api_swarm_test.go:297: DockerSwarmSuite.TestAPISwarmLeaderElection

is well known flaky test, #32673

@kolyshkin
Copy link
Contributor Author

kolyshkin commented Jan 25, 2019

reserved for Derek commands

@alexellis
Copy link
Contributor

Nice to see Derek being put to use 👍

@codecov
Copy link

codecov bot commented Jan 28, 2019

Codecov Report

❗ No coverage uploaded for pull request base (master@27c7178). Click here to learn what that means.
The diff coverage is 100%.

@@            Coverage Diff            @@
##             master   #38634   +/-   ##
=========================================
  Coverage          ?   36.94%           
=========================================
  Files             ?      610           
  Lines             ?    45669           
  Branches          ?        0           
=========================================
  Hits              ?    16871           
  Misses            ?    26475           
  Partials          ?     2323

@cyphar
Copy link
Contributor

cyphar commented Jan 29, 2019

hdr.Format = tar.FormatPAX

This might cause image-cache breakage because mtime and atime were ignored under USTAR but in PAX they can be represented. The Go 1.10 fix was actually because of a bug report I submitted about the above, but as a result you might end up with different images.

@thaJeztah
Copy link
Member

This might cause image-cache breakage because mtime and atime were ignored under USTAR but in PAX they can be represented.

hmf.

I wonder; I recall some older PR's addressing those to be ignored; #12031 (possibly related #11422). So wondering if this will be an issue 🤔

@thaJeztah
Copy link
Member

ping @tonistiigi @dmcgowan PTAL

Copy link
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 (issue is indeed fix with this change)

@thaJeztah
Copy link
Member

Caught up with @tonistiigi to verify if this would affect caching, and we should be good to go, so I'll go ahead and merge.

Feel free to continue the discussion though if there are things we overlooked

@kolyshkin
Copy link
Contributor Author

hdr.Format = tar.FormatPAX

This might cause image-cache breakage because mtime and atime were ignored under USTAR but in PAX they can be represented.

@cyphar as far as I remember, the sending side already takes care of this, see

moby/pkg/archive/archive.go

Lines 365 to 368 in 44af96c

hdr.Format = tar.FormatPAX
hdr.ModTime = hdr.ModTime.Truncate(time.Second)
hdr.AccessTime = time.Time{}
hdr.ChangeTime = time.Time{}

thaJeztah added a commit to thaJeztah/cli that referenced this pull request Mar 27, 2019
relevant changes;

- moby/moby#38006 / docker-archive/engine#114 client: use io.LimitedReader for reading HTTP error
- moby/moby#38634 / docker-archive/engine#167 pkg/archive:CopyTo(): fix for long dest filename
  - fixes docker/for-linux#484 for 18.09
- moby/moby#38944 / docker-archive/engine#183 gitutils: add validation for ref
- moby/moby#37780 / docker-archive/engine#55 pkg/progress: work around closing closed channel panic
  - addresses moby/moby#/37735 pkg/progress: panic due to race on shutdown

Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
docker-jenkins pushed a commit to docker-archive/docker-ce that referenced this pull request Mar 27, 2019
relevant changes;

- moby/moby#38006 / docker-archive/engine#114 client: use io.LimitedReader for reading HTTP error
- moby/moby#38634 / docker-archive/engine#167 pkg/archive:CopyTo(): fix for long dest filename
  - fixes docker/for-linux#484 for 18.09
- moby/moby#38944 / docker-archive/engine#183 gitutils: add validation for ref
- moby/moby#37780 / docker-archive/engine#55 pkg/progress: work around closing closed channel panic
  - addresses moby/moby#/37735 pkg/progress: panic due to race on shutdown

Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
Upstream-commit: 010c234a0d5a03d450ebec60be37dd9f279feeca
Component: cli
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.

7 participants