Skip to content

devices: correctly check device types#2529

Merged
cyphar merged 1 commit intoopencontainers:masterfrom
cyphar:devices-filemode
Jul 29, 2020
Merged

devices: correctly check device types#2529
cyphar merged 1 commit intoopencontainers:masterfrom
cyphar:devices-filemode

Conversation

@cyphar
Copy link
Member

@cyphar cyphar commented Jul 28, 2020

(mode&S_IFCHR == S_IFCHR) is the wrong way of checking the type of an
inode because the S_IF* bits are actually not a bitmask and instead must
be checked using S_IF*. This bug was neatly hidden behind a (major == 0)
sanity-check but that was removed by me[1].

In addition, add a test that makes sure that HostDevices() doesn't give
rubbish results -- because we broke this and fixed this before[2].

[1] 24388be ("configs: use different types for .Devices and .Resources.Devices")
[2] 3ed492a ("Handle non-devices correctly in DeviceFromPath")

Fixes: b0d014d ("libcontainer: one more switch from syscall to x/sys/unix")
Signed-off-by: Aleksa Sarai cyphar@cyphar.com

@AkihiroSuda
Copy link
Member

Can we have rc92 right after merging this?

@cyphar
Copy link
Member Author

cyphar commented Jul 28, 2020

Sure, especially since this breaks Docker.

@tao12345666333
Copy link

Thanks!
This bug is too hidden. I have been debugging this problem for several days. 😹

(mode&S_IFCHR == S_IFCHR) is the wrong way of checking the type of an
inode because the S_IF* bits are actually not a bitmask and instead must
be checked using S_IF*. This bug was neatly hidden behind a (major == 0)
sanity-check but that was removed by [1].

In addition, add a test that makes sure that HostDevices() doesn't give
rubbish results -- because we broke this and fixed this before[2].

[1]: 24388be ("configs: use different types for .Devices and .Resources.Devices")
[2]: 3ed492a ("Handle non-devices correctly in DeviceFromPath")

Fixes: b0d014d ("libcontainer: one more switch from syscall to x/sys/unix")
Signed-off-by: Aleksa Sarai <cyphar@cyphar.com>
@thaJeztah
Copy link
Member

Thanks @cyphar 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants