bugfix: allow hardlink to softlink file by fuweid · Pull Request #2458 · containerd/containerd

fuweid · 2018-07-12T06:45:38Z

With fs.RootPath, the target file will be the file which the softlink points to, like:

  touch /tmp/zzz
  ln -s /tmp/zzz /tmp/xxx
  ln /tmp/xxx /tmp/yyy

The /tmp/yyy should be same with the /tmp/xxx, not /tmp/zzz.

Signed-off-by: Wei Fu [email protected]

fuweid · 2018-07-12T06:47:12Z

There is the Dockerfile for the case:

FROM busybox:latest

RUN touch /tmp/zzz \
 && ln -s /tmp/zzz /tmp/xxx \
 && ln /tmp/xxx /tmp/yyy

And we will get message like that:

➜  image ctr image pull localhost:5000/test:1
localhost:5000/test:1:                                                            resolved       |++++++++++++++++++++++++++++++++++++++|
manifest-sha256:ef7956cb7dd23df318fb83a4d0e8f54ea165048eca66db1387f882d4b7880e93: done           |++++++++++++++++++++++++++++++++++++++|
layer-sha256:970eabf5aa7df00f9b6fffb5861facd625f2851f4acdcb09768648249591687f:    done           |++++++++++++++++++++++++++++++++++++++|
config-sha256:358a921423bc8db85f6f4b32cc6d6e3d33700eb58fe73084bda6dacb6ac92a49:   done           |++++++++++++++++++++++++++++++++++++++|
layer-sha256:07a152489297fc2bca20be96fab3527ceac5668328a30fd543a160cd689ee548:    done           |++++++++++++++++++++++++++++++++++++++|
elapsed: 0.1 s                                                                    total:   0.0 B (0.0 B/s)
unpacking sha256:ef7956cb7dd23df318fb83a4d0e8f54ea165048eca66db1387f882d4b7880e93...
INFO[0000] Apply failure, attempting cleanup             error="rpc error: code = NotFound desc = link /tmp/extract-661700870/tmp/zzz /tmp/extract-661700870/tmp/yyy: no such file or directory" key="extract-514942130-FIi0 sha256:ebac1d644fd76b7d13b5ab99bec73f63be2eb043567556a85625fa98f1fbec3b"
ctr: failed to extract layer sha256:d9acc7ab6c3578d3537f6bae0db2e667eaf38e8b4f612b661135f6ca261dd364: rpc error: code = NotFound desc = link /tmp/extract-661700870/tmp/zzz /tmp/extract-661700870/tmp/yyy: no such file or directory

AkihiroSuda · 2018-07-12T07:26:16Z

Could you add UT?

fuweid · 2018-07-12T07:42:14Z

Could you add UT?

Sure. Will do @AkihiroSuda

fuweid · 2018-07-16T14:51:57Z

Hi @AkihiroSuda and @dmcgowan

I update the code and the test case. Please take a look, thanks.

BTW, the CI fails cause of make check-protos check-api-descriptors;. Is it about the goproto tools?

AkihiroSuda · 2018-07-16T15:06:26Z

Why do we need "/" here?

We don't need separator here. I have removed it. Thanks!

fuweid · 2018-07-17T03:02:41Z

ping @AkihiroSuda . Does the change make senses?

codecov-io · 2018-07-17T16:12:32Z

Codecov Report

Merging #2458 into master will increase coverage by <.01%.
The diff coverage is 50%.

@@            Coverage Diff             @@
##           master    #2458      +/-   ##
==========================================
+ Coverage   44.73%   44.73%   +<.01%     
==========================================
  Files          93       93              
  Lines        9490     9501      +11     
==========================================
+ Hits         4245     4250       +5     
- Misses       4562     4566       +4     
- Partials      683      685       +2

Flag	Coverage Δ
#linux	`48.94% <60%> (ø)`	⬆️
#windows	`40.98% <0%> (-0.06%)`	⬇️

