This is an archive of the discontinued LLVM Phabricator instance.

[Support] Add support for getting file system permissions on Windows and implement sys::fs::set/getPermissions to work with them
ClosedPublic

Authored by jhenderson on Mar 8 2017, 6:52 AM.

Details

Summary

This change adds support for functions to set and get file permissions, in a similar manner to the C++17 permissions() function in <filesystem>. The setter uses chmod on Unix systems and SetFileAttributes on Windows, setting the permissions as passed in. The getter simply uses the existing status() function.

Prior to this change, status() would always return an unknown value for the permissions on a Windows file, making it impossible to test the new function on Windows. I have therefore added support for this as well. On Linux, prior to this change, the permissions included the file type, which should actually be accessed via a different member of the file_status class.

Note that on Windows, only the *_write permission bits have any affect - if any are set, the file is writable, and if not, the file is read-only. This is in common with what MSDN describes for their behaviour of std::filesystem::permissions(), and also what boost::filesystem does.

The motivation behind this change is so that we can easily test behaviour on read-only files in LLVM unit tests, but I am sure that others may find it useful in some situations.

Diff Detail

Repository
rL LLVM

Event Timeline

jhenderson updated this revision to Diff 91009.Mar 8 2017, 6:52 AM
jhenderson created this revision.

Upload correct diff.

labath edited edge metadata.Mar 8 2017, 7:01 AM

Sounds reasonable to me, but I'd wait for others' opinion.

lib/Support/Windows/Path.inc
598 ↗(On Diff #91009)

widenPath already accepts a Twine. No need to convert to a StringRef.

zturner added inline comments.Mar 8 2017, 7:32 AM
include/llvm/Support/FileSystem.h
573 ↗(On Diff #91009)

This is called permissions which makes it sound like a getter, not a setter. If you want this to *set* the file permissions you should make it explicit (i.e. set_permissions). That said, you might as well leave this function around as a getter for the permissions, but it should be trivially implementable in Path.cpp as status followed by return st.perms();.

lib/Support/Windows/Path.inc
532–534 ↗(On Diff #91009)

Is it worth checking whether a file is an exe or batch file before deciding to put all_exe in it? (I don't have a strong opinion, just a thought).

603 ↗(On Diff #91009)

Isn't owner_write and group_write a subset of all_write? No need to specify all 3 of them as the entire expression just reduces to all_write.

amccarth added inline comments.Mar 8 2017, 2:08 PM
lib/Support/Windows/Path.inc
601 ↗(On Diff #91009)

I think you should start with the current file attributes and then add or clear bits as needed. Changing the permissions should not clobber the archive flag, the indexing flag, the hidden flag, or the temporary flag or any other flags that appear in future versions of Windows.

aaron.ballman edited edge metadata.Mar 8 2017, 8:59 PM

My biggest concern with this is that it doesn't really set the file permissions on Windows, it just sets the file *attributes*. It's perfectly cromulent for a file to have the read-only attribute set but still not have read permissions for the current user, or to not have the read-only attribute set and still not be writable for the current user, for instance. Is that something we should be caring about?

lib/Support/Windows/Path.inc
532–534 ↗(On Diff #91009)

I have a slight preference for that, with a FIXME for the more esoteric executable file formats, should anyone ever care about those.

zturner edited edge metadata.Mar 8 2017, 9:14 PM

My biggest concern with this is that it doesn't really set the file permissions on Windows, it just sets the file *attributes*. It's perfectly cromulent for a file to have the read-only attribute set but still not have read permissions for the current user, or to not have the read-only attribute set and still not be writable for the current user, for instance. Is that something we should be caring about?

To be fair, to actually implement this as setting the permissions would be a huge effort and involve messing with ACLs and all kinds of other nonsense that just seems completely unnecessary, especially when the only real motivation is to test the behavior of read-only files (which would probably double as a reasonably effective test for the case where you don't have actual permissions to write the file either).

The author points out that the behavior here is consistent with std::filesystem and boost::filesystem, so it seems reasonable here.

lib/Support/Windows/Path.inc
532–534 ↗(On Diff #91009)

The only reason my preference is slight instead of strong is because you can actually execute anything that has a ShellExecute handler installed. On the other hand, I don't think we need to solve all the world's problems, just LLVM's. And I think we only care about EXEs :)

My biggest concern with this is that it doesn't really set the file permissions on Windows, it just sets the file *attributes*. It's perfectly cromulent for a file to have the read-only attribute set but still not have read permissions for the current user, or to not have the read-only attribute set and still not be writable for the current user, for instance. Is that something we should be caring about?

To be fair, to actually implement this as setting the permissions would be a huge effort and involve messing with ACLs and all kinds of other nonsense that just seems completely unnecessary, especially when the only real motivation is to test the behavior of read-only files (which would probably double as a reasonably effective test for the case where you don't have actual permissions to write the file either).

Yeah, that's why I was asking how much we cared. ;-)

The author points out that the behavior here is consistent with std::filesystem and boost::filesystem, so it seems reasonable here.

Ah, I missed that this was consistent with std::filesystem, which is plenty to convince me.

Just for the sake of clarity, I've only compared against the documented behaviour of std::filesystem on MSDN, rather than the actual behaviour. I've tried using std::experimental::filesystem (I don't have a version of MSVC with std::filesystem yet), and it does exhibit the behaviour described in the documentation, as far as I can observe, i.e. any write bit marks the file as read-only, otherwise the file is marked as writable, and all other permission bits are ignored. I don't know how to check the varying levels of owner/group/others at this point, but I see no reason to disbelieve the documentation.

I'll work on the various other improvements as suggested now and will update the patch.

Thanks all.

include/llvm/Support/FileSystem.h
573 ↗(On Diff #91009)

I did consider doing this (indeed my first implementation called it set_file_permissions), but I went with permissions, to match the std::filesystem function. I will add the getter regardless, as it makes sense.

lib/Support/Windows/Path.inc
532–534 ↗(On Diff #91009)

Doing this for an .exe (or indeed any executable that is actually a program rather than a script without regards to the file extension) is certainly possible using GetBinaryType. I'm not sure if there's any way of doing this for batch files other than inspecting the file extension, but I guess that is what Windows does itself.

As with other behaviour regarding the permissions bits, the argument for not doing this is because the *exe permission bit is completely ignored by Windows, according to the MSDN documentation on std::filesystem. Thus, using status() would yield different permissions to the std version in the future once it is fully supported. I would think that eventually we could replace this function with std::filesytem::status(), so compatibility is probably important. However, if people are not concerned about this, I will go ahead and implement it that way.

598 ↗(On Diff #91009)

Thanks, didn't notice that. I was basing my code purely on what the status() function already does.

601 ↗(On Diff #91009)

Good point. Thanks.

603 ↗(On Diff #91009)

Oops, that was originally meant to be others_write, rather than all_write, but of course, all_write is entirely sufficient here as you point out.

jhenderson updated this revision to Diff 91162.Mar 9 2017, 6:32 AM
jhenderson marked 3 inline comments as done.

Addressed review comments. I have added a getter as suggested by @zturner and have used that in the unit tests. I have also fixed the various minor points others have pointed out. I haven't renamed the setter function at this point, for the reasons addressed in my inline comments, and I also still always return the all_exe permission for reasons outlined as well.

I haven't been able to come up with a good simple test for checking that the file attributes are not accidentally clobbered. The problem is that to test them, I need to call the Windows Get/SetFileAttributes functions, which require wide paths and the widenPath function is in a private header, so isn't something that can be used in the tests. I don't really know how I'd go about moving that code, since it is completely Windows specific. I did test the behaviour manually.

amccarth accepted this revision.Mar 9 2017, 8:39 AM

I'm happy if the other reviewers are.

This revision is now accepted and ready to land.Mar 9 2017, 8:39 AM

Honestly I'm not real happy with the name. It probably seems like a minor point but I'm tempted to just say that STL has it wrong too (it wouldn't be the first time, i.e. vector<bool>). If nobody else feels super strongly I can let it pass, but I would really rather see either get_permissions / set_permissions or permissions / set_permissions in keeping with LLVM traditions.

jhenderson updated this revision to Diff 91300.Mar 10 2017, 2:06 AM
jhenderson edited the summary of this revision. (Show Details)

Renamed function. I'm not strongly opposed to this.

jhenderson marked an inline comment as done.Mar 10 2017, 2:09 AM

I chose to rename the functions to get/setPermissions. I noticed that function names in this file are in two different styles, so I had to pick one, so I went with the wider LLVM style, since we are now deliberately NOT matching the std::filesystem function name.

jhenderson updated this revision to Diff 91302.Mar 10 2017, 2:19 AM

Removed spurious #include from unit tests that had snuck in due to unrelated local changes.

@zturner or @amccarth, would one of you mind committing this, please? I don't have commit access yet. Also @zturner, your LGTM doesn't seem to be showing up here for some reason...!

aaron.ballman closed this revision.Mar 13 2017, 5:29 AM

LGTM as well; I've commit in r297617, thank you!

LGTM as well; I've commit in r297617, thank you!

I had to revert, as this broke at least one of the bots: http://bb.pgr.jp/builders/cmake-llvm-x86_64-linux/builds/49970

LGTM as well; I've commit in r297617, thank you!

I had to revert, as this broke at least one of the bots: http://bb.pgr.jp/builders/cmake-llvm-x86_64-linux/builds/49970

I'm just verifying a fix locally, but it turns out the problem is that on Linux, the file type is encoded in the st_mode field of the stat struct, but is not removed when setting the permissions value. As a result, the getter gets a value with the S_IFREG still set. My plan is to remove this bit (and all other file type bits) when the value is set in getStatus, so that the getter only cares about the permissions as defined in the perms enum. I didn't spot the issue previously because I did not realise that the unit tests were not being built on Linux.

jhenderson updated this revision to Diff 91571.Mar 13 2017, 9:18 AM
jhenderson retitled this revision from [Support] Add support for getting file system permissions on Windows and implement sys::fs::permissions to set them to [Support] Add support for getting file system permissions on Windows and implement sys::fs::set/getPermissions to work with them.
jhenderson edited the summary of this revision. (Show Details)

Fixed Linux unit tests by adding a new member of the perms structure to cover all valid values (except perms_not_known) and using this to mask the Perms member of the file_status struct. This removes the file type from this member, which was probably the original desired behaviour anyway. Users can get the file type from the Type member instead.

jhenderson reopened this revision.Mar 13 2017, 9:18 AM
This revision is now accepted and ready to land.Mar 13 2017, 9:18 AM

Can someone more familiar with UNIX permissions review the new changes before we land them?

jhenderson requested review of this revision.Mar 15 2017, 2:14 AM
jhenderson edited edge metadata.
zturner accepted this revision.Mar 15 2017, 10:33 AM

The changes look ok to me. james, did you confirm the tests pass on Linux?

This revision is now accepted and ready to land.Mar 15 2017, 10:33 AM

The changes look ok to me. james, did you confirm the tests pass on Linux?

I did once, but I'll double check. I have commit access now, so can commit this myself once I've done that. Thanks for the help!

This revision was automatically updated to reflect the committed changes.