llvm dev - Apr 2020 - clang-format sets executable permission on windows (openNativeFile ignores mode on Windows)

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
-------------- next part --------------
An HTML attachment was scrubbed...
URL:
<http://lists.llvm.org/pipermail/llvm-dev/attachments/20200417/31a2d4c1/attachment.html>

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
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL:
<http://lists.llvm.org/pipermail/llvm-dev/attachments/20200420/8c8f3908/attachment.html>

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<mailto: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<mailto:llvm-dev at lists.llvm.org>
https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev
-------------- next part --------------
An HTML attachment was scrubbed...
URL:
<http://lists.llvm.org/pipermail/llvm-dev/attachments/20200420/8aae152e/attachment.html>