oci-image-tool: Cleanup partially-unpacked directories on failures #250

coolljt0725 · 2016-09-02T09:25:59Z

Signed-off-by: Lei Jitang [email protected]

runcom · 2016-09-02T09:28:56Z

does it make sense to unpack to a temp dir and then os.Rename? to have something more atomic somehow (on error just throw off the temp)

jonboulle · 2016-09-02T14:18:50Z

aiyeee, I really don't like this code. There is (probably?) a breakout if this is hit (removing a path outside the tarball) - and if I'm wrong about that one I'd bet there are other corner cases not covered yet.

Can we use some existing, well-tested tar extracting code instead?

runcom · 2016-09-02T14:23:14Z

This is already coming from Docker code which is imo well tested. Not sure about other options.

wking · 2016-09-02T16:17:29Z

On Fri, Sep 02, 2016 at 07:18:51AM -0700, Jonathan Boulle wrote:

There is (probably?) a breakout if
this
is hit (removing a path outside the tarball) - and if I'm wrong
about that one I'd bet there are other corner cases not covered yet.

Yeah, that looks like a bug to me. You could fix it by moving:

entries[path] = true

after the check.

But I think this per-layer unwinding approach is trying too hard. And
there will be some things (whiteout removal, file clobbers) that you
can't unwind easily. I'd rather not try to cleanup inside
unpackLayer and instead address #242 by (conditionally) catching
unpack errors and using:

os.RemoveAll(dest)

in unpack if any of the unpackLayer calls failed.

coolljt0725 · 2016-09-02T16:43:42Z

@jonboulle

aiyeee, I really don't like this code. There is (probably?) a breakout if this is hit (removing a path outside the tarball) - and if I'm wrong about that one I'd bet there are other corner cases not covered yet.

it's does a bug here, thank you for pointing out:)

@wking I has thought about catching unpack errors and using: os.RemoveAll(dest), but I think if there are already some files in dest before unpacking, it would make mistakes. So I just want to delete what had been unpacked.
But this approach of this pr only remove the current layer if encounter error, still need to some work to remove already unpacked layer.

@runcom Do you mean we unpack the layer to a tmp file and then rename to dest if no error encounter? In this way, I think it would make mistake if there are already files indest

