Skip to content

Graphdriver: fix "device" mode not being detected if "character-device" bit is set#38758

Merged
thaJeztah merged 1 commit intomoby:masterfrom
thaJeztah:add_missing_char_device_mode
Feb 20, 2019
Merged

Graphdriver: fix "device" mode not being detected if "character-device" bit is set#38758
thaJeztah merged 1 commit intomoby:masterfrom
thaJeztah:add_missing_char_device_mode

Conversation

@thaJeztah
Copy link
Copy Markdown
Member

@thaJeztah thaJeztah commented Feb 19, 2019 •
edited
Loading

Due to a bug in Golang (github.com/golang#27640), the "character device" bit was omitted when checking file-modes with os.ModeType.

This bug was resolved in Go 1.12 through golang/go@a2a3dd0, but as a result, graphdrivers would no longer recognize "device" files, causing pulling of images that have a file with this filemode to fail;

failed to register layer:
unknown file type for /var/lib/docker/vfs/dir/.../dev/console

The current code checked for an exact match of Modes to be set. The os.ModeCharDevice and os.ModeDevice bits will always be set in tandem, however, because the code was only looking for an exact match, this detection broke now that os.ModeCharDevice was added.

This patch changes the code to be more defensive, and instead check if the os.ModeDevice bit is set (either with, or without the os.ModeCharDevice bit).

In addition, some information was added to the error-message if no type was matched, to assist debugging in case additional types are added in future.

@thaJeztah
Copy link
Copy Markdown
Member Author

thaJeztah commented Feb 19, 2019

@kolyshkin @tonistiigi @dmcgowan PTAL

Also wondering if this should be changed to os.ModeCharDevice?

moby/pkg/term/term_linux_test.go

Lines 23 to 26 in 8e610b2

func newTtyForTest(t *testing.T) (*os.File, error) {
RequiresRoot(t)
return os.OpenFile("/dev/tty", os.O_RDWR, os.ModeDevice)
}

@thaJeztah thaJeztah mentioned this pull request Feb 19, 2019
Merged
@thaJeztah thaJeztah mentioned this pull request Feb 19, 2019
Open
@thaJeztah
Copy link
Copy Markdown
Member Author

thaJeztah commented Feb 19, 2019

also opened containerd/continuity#139 for continuity

@kolyshkin @cpuguy83 @tonistiigi

@thaJeztah
Copy link
Copy Markdown
Member Author

thaJeztah commented Feb 19, 2019 •
edited
Loading

hm... thought I checked, but looks like this does not fix the issue on Go 1.12;

1db09adb5ddd: Pull complete 
failed to register layer: unknown file type (69206418 / 69206016) for /var/lib/docker/vfs/dir/a04475818f8aac181919e10d4fc3f9a944dab33fa052a9ed2af74db7a7e5acf3/dev/console

failed to register layer: unknown file type (69206418 / 69206016 / Dcrw--w--w-) for /var/lib/docker/vfs/dir/fb91079168d45edb42c97b496dd86a51f3242e11decb8cfb2a38a6a8c5dfa2c1/dev/console

@tonistiigi
Copy link
Copy Markdown
Member

tonistiigi commented Feb 19, 2019

@thaJeztah 69206016 is os.ModeCharDevice|os.ModeDevice

@codecov
Copy link
Copy Markdown

codecov bot commented Feb 19, 2019 •
edited
Loading

Codecov Report

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

@@            Coverage Diff            @@
##             master   #38758   +/-   ##
=========================================
  Coverage          ?   36.54%           
=========================================
  Files             ?      610           
  Lines             ?    45394           
  Branches          ?        0           
=========================================
  Hits              ?    16589           
  Misses            ?    26511           
  Partials          ?     2294

@thaJeztah
Copy link
Copy Markdown
Member Author

thaJeztah commented Feb 19, 2019

Thanks @tonistiigi - I started to suspect that after adding the .String() presentation in my error message (and it showing Dc). Do you know if os.ModeCharDevice and os.ModeDevice will always be both set, or is it possible that os.ModeCharDevice only is set?

@tonistiigi
Copy link
Copy Markdown
Member

tonistiigi commented Feb 19, 2019

@thaJeztah It looks like that https://github.com/golang/go/blob/master/src/archive/tar/common.go#L578-L589

@thaJeztah thaJeztah force-pushed the add_missing_char_device_mode branch from 4939a90 to 2b43380 Compare February 19, 2019 20:51
@thaJeztah thaJeztah changed the title Graphdriver: fix CharDevice not being detected Graphdriver: fix "device" mode not being detected if "character-device" bit is set Feb 19, 2019
@thaJeztah thaJeztah force-pushed the add_missing_char_device_mode branch from 2b43380 to 85469f1 Compare February 19, 2019 20:55
@thaJeztah
Copy link
Copy Markdown
Member Author

thaJeztah commented Feb 19, 2019

@tonistiigi thanks! Updated the PR, and took a slightly different approach that (I think) is a bit more defensive; let me know if this looks good to you (otherwise, I'll happily change it back to the old version)

kolyshkin
kolyshkin reviewed Feb 19, 2019
View reviewed changes
@thaJeztah
Graphdriver: fix "device" mode not being detected if "character-devic…
c7a38c2 
…e" bit is set

Due to a bug in Golang (github.com/golang#27640), the "character device"
bit was omitted when checking file-modes with `os.ModeType`.

This bug was resolved in Go 1.12, but as a result, graphdrivers
would no longer recognize "device" files, causing pulling of
images that have a file with this filemode to fail;

    failed to register layer:
    unknown file type for /var/lib/docker/vfs/dir/.../dev/console

The current code checked for an exact match of Modes to be set. The
`os.ModeCharDevice` and `os.ModeDevice` bits will always be set in
tandem, however, because the code was only looking for an exact
match, this detection broke now that `os.ModeCharDevice` was added.

This patch changes the code to be more defensive, and instead
check if the `os.ModeDevice` bit is set (either with, or without
the `os.ModeCharDevice` bit).

In addition, some information was added to the error-message if
no type was matched, to assist debugging in case additional types
are added in future.

Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
@thaJeztah thaJeztah force-pushed the add_missing_char_device_mode branch from 85469f1 to c7a38c2 Compare February 20, 2019 10:09
@thaJeztah thaJeztah mentioned this pull request Feb 20, 2019
Merged
@kolyshkin
Copy link
Copy Markdown
Contributor

kolyshkin commented Feb 20, 2019

LGTM

@thaJeztah thaJeztah merged commit 9688f12 into moby:master Feb 20, 2019
@thaJeztah thaJeztah deleted the add_missing_char_device_mode branch February 20, 2019 22:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@kolyshkin kolyshkin kolyshkin left review comments

@tonistiigi tonistiigi tonistiigi approved these changes

@AkihiroSuda AkihiroSuda AkihiroSuda approved these changes

Assignees

No one assigned

Labels

area/storage Image Storage process/cherry-picked status/2-code-review

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@thaJeztah @tonistiigi @kolyshkin @AkihiroSuda @GordonTheTurtle