Impacted Files	Coverage Δ
archive/tar.go	`43.11% <50%> (+0.05%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 94cfce6...3b1534c. Read the comment docs.

dmcgowan · 2018-07-17T21:05:50Z

The result of this should still be checked before returning since base could be ".." and this would return the parent directory. The link operation should fail but it should not get all the way to making a syscall with the parent directory as an arg.

Make senses. Have updated. Please take a look. @dmcgowan

cc @AkihiroSuda

dmcgowan · 2018-07-18T22:26:39Z

I don't think the Lstat is necessary here. When we do get a syscall error we want to error, but invalid input here should probably just end up returning a valid path, for example if you tried to join / and .. then instead of erroring, you would just return /.

Maybe try just doing this with another split

targetPath := filepath.Join(ppath, base) if filepath.Base(targetPath) != ppath { targetPath = ppath } return targetPath, nil

In this case any further attempt to alter the directory location with the base name will just end up with ppath which we know is a safe path even though it might result in a Link error.

Actually, I made it complicated by the Lstat.

As you mentioned, don't allow the base to change the ppath so that we keep the scope. But it's quite strict. The base can be out of the ppath but in the root, which make the error information accuracy. In this case, we can check targetPath is out of root or not. If does, we just return the root.

How about this solution? 😄

With `fs.RootPath`, the target file will be the file which the softlink points to, like: touch /tmp/zzz ln -s /tmp/zzz /tmp/xxx ln /tmp/xxx /tmp/yyy The `/tmp/yyy` should be same with the `/tmp/xxx`, not `/tmp/zzz`. We should allow hardlink to softlink file. Signed-off-by: Wei Fu <[email protected]>

dmcgowan · 2018-07-19T18:35:47Z

LGTM

fuweid force-pushed the bugfix_apply_hardlink_to_softlink branch from 12d7ec1 to 3e689c9 Compare July 12, 2018 06:47

fuweid changed the title ~~bugfix: allow hardlink to softlink file~~ [WIP] bugfix: allow hardlink to softlink file Jul 13, 2018

fuweid force-pushed the bugfix_apply_hardlink_to_softlink branch from 3e689c9 to 9c814e7 Compare July 16, 2018 14:19

fuweid changed the title ~~[WIP] bugfix: allow hardlink to softlink file~~ bugfix: allow hardlink to softlink file Jul 16, 2018

fuweid force-pushed the bugfix_apply_hardlink_to_softlink branch from 9c814e7 to 3bd32ea Compare July 16, 2018 14:38

fuweid force-pushed the bugfix_apply_hardlink_to_softlink branch from 3bd32ea to 86d382a Compare July 16, 2018 15:03

AkihiroSuda reviewed Jul 16, 2018

View reviewed changes

fuweid force-pushed the bugfix_apply_hardlink_to_softlink branch 3 times, most recently from 1a58a48 to efae3d8 Compare July 17, 2018 02:50

fuweid force-pushed the bugfix_apply_hardlink_to_softlink branch from efae3d8 to dc12223 Compare July 17, 2018 15:06

crosbymichael requested review from AkihiroSuda and dmcgowan July 17, 2018 18:23

dmcgowan reviewed Jul 17, 2018

View reviewed changes

fuweid force-pushed the bugfix_apply_hardlink_to_softlink branch from dc12223 to 52b74d1 Compare July 18, 2018 07:21

AkihiroSuda approved these changes Jul 18, 2018

View reviewed changes

dmcgowan reviewed Jul 18, 2018

View reviewed changes

fuweid force-pushed the bugfix_apply_hardlink_to_softlink branch from 52b74d1 to 3b1534c Compare July 19, 2018 07:14

crosbymichael merged commit 49fb363 into containerd:master Jul 19, 2018

thaJeztah mentioned this pull request Jan 23, 2019

[release/1.2 backport] fix: SCHILY.xattrs should be SCHILY.xattr #2953

Merged

fuweid deleted the bugfix_apply_hardlink_to_softlink branch February 20, 2019 06:33

Conversation

fuweid commented Jul 12, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

fuweid commented Jul 12, 2018

Uh oh!

AkihiroSuda commented Jul 12, 2018

Uh oh!

fuweid commented Jul 12, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

fuweid commented Jul 16, 2018

Uh oh!

AkihiroSuda Jul 16, 2018

Choose a reason for hiding this comment

Uh oh!

fuweid Jul 16, 2018

Choose a reason for hiding this comment

Uh oh!

fuweid commented Jul 17, 2018

Uh oh!

codecov-io commented Jul 17, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

dmcgowan Jul 17, 2018

Choose a reason for hiding this comment

Uh oh!

fuweid Jul 18, 2018

Choose a reason for hiding this comment

Uh oh!

dmcgowan Jul 18, 2018

Choose a reason for hiding this comment

Uh oh!

fuweid Jul 19, 2018

Choose a reason for hiding this comment

Uh oh!

dmcgowan commented Jul 19, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

fuweid commented Jul 12, 2018 •

edited

Loading

fuweid commented Jul 12, 2018 •

edited

Loading

codecov-io commented Jul 17, 2018 •

edited

Loading