Thank you all for reviewing, I'll work on this tomorrow :)
`

wking · 2016-09-02T17:11:47Z

On Fri, Sep 02, 2016 at 09:43:44AM -0700, Lei Jitang wrote:

@wking I has thought about catching unpack errors and using: os.RemoveAll(dest), but I think if there are already some files in
dest before unpacking, it would make mistakes.

I think it should be an error if there is anything in ‘dest’ before
you start unpacking.

coolljt0725 · 2016-09-05T10:50:45Z

@wking that's make sense.
updated

wking · 2016-09-06T17:25:08Z

image/manifest.go

+func (m *manifest) unpack(w walker, dest string) (retErr error) {
+	// error out if the dest directory is not empty
+	if _, err := os.Stat(dest); err == nil {
+		s, _ := ioutil.ReadDir(dest)


You don't need both Stat and ReadDir. If you want to error out if there is an entry at that path, just use Stat. If you want to error out unless the path has no entry or contains an empty directory, just use ReadDir. The latter sounds better to me (you can use it with mktemp -d), but I'm ok with either.

And if you end up using ReadDir, you should be checking for errors that it returns and returning those, instead of returning your own “is not empty” error in those cases (e.g. maybe the directory is not empty, but you lack read permission).

@wking Thanks, will update

coolljt0725 · 2016-09-07T02:44:30Z

@wking updated

wking · 2016-09-07T07:58:17Z

image/manifest.go

+	// error out if the dest directory is not empty
+	if s, err := ioutil.ReadDir(dest); err != nil && !os.IsNotExist(err) || len(s) > 0 {
+		if err != nil {
+			return fmt.Errorf("failed to open %s: %v", dest, err)


Use errors.Wrap instead of fmt.Errorf? There is a lot of existing use of errors.Wrap in this package.

coolljt0725 · 2016-09-07T08:35:22Z

@wking updated, thanks :)

wking · 2016-09-07T09:54:22Z

7684889 looks good to me.

jonboulle · 2016-09-07T11:53:42Z

Looks good but can you add a test for this please?

coolljt0725 · 2016-09-07T13:47:24Z

@jbouzane test added, but the test block by #257
with #257, make test paas
comment the codes added in image/manifest.go and make test again test will failed with

FAIL: TestUnpackLayerRemovePartialyUnpackedFile (0.01s)
        manifest_test.go:113: Execpt partialy unpacked file has been removed

Signed-off-by: Lei Jitang <[email protected]>

wking · 2016-09-14T06:03:56Z

On Wed, Sep 07, 2016 at 06:47:25AM -0700, Lei Jitang wrote:

test added, but the test block by #257

Rebase now that #257 has landed?

coolljt0725 · 2016-09-14T07:32:40Z

@wking Should we continue this after splitting tool to a separate repo?

wking · 2016-09-14T08:10:24Z

On Wed, Sep 14, 2016 at 12:32:40AM -0700, Lei Jitang wrote:

@wking Should we continue this after splitting tool to a separate
repo?

If you like. I'm a maintainer of neither, but I see no reason to
pause development while we wait for the shift.

coolljt0725 force-pushed the remove_failure branch 2 times, most recently from c94c189 to dbd6607 Compare September 5, 2016 10:50

wking reviewed Sep 6, 2016
View reviewed changes

coolljt0725 force-pushed the remove_failure branch 2 times, most recently from 6451e5c to aa6be98 Compare September 7, 2016 02:30

wking reviewed Sep 7, 2016
View reviewed changes

coolljt0725 force-pushed the remove_failure branch from aa6be98 to 7684889 Compare September 7, 2016 08:28

jonboulle added the reviewed/needs tests label Sep 7, 2016

coolljt0725 force-pushed the remove_failure branch from 7684889 to b0200b1 Compare September 7, 2016 13:43

coolljt0725 force-pushed the remove_failure branch 2 times, most recently from 0d0414f to 781b8ff Compare September 8, 2016 03:26

coolljt0725 added 2 commits September 7, 2016 23:37

oci-image-tool: Cleanup partially-unpacked directories on failures

5089126

Signed-off-by: Lei Jitang <[email protected]>

Add license header

781b8ff

Signed-off-by: Lei Jitang <[email protected]>

coolljt0725 mentioned this pull request Sep 18, 2016

Cleanup partially-unpacked directories on failures opencontainers/image-tools#7

Merged

coolljt0725 closed this Sep 18, 2016

oci-image-tool: Cleanup partially-unpacked directories on failures #250

oci-image-tool: Cleanup partially-unpacked directories on failures #250

Uh oh!

Conversation

coolljt0725 commented Sep 2, 2016

Uh oh!

runcom commented Sep 2, 2016

Uh oh!

jonboulle commented Sep 2, 2016

Uh oh!

runcom commented Sep 2, 2016

Uh oh!

wking commented Sep 2, 2016

Uh oh!

coolljt0725 commented Sep 2, 2016

Uh oh!

wking commented Sep 2, 2016

Uh oh!

coolljt0725 commented Sep 5, 2016

Uh oh!

wking Sep 6, 2016

Choose a reason for hiding this comment

Uh oh!

wking Sep 6, 2016

Choose a reason for hiding this comment

Uh oh!

coolljt0725 Sep 7, 2016

Choose a reason for hiding this comment

Uh oh!

coolljt0725 commented Sep 7, 2016

Uh oh!

wking Sep 7, 2016

Choose a reason for hiding this comment

Uh oh!

coolljt0725 commented Sep 7, 2016

Uh oh!

wking commented Sep 7, 2016 via email

Uh oh!

jonboulle commented Sep 7, 2016

Uh oh!

coolljt0725 commented Sep 7, 2016

Uh oh!

wking commented Sep 14, 2016

Uh oh!

coolljt0725 commented Sep 14, 2016

Uh oh!

wking commented Sep 14, 2016

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants