Tar, Zip: respect umask when creating files.#71647
Conversation
ghost
added
area-System.IO
community-contribution
|
Tagging subscribers to this area: @dotnet/area-system-io Issue DetailsAddresses #69760 and makes similar changes for Zip. @carlossanlop @eerhardt @dotnet/area-system-io ptal.
|
adamsitnik
left a comment
adamsitnik
left a comment
There was a problem hiding this comment.
The umask changes LGTM, but I would like to clarify PreallocationSize change before we merge.
Thank you @tmds !
src/libraries/System.Formats.Tar/src/System/Formats/Tar/TarEntry.cs
Outdated
Show resolved
Hide resolved
src/libraries/System.Formats.Tar/src/System/Formats/Tar/TarEntry.cs
Outdated
Show resolved
Hide resolved
src/libraries/System.Formats.Tar/src/System/Formats/Tar/TarEntry.cs
Outdated
Show resolved
Hide resolved
src/libraries/System.Formats.Tar/src/System/Formats/Tar/TarEntry.cs
Outdated
Show resolved
Hide resolved
...pression.ZipFile/src/System/IO/Compression/ZipFileExtensions.ZipArchiveEntry.Extract.Unix.cs
Show resolved
Hide resolved
src/libraries/System.IO.Compression.ZipFile/tests/ZipFile.Unix.cs
Outdated
Show resolved
Hide resolved
| fileStreamOptions.UnixCreateMode = (UnixFileMode)Convert.ToInt32(expectedPermissions, 8); | ||
| } | ||
| } | ||
| new FileStream(filename, fileStreamOptions).Dispose(); |
There was a problem hiding this comment.
Are the current tests failing without this change? Before we would only create a new file when the expectedPermissions was null or empty. For the other 3 .zip files, we were straight expecting the mode hard-coded into the tests.
Now we are always creating a new file and using its mode.
There was a problem hiding this comment.
Yes. The current tests require the exact permission. Here we're determining the expected permission that takes into account the umask.
There was a problem hiding this comment.
The changes to this test suite are to update the expected permissions so they take into account the umask.
I think they still cover what was intended.
@eerhardt is this good for you?
There was a problem hiding this comment.
My concern is that we aren't testing that GroupWrite and OtherWrite bits are set correctly anymore. I think we should have tests that respect the umask (what you are fixing here) and tests that clear the umask and ensure the full permissions are kept.
There was a problem hiding this comment.
I've added tests that run with a zero umask. ptal.
|
@eerhardt I'll address your feedback tomorrow. |
|
/azp run runtime-extra-platforms |
|
Azure Pipelines successfully started running 1 pipeline(s). |
carlossanlop
left a comment
carlossanlop
left a comment
There was a problem hiding this comment.
Thank you so much for helping with this, @tmds. I ran the runtime-extra-platforms pipeline to check the results in all the exotic platforms.
Did you find all the existing tar tests sufficient to verify your changes?
| const UnixFileMode OwnershipPermissions = | ||
| UnixFileMode.UserRead | UnixFileMode.UserWrite | UnixFileMode.UserExecute | | ||
| UnixFileMode.GroupRead | UnixFileMode.GroupWrite | UnixFileMode.GroupExecute | | ||
| UnixFileMode.OtherRead | UnixFileMode.OtherWrite | UnixFileMode.OtherExecute; |
There was a problem hiding this comment.
Should we have some additional members on UnixFileMode that represent common combinations of these flags?
There was a problem hiding this comment.
I had suggested some as part of the API proposal. They were left out because they'd mess up 'ToString'.
These are common combinations defined in stat.h:
ACCESSPERMS 0777
DEFFILEMODE 0666
ALLPERMS 07777
cc @eerhardt
There was a problem hiding this comment.
Maybe our internal usage is enough justification to add these common combinations now?
cc @bartonjs
There was a problem hiding this comment.
Having the consts somewhere and having them be values in the enum are two different things.
If OwnershipPermissions were defined as-shown in the enum then having the 0777 value would ToString() not as UserRead | UserWrite | ... but as OwnershipPermissions, which gets... weird.
Putting them somewhere else as a public const doesn't impact the ToString() behavior. The best I can see would be something like File.UnixOwnershipMask.
I checked. Tar is missing tests similar to those in Can we defer these tests to another PR? I'd like to finish this before I go on PTO mid next week. |
| [LibraryImport("libc", StringMarshalling = StringMarshalling.Utf8, SetLastError = true)] | ||
| private static partial int mkfifo(string path, int mode); | ||
|
|
||
| [LibraryImport("libc", StringMarshalling = StringMarshalling.Utf8)] |
There was a problem hiding this comment.
| [LibraryImport("libc", StringMarshalling = StringMarshalling.Utf8)] | |
| [LibraryImport("libc")] |
No strings are being marshalled here, so no need.
This can be addressed in a different PR, if we don't want to reset CI.
6 participants
Addresses #69760 and makes similar changes for Zip.
@carlossanlop @eerhardt @dotnet/area-system-io ptal.