clang-format sets executable permission on windows (openNativeFile ignores mode on Windows)

classic Classic list List threaded Threaded
7 messages Options
Reply | Threaded
Open this post in threaded view
|

clang-format sets executable permission on windows (openNativeFile ignores mode on Windows)

Fangrui Song via cfe-dev

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


_______________________________________________
cfe-dev mailing list
[hidden email]
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev
Reply | Threaded
Open this post in threaded view
|

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

Fangrui Song via cfe-dev
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 <[hidden email]> 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
[hidden email]
https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev

_______________________________________________
cfe-dev mailing list
[hidden email]
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev
Reply | Threaded
Open this post in threaded view
|

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

Fangrui Song via cfe-dev

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 <[hidden email]>
Sent: Monday, April 20, 2020 8:36 AM
To: Chris Tetreault <[hidden email]>
Cc: LLVM Dev <[hidden email]>; [hidden email]
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 <[hidden email]> 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
[hidden email]
https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev


_______________________________________________
cfe-dev mailing list
[hidden email]
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev
Reply | Threaded
Open this post in threaded view
|

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

Fangrui Song via cfe-dev
That's weird.

All of the LLVM source files I have from git.exe have "Read & Execute" permission.  If those aren't showing execute permission for you under Cygwin then we need to understand how Cygwin is mapping Windows file permissions to the Unix model before we can determine what we would need to change in LLVM to get the same outcome.

Can you use the Window's File Explorer properties page to compare the security settings of two source files, one that shows 644 in Cywin and one that shows 755 after being re-written by clang-format?

On Mon, Apr 20, 2020 at 9:32 AM Chris Tetreault <[hidden email]> wrote:

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 <[hidden email]>
Sent: Monday, April 20, 2020 8:36 AM
To: Chris Tetreault <[hidden email]>
Cc: LLVM Dev <[hidden email]>; [hidden email]
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 <[hidden email]> 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
[hidden email]
https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev


_______________________________________________
cfe-dev mailing list
[hidden email]
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev
Reply | Threaded
Open this post in threaded view
|

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

Fangrui Song via cfe-dev

-rw-r--r-- 1 my_user Domain Users 0 Apr 20 10:25 foo

 

foo has Windows permissions:

Everyone: Read

Domain Users: Read

my_user: Read, Write

 

-rwxr-xr-x 1 my_user Domain Users 0 Apr 20 10:25 foo

 

foo has Windows permissions:

Everyone: Read, Execute

Domain Users: Read, Exectue

my_user: Full Control

 

It seems that Cygwin actually reads and writes NTFS permissions

 

Thanks,

   Christopher Tetreault

 

From: Adrian McCarthy <[hidden email]>
Sent: Monday, April 20, 2020 10:02 AM
To: Chris Tetreault <[hidden email]>
Cc: LLVM Dev <[hidden email]>; [hidden email]
Subject: [EXT] Re: [llvm-dev] clang-format sets executable permission on windows (openNativeFile ignores mode on Windows)

 

That's weird.

 

All of the LLVM source files I have from git.exe have "Read & Execute" permission.  If those aren't showing execute permission for you under Cygwin then we need to understand how Cygwin is mapping Windows file permissions to the Unix model before we can determine what we would need to change in LLVM to get the same outcome.

 

Can you use the Window's File Explorer properties page to compare the security settings of two source files, one that shows 644 in Cywin and one that shows 755 after being re-written by clang-format?

 

On Mon, Apr 20, 2020 at 9:32 AM Chris Tetreault <[hidden email]> wrote:

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 <[hidden email]>
Sent: Monday, April 20, 2020 8:36 AM
To: Chris Tetreault <[hidden email]>
Cc: LLVM Dev <[hidden email]>; [hidden email]
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 <[hidden email]> 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
[hidden email]
https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev


_______________________________________________
cfe-dev mailing list
[hidden email]
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev
Reply | Threaded
Open this post in threaded view
|

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

Fangrui Song via cfe-dev
In reply to this post by Fangrui Song via cfe-dev

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 <[hidden email]>
Sent: Monday, April 20, 2020 11:47 AM
To: Adrian McCarthy <[hidden email]>
Cc: Chris Tetreault <[hidden email]>; LLVM Dev <[hidden email]>
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 <[hidden email]>
Sent: Monday, April 20, 2020 1:48 PM
To: Robinson, Paul <[hidden email]>
Cc: Chris Tetreault <[hidden email]>; LLVM Dev <[hidden email]>
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 

 

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 <[hidden email]> 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 <[hidden email]> On Behalf Of Chris Tetreault via cfe-dev
Sent: Monday, April 20, 2020 12:32 PM
To: Adrian McCarthy <[hidden email]>
Cc: LLVM Dev <[hidden email]>; [hidden email]
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 <[hidden email]>
Sent: Monday, April 20, 2020 8:36 AM
To: Chris Tetreault <[hidden email]>
Cc: LLVM Dev <[hidden email]>; [hidden email]
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 <[hidden email]> 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
[hidden email]
https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev


_______________________________________________
cfe-dev mailing list
[hidden email]
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev
Reply | Threaded
Open this post in threaded view
|

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

Fangrui Song 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.


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 <[hidden email]> 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 <[hidden email]>
Sent: Monday, April 20, 2020 11:47 AM
To: Adrian McCarthy <[hidden email]>
Cc: Chris Tetreault <[hidden email]>; LLVM Dev <[hidden email]>
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 <[hidden email]>
Sent: Monday, April 20, 2020 1:48 PM
To: Robinson, Paul <[hidden email]>
Cc: Chris Tetreault <[hidden email]>; LLVM Dev <[hidden email]>
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 

 

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 <[hidden email]> 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 <[hidden email]> On Behalf Of Chris Tetreault via cfe-dev
Sent: Monday, April 20, 2020 12:32 PM
To: Adrian McCarthy <[hidden email]>
Cc: LLVM Dev <[hidden email]>; [hidden email]
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 <[hidden email]>
Sent: Monday, April 20, 2020 8:36 AM
To: Chris Tetreault <[hidden email]>
Cc: LLVM Dev <[hidden email]>; [hidden email]
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 <[hidden email]> 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
[hidden email]
https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev


_______________________________________________
cfe-dev mailing list
[hidden email]
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev