[cfe-dev] [llvm-dev] clang-format sets executable permission on windows (openNativeFile ignores mode on Windows)

[Adrian McCarthy via cfe-dev]

I agree that updating an existing file (which is what clang-format mostly
does) should preserve as much of the metadata of the file as possible. If
the original file has a non-default DACL, the new file should have the same
DACL.  It might be worth investigating whether we should use Windows's
ReplaceFileW API.

https://docs.microsoft.com/en-us/windows/win32/api/winbase/nf-winbase-replacefilew


This would have other nice benefits, like preserving the original file
creation time, the file owner, etc.

On Mon, Apr 20, 2020 at 12:27 PM Chris Tetreault <ctetreau at quicinc.com>
wrote:

> Yes, this is my main concern. It seems to be idiomatic in win32
> programming to just accept the default permissions. While googling this
> issue, one of the top posts is a Raymond Chen MSDN blog that basically says
> as much. If the clang driver wants to produce a .o file with executable
> permissions, that’s probably not a big deal. Furthermore, I agree that it
> might not be a great idea to do something else unless it’s an actual good
> solution. That said:
>
>
>
>    1. llvm has a create file function that claims to set permissions on
>    the file using a unix-style octal triple. If this function is not actually
>    portable on Windows, this should be documented
>    2. clang-format -i should not modify the file permissions. If this
>    means that it cannot use llvm::sys::fs::openFileForReadWrite, then so be
>    it. Perhaps some sort of duplicateFile(FileHandle * f, …) or
>    createBasedOn(FileHandle *f, …) function can be added that creates a file
>    with the exact permissions of some other file? It should be easier to make
>    this portable. I’m pretty sure win32 offers a way to query the
>    SECURITY_ATTRIBUTES of a file.
>
>
>
> Thanks,
>
>    Christopher Tetreault
>
>
>
> *From:* Robinson, Paul <paul.robinson at sony.com>
> *Sent:* Monday, April 20, 2020 11:47 AM
> *To:* Adrian McCarthy <amccarth at google.com>
> *Cc:* Chris Tetreault <ctetreau at quicinc.com>; LLVM Dev <
> llvm-dev at lists.llvm.org>
> *Subject:* [EXT] RE: [llvm-dev] clang-format sets executable permission
> on windows (openNativeFile ignores mode on Windows)
>
>
>
> I think the original issue tells us what the correct behavior is:
> clang-format should write the new file using the exact same permissions as
> the old file.
>
> Mappings used/reported by various tools imitating other environments is
> not really the issue.
>
> --paulr
>
>
>
> *From:* Adrian McCarthy <amccarth at google.com>
> *Sent:* Monday, April 20, 2020 1:48 PM
> *To:* Robinson, Paul <paul.robinson at sony.com>
> *Cc:* Chris Tetreault <ctetreau at quicinc.com>; LLVM Dev <
> llvm-dev at lists.llvm.org>
> *Subject:* Re: [llvm-dev] clang-format sets executable permission on
> windows (openNativeFile ignores mode on Windows)
>
>
>
> Mapping between Windows DACLs and Posix user-group-other file permissions
> is complex, depends on externalities, and is necessarily lossy:
>
>
>
> http://www.cygwin.com/cygwin-ug-net/using-filemodes.html
> <https://urldefense.com/v3/__http:/www.cygwin.com/cygwin-ug-net/using-filemodes.html__;!!JmoZiZGBv3RvKRSx!trNI9pSd6Lx_2kTeydRx_JBKT7WHIoogsKZbyEey-u4d8kr9jxcV2oZI8ez9B19Q2Q$>
>
>
> http://www.cygwin.com/cygwin-ug-net/ntsec.html
> <https://urldefense.com/v3/__http:/www.cygwin.com/cygwin-ug-net/ntsec.html__;!!JmoZiZGBv3RvKRSx!trNI9pSd6Lx_2kTeydRx_JBKT7WHIoogsKZbyEey-u4d8kr9jxcV2oZI8eyvqe7jeA$>
>
>
>
>
> While there's a lot of information at those links, they don't completely
> explain how the mapping works.  (And who knows if GnuWin32 does the mapping
> the same way?)
>
>
>
> If we're going to propose have LLVM tools set non-default DACLs when
> writing files on NTFS such that Cygwin doesn't show `x`, then I think we'll
> need to know exactly what those DACLs will look like.
>
>
>
> We'll also have to consider whether there are any drawbacks to having LLVM
> produce files with different permissions every other development tools
> (like editors, IDEs, reporting tools, etc.).
>
>
>
> On Mon, Apr 20, 2020 at 10:11 AM Robinson, Paul <paul.robinson at sony.com>
> wrote:
>
> I agree, spurious Execute permission is a problem, and as an aside,
> Execute-only is a real and useful thing.
>
> Cygwin probably keeps a `umask` notion around somewhere.
>
> If I use GnuWin32’s `ls` to look at files, they (mostly) don’t have
> Execute on my system, so I think it’s not git itself setting x permission.
>
> --paulr
>
>
>
> *From:* cfe-dev <cfe-dev-bounces at lists.llvm.org> *On Behalf Of *Chris
> Tetreault via cfe-dev
> *Sent:* Monday, April 20, 2020 12:32 PM
> *To:* Adrian McCarthy <amccarth at google.com>
> *Cc:* LLVM Dev <llvm-dev at lists.llvm.org>; cfe-dev at lists.llvm.org
> *Subject:* Re: [cfe-dev] [llvm-dev] clang-format sets executable
> permission on windows (openNativeFile ignores mode on Windows)
>
>
>
> I’m using Cygwin to interact with the source tree. My sources (I’m working
> on LLVM itself) were created by other developers (and by extension, created
> by git.exe on my machine), but a quick expirement of trying “touch foo”
> shows that Cygwin creates files with 644 mode. Finding the file I created
> in explorer and checking the properties shows it has the following NTFS
> permissions:
>
>
>
> My user: Read, Write
>
> Domain users: Read
>
> Everyone: Read
>
>
>
> I suppose “whatever Cygwin does” might serve as a good model of how me
> might meaningfully translate unix-style permissions to windows. I’ve
> created a bug for this (45619), I can update it with this suggestion.
>
>
>
> As for the harmlessness of a source file being committed with executable
> permissions, I think it’s a security risk. An attacker can know that a
> program creates files with executable permissions. They can presumably
> trick llvm into producing a script that they can run on a target machine.
> Even if you disagree that this is a big deal, I’ve worked on projects
> previously where you would get dinged in code review for setting executable
> permissions on source files. It would be hugely annoying to have to
> remember to fixup the permissions every time you invoke clang-format.
>
>
>
> Thanks,
>
>    Christopher Tetreault
>
>
>
> *From:* Adrian McCarthy <amccarth at google.com>
> *Sent:* Monday, April 20, 2020 8:36 AM
> *To:* Chris Tetreault <ctetreau at quicinc.com>
> *Cc:* LLVM Dev <llvm-dev at lists.llvm.org>; cfe-dev at lists.llvm.org
> *Subject:* [EXT] Re: [llvm-dev] clang-format sets executable permission
> on windows (openNativeFile ignores mode on Windows)
>
>
>
> I don't claim to understand NTFS permissions fully, but this mostly sounds
> like a problem of how the NTFS permissions are presented as a Unix-style
> mode.
>
>
>
> When you create a new file without specifying explicit permissions (as
> most tools do), Windows (or NTFS) grants the new file the same permissions
> as the folder that contains it.  Folders generally have the "Read &
> Execute" permission, since that's what lets a user navigate the
> filesystem.  Whatever tool you're using to translate Windows/NTFS
> permissions into a Unix-style mode is probably showing x when the file has
> R&E.
>
>
>
> I'm curious how your source files were originally created without R&E.
>
>
>
> On something like a text file or a source file, R&E is common and harmless.
>
>
>
> Common:  If I create a text file from Explorer or Notepad or Visual
> Studio, that file gets R&E.  I have source files that have never been
> touched by clang-format, and they have the same permissions, including R&E,
> as the ones that have.
>
>
>
> Harmless:  Since text files aren't executable, having R&E doesn't grant
> anything beyond Read.  A possible exception might be batch files (.BAT).
> Does the command interpreter check R&E for those?  I don't know offhand.
> If it does, would you want to force the user to change permissions of a
> .BAT file they had just written in a text editor before they try it?
>
>
>
> That said, I don't entirely understand the permission model has both
> "Read" and "Read & Execute".  I'd guess that it's because having orthogonal
> "Read" and "Execute" permissions would allow a nonsense state that marks a
> file as executable but not readable.
>
>
>
> The flags like FILE_GENERIC_READ and FILE_GENERIC_EXECUTE don't map
> one-to-one with the Windows/NTFS file permissions.  Rather, they are mostly
> used to specify the type of access a particular operation needs (to a call
> like CreateFileW, which doesn't necessarily create a file but often opens
> one instead).  The system checks the requested access against what's
> allowed by the permissions granted in the file's security descriptor (as
> well as types of shared access allowed by others who already have the file
> open).
>
>
>
> On Fri, Apr 17, 2020 at 4:53 PM Chris Tetreault via llvm-dev <
> llvm-dev at lists.llvm.org> wrote:
>
> Hi,
>
>
>
>                 I’m having an issue where clang-format is setting the
> executable bit on all source files it modifies when using the -i parameter.
> I spent some time troubleshooting this issue today, and I found that
> clang-format create a new temporary file, writes the formatted source into
> that file, then copies it over the old file. Deep in the bowels of
> openNativeFile in lib/Support/Windows/Path.inc, in openNativeFileInternal,
> CreateFileW is called with a SECURITY_ATTRIBUTES with lpSecurityDescriptor
> = nullptr. The result of this is you get a new file with the default
> permissions based on whatever NTFS decides to do. On my machine, this ends
> up being a file with 755 mode. This is happening because the mode parameter
> to openNativeFile is unused. This issue occurs in clang-format 9, and on
> HEAD of master.
>
>
>
>                 I spent some time thinking about how to improve this state
> of affairs. I feel like just letting files modified by clang-format get
> their permissions changed severely limits the convenience of the tool. Just
> some thoughts I had:
>
>
>
> 1.      Would it be so terrible if files created by openNativeFile on
> windows queried the default SECURITY_ATTRIBUTES and stripped executable
> permissions? Does anything actually need llvm to produce files with
> executable permissions? I did a quick search but can’t find anywhere in the
> codebase that actually sets a value for mode.
>
> 2.      How would we even map unix-style permissions to windows? I see in
> the MS docs that there are coarse-grained permission types that map to unix
> permissions (FILE_GENERIC_READ, FILE_GENERIC_WRITE, and
> FILE_GENERIC_EXECUTE). For file creation, the current user could be the
> owner. Perhaps all groups the user is a member of could get the group
> permissions, and maybe Authenticated Users for other?
>
> 3.      On my previous project, we used clang-format, and I never had
> this issue. I was using a very old version though, so I don’t know if my
> configuration is just different, or if this behavior changed at some point
>
>
>
> Thanks,
>
>                 Christopher Tetreault
>
> _______________________________________________
> LLVM Developers mailing list
> llvm-dev at lists.llvm.org
> https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev
> <https://urldefense.com/v3/__https:/lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev__;!!JmoZiZGBv3RvKRSx!uFI6EncSLawiF8_tUteK_rGAExNn6WIHz8XYhuuZ6C62D3a852NK-FDno1smmiyVQg$>
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-dev/attachments/20200420/85dd34a2/attachment-0001.html>