This is an archive of the discontinued LLVM Phabricator instance.

[llvm-objcopy] Don't change permissions of non-regular output files
ClosedPublic

Authored by MaskRay on Jul 4 2019, 11:28 PM.

Details

Summary

This fixes the EPERM error when a regular user executes llvm-objcopy a.o /dev/null

A new overload of llvm::sys::fs::setPermissions is added.
Users should provide perm & ~umask as the parameter if they intend to
respect umask.

The existing overload of llvm::sys::fs::setPermissions may be deleted if
we can find an implementation of fchmod() on Windows.

Diff Detail

Event Timeline

MaskRay created this revision.Jul 4 2019, 11:28 PM
MaskRay updated this revision to Diff 208121.Jul 4 2019, 11:36 PM

Fix unittest

MaskRay updated this revision to Diff 208122.Jul 4 2019, 11:51 PM

Add a #ifdef _Win32 to (hopefully) fix Windows tests.
I don't know how to implement fchmod on Windows...

Thanks @MaskRay, this is a big upgrade to what I had.

lib/Support/Windows/Path.inc
773

By no means a Windows expert, but it seems like _get_osfhandle and SetFileInformationByHandle would be a place to start?

unittests/Support/Path.cpp
1541

I'm not sure this test fixture is needed anymore, but it's also not hurting.

MaskRay marked an inline comment as done.Jul 5 2019, 12:03 AM
MaskRay added inline comments.
lib/Support/Windows/Path.inc
773

I don't even have a Windows for testing... I'll leave this to a Windows expert.

This revision is now accepted and ready to land.Jul 8 2019, 10:51 AM

I can't verify the windows parts today. If you can wait until tomorrow I can run it on a windows machine to test it.

I just built this and ran the tests on my Windows machine, and saw no issues. However, I'm not sure I follow why you'd expect there to be any issues with the current state of the code?

I'm happy with the Windows side not being implemented for now, but I'm also not sure why you need the new overload at all? Isn't the OStat.type() check sufficient?

include/llvm/Support/FileSystem.h
670

Doxygen comments to match the other overload?

lib/Support/Windows/Path.inc
773

Perhaps it's worth return a not_supported error here?

test/tools/llvm-objcopy/ELF/mirror-permissions-unix.test
41

I feel like this needs to actually test that the permissions haven't been changed.

but I'm also not sure why you need the new overload at all? Isn't the OStat.type() check sufficient?

I agree with @jhenderson here. If we don't implement a setPermissions(int, perms) for Windows, I reckon we should only use setPermissions(const Twine&, perms).

lib/Support/Windows/Path.inc
773

We could do something hacky like this to get it to at least work on Windows.

HANDLE Handle = _get_osfhandle(FD);
char Buf[256];
GetFinalPathNameByHandleA(Handle, Buf, 256, 0);
return setPermissions(Buf, Permissions);
MaskRay updated this revision to Diff 208867.Jul 9 2019, 7:36 PM
MaskRay marked 2 inline comments as done.
MaskRay edited the summary of this revision. (Show Details)

Address review comments

include/llvm/Support/FileSystem.h
670

The purpose is obvious from the function signature and the overload above. Adding another few lines of comment just clutter the code.

Actually, I think std::error_code setPermissions(const Twine &Path, perms Permissions); should just be removed if we can find an implementation of fchmod on Windows - it is very rare to set mode bits of an unknown file (with a path).

MaskRay marked an inline comment as done.Jul 9 2019, 7:53 PM
jhenderson added inline comments.Jul 10 2019, 3:45 AM
include/llvm/Support/FileSystem.h
670

In the source code, I agree, but if you don't add the doxygen comments, it will have none in the doxygen output, and therefore will look odd and is not so easy to switch around. Also, they have clearly different signatures so the meaning could be considered different.

Also, why would we delete the path overload? What if we don't have easy access to a FD (or don't want to try to look one up)?

lib/Support/Windows/Path.inc
774

make_error_code(errc::not_supported) would be a bit cleaner.

MaskRay updated this revision to Diff 208934.Jul 10 2019, 6:08 AM

Switch to return std::make_error_code(std::errc::not_supported);

MaskRay marked an inline comment as done.Jul 10 2019, 6:16 AM
MaskRay added inline comments.
include/llvm/Support/FileSystem.h
670

In the source code, I agree, but if you don't add the doxygen comments, it will have none in the doxygen output, and therefore will look odd and is not so easy to switch around.

When the function does more involved tasks, I agree. Here, the function doesn't do anything smart. The comment just makes reading through the file harder.

Also, why would we delete the path overload? What if we don't have easy access to a FD (or don't want to try to look one up)?

chmod has much fewer use cases than fchmod. It is rare to chmod a foreign file. Usually, you also write/stat/read the file, then you'll need a handle. Once you have the handle, it is more natural to use fchmod (it also prevents TOCTOU race). setPermissions(Path, perms) was currently only used in one place in objcopy, I can hardly think of any future use case of it.

jhenderson added inline comments.Jul 10 2019, 6:34 AM
include/llvm/Support/FileSystem.h
670

When the function does more involved tasks, I agree. Here, the function doesn't do anything smart. The comment just makes reading through the file harder.

See also md5_contents, is_local, make_absolute, copy_file, and various other methods in this file, and most of them have simple behaviour. Each of them have nearly-identical doxygen comments for multiple overloads. You may disagree with this, but that is the established pattern, and makes a lot more sense when you're reading the doxygen itself. You can probably get away with a single-line comment referring to the "main" overload (i.e. the one with the comments), just noting the different parameter.

MaskRay updated this revision to Diff 208955.Jul 10 2019, 7:09 AM

Add "Vesion of setPermissions accepting a file descriptor."

I just built this and ran the tests on my Windows machine, and saw no issues. However, I'm not sure I follow why you'd expect there to be any issues with the current state of the code?

I'm happy with the Windows side not being implemented for now, but I'm also not sure why you need the new overload at all? Isn't the OStat.type() check sufficient?

Thanks for the update. Can you answer these comments, please?

I just built this and ran the tests on my Windows machine, and saw no issues. However, I'm not sure I follow why you'd expect there to be any issues with the current state of the code?

I'm happy with the Windows side not being implemented for now, but I'm also not sure why you need the new overload at all? Isn't the OStat.type() check sufficient?

Thanks for the update. Can you answer these comments, please?

With the current state of the code, objcopy a.o /dev/null => EPERM because it tries to set the mode bits of /dev/null (/dev/null is owned by root).
If you are root, you can even change its mode bits (usually rw-rw-rw-).

The new overload saves one stat syscall and avoids the race condition (if the filename is moved between sys::fs::openFileForWrite and sys::fs::setPermissions, the updated code will not operate on a nonexistent/different file). I'm sorry that I didn't read carefully when restoreStatOnFile was initially checked in.

jhenderson accepted this revision.Jul 11 2019, 1:31 AM

Thanks, LGTM!

This revision was automatically updated to reflect the committed changes.