Skip to content

Conversation

@rhatdan
Copy link
Member

@rhatdan rhatdan commented Mar 19, 2019

When trying to vendor into containers/image we found issues
with cross platform compilation.

Signed-off-by: Daniel J Walsh dwalsh@redhat.com

@rhatdan
Copy link
Member Author

rhatdan commented Mar 19, 2019

utils.go Outdated
st, err := os.Stat(tmpDir)
if err == nil && int(st.Sys().(*syscall.Stat_t).Uid) == os.Getuid() && st.Mode().Perm() == 0700 {
st, err := system.Stat(tmpDir)
if err == nil && int(st.UID()) == os.Getuid() && st.Mode() == 0700 {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should compile, which is good, but st.UID() is hardwired to always return 0, while os.Getuid() is documented to always return -1, so our attempt to create this directory ends up either failing and producing an error, or succeeding and then being ignored.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My preference is to avoid creating a directory that we're just going to ignore.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why would stating of the directory return UID==0?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

pkg/system's Stat() returns a pointer to a StatT, which on Windows systems only ever returns 0 from its UID() method.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you want to make this OS Specific then?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we end up using this code only when os.Geteuid() != 0, on Windows it returns always -1. Could we just check os.Geteuid() value and treat -1 differently?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I removed the checks altogether, if the directory gets created with from podman, then we need to believe it got created with the correct ownership and permissions.

@vrothberg
Copy link
Member

Do you know if containers/storage has consumers on Windows? If it's for historical reasons, we may have the chance to ditch the windows builds.

When trying to vendor into containers/image we found issues
with cross platform compilation.

Signed-off-by: Daniel J Walsh <dwalsh@redhat.com>
Copy link
Member Author

@rhatdan rhatdan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No consumers that we know of, but we wanted to make sure it compiles on other platforms.

utils.go Outdated
st, err := os.Stat(tmpDir)
if err == nil && int(st.Sys().(*syscall.Stat_t).Uid) == os.Getuid() && st.Mode().Perm() == 0700 {
st, err := system.Stat(tmpDir)
if err == nil && int(st.UID()) == os.Getuid() && st.Mode() == 0700 {
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I removed the checks altogether, if the directory gets created with from podman, then we need to believe it got created with the correct ownership and permissions.

@nalind
Copy link
Member

nalind commented Mar 20, 2019

I don't expect $XDG_RUNTIME_DIR to be set on Windows, and at any rate we don't create a directory right here if it is set, so LGTM.

@rhatdan rhatdan merged commit f17e56f into containers:master Mar 20, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants