Skip to content

Tar, Zip: respect umask when creating files. - #71647

Merged
eerhardt merged 5 commits into
dotnet:mainfrom
tmds:zip_tar_umask
Jul 8, 2022
Merged

Tar, Zip: respect umask when creating files.#71647
eerhardt merged 5 commits into
dotnet:mainfrom
tmds:zip_tar_umask

Conversation

@tmds

@tmdstmds commented Jul 5, 2022

Copy link
Copy Markdown
Member

Addresses #69760 and makes similar changes for Zip.

@carlossanlop@eerhardt @dotnet/area-system-io ptal.

@ghostghost added area-System.IO community-contribution Indicates that the PR has been added by a community member labels Jul 5, 2022
@ghost

ghost commented Jul 5, 2022

Copy link
Copy Markdown

Tagging subscribers to this area: @dotnet/area-system-io
See info in area-owners.md if you want to be subscribed.

Issue Details

Addresses #69760 and makes similar changes for Zip.

@carlossanlop@eerhardt @dotnet/area-system-io ptal.

Author:tmds
Assignees:-
Labels:

area-System.IO

Milestone:-

adamsitnik
adamsitnik reviewed Jul 6, 2022
View reviewed changes

@adamsitnikadamsitnik left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The umask changes LGTM, but I would like to clarify PreallocationSize change before we merge.

Thank you @tmds !

Comment threadsrc/libraries/System.Formats.Tar/src/System/Formats/Tar/TarEntry.cs Outdated
Comment threadsrc/libraries/System.Formats.Tar/src/System/Formats/Tar/TarEntry.cs Outdated
@adamsitnikadamsitnik added this to the 7.0.0 milestone Jul 6, 2022
@adamsitnikadamsitnik self-assigned this Jul 6, 2022
eerhardt
eerhardt reviewed Jul 6, 2022
View reviewed changes
Comment threadsrc/libraries/System.Formats.Tar/src/System/Formats/Tar/TarEntry.cs Outdated
eerhardt
eerhardt reviewed Jul 6, 2022
View reviewed changes
Comment threadsrc/libraries/System.Formats.Tar/src/System/Formats/Tar/TarEntry.cs Outdated
eerhardt
eerhardt reviewed Jul 6, 2022
View reviewed changes
Comment threadsrc/libraries/System.IO.Compression.ZipFile/tests/ZipFile.Unix.cs Outdated
eerhardt
eerhardt reviewed Jul 6, 2022
View reviewed changes
Comment threadsrc/libraries/System.IO.Compression.ZipFile/tests/ZipFile.Unix.cs
fileStreamOptions.UnixCreateMode = (UnixFileMode)Convert.ToInt32(expectedPermissions, 8);
}
}
new FileStream(filename, fileStreamOptions).Dispose();

@eerhardteerhardtJul 6, 2022

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@tmdstmdsJul 6, 2022

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes. The current tests require the exact permission. Here we're determining the expected permission that takes into account the umask.

@tmdstmdsJul 7, 2022

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

@eerhardteerhardtJul 7, 2022

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@tmdstmdsJul 8, 2022

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've added tests that run with a zero umask. ptal.

@tmds

tmds commented Jul 6, 2022

Copy link
Copy Markdown
MemberAuthor

@eerhardt I'll address your feedback tomorrow.

@carlossanlop

carlossanlop commented Jul 7, 2022

Copy link
Copy Markdown
Contributor

/azp run runtime-extra-platforms

@azure-pipelines

azure-pipelinesBot commented Jul 7, 2022

Copy link
Copy Markdown
Azure Pipelines successfully started running 1 pipeline(s).

carlossanlop
carlossanlop reviewed Jul 7, 2022
View reviewed changes

@carlossanlopcarlossanlop left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

stephentoub
stephentoub reviewed Jul 7, 2022
View reviewed changes
Comment threadsrc/libraries/System.Formats.Tar/src/System/Formats/Tar/TarEntry.cs
const UnixFileMode OwnershipPermissions =
UnixFileMode.UserRead | UnixFileMode.UserWrite | UnixFileMode.UserExecute |
UnixFileMode.GroupRead | UnixFileMode.GroupWrite | UnixFileMode.GroupExecute |
UnixFileMode.OtherRead | UnixFileMode.OtherWrite | UnixFileMode.OtherExecute;

@stephentoubstephentoubJul 7, 2022

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we have some additional members on UnixFileMode that represent common combinations of these flags?

@tmdstmdsJul 8, 2022

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@eerhardteerhardtJul 8, 2022

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe our internal usage is enough justification to add these common combinations now?

cc @bartonjs

@bartonjsbartonjsJul 8, 2022

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@carlossanlopcarlossanlop mentioned this pull request Jul 7, 2022
Closed
@tmds

tmds commented Jul 8, 2022

Copy link
Copy Markdown
MemberAuthor

Did you find all the existing tar tests sufficient to verify your changes?

I checked. Tar is missing tests similar to those in src/libraries/System.IO.Compression.ZipFile/tests/ZipFile.Unix.cs.
These tests verify the mode gets stored and restored from/to the filesystem.

Can we defer these tests to another PR? I'd like to finish this before I go on PTO mid next week.
If I find some time for it still, I'll make make a PR with the tests next week.

eerhardt
eerhardt approved these changes Jul 8, 2022
View reviewed changes

@eerhardteerhardt left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. Thanks @tmds!

Comment threadsrc/libraries/System.IO.Compression.ZipFile/tests/ZipFile.Unix.cs
[LibraryImport("libc", StringMarshalling = StringMarshalling.Utf8, SetLastError = true)]
private static partial int mkfifo(string path, int mode);

[LibraryImport("libc", StringMarshalling = StringMarshalling.Utf8)]

@eerhardteerhardtJul 8, 2022

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
[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.

@eerhardt
eerhardt merged commit 8a192f8 into dotnet:mainJul 8, 2022
@carlossanlopcarlossanlop mentioned this pull request Jul 12, 2022
Open
54 tasks
@tmdstmds mentioned this pull request Jul 13, 2022
Merged
This was referenced Jul 20, 2022
Closed
Closed
@karelzkarelz mentioned this pull request Aug 7, 2022
Closed
@ghostghost locked as resolved and limited conversation to collaborators Aug 7, 2022
Sign up for freeto subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

@adamsitnikadamsitnikadamsitnik left review comments
@eerhardteerhardteerhardt approved these changes
@bartonjsbartonjsbartonjs left review comments
@stephentoubstephentoubstephentoub approved these changes
+1 more reviewer
@carlossanlopcarlossanlopcarlossanlop left review comments
Reviewers whose approvals may not affect merge requirements

Assignees

@adamsitnikadamsitnik

Labels

area-System.IO.Compressioncommunity-contributionIndicates that the PR has been added by a community member

Projects

None yet

Milestone

7.0.0

Development

Successfully merging this pull request may close these issues.

6 participants

@tmds@carlossanlop@stephentoub@adamsitnik@eerhardt@bartonjs
, 'i'); if (__m === '*' || __re.test(location.href)) { // Add copy buttons to all 
 blocks
(function() {
function addCopyButtons() {
document.querySelectorAll('pre code').forEach(function(codeBlock) {
if (codeBlock.parentElement.hasAttribute('data-copy-added')) return;
codeBlock.parentElement.setAttribute('data-copy-added', 'true');
var btn = document.createElement('button');
btn.textContent = 'Copy';
btn.style.cssText = 'position:absolute;top:4px;right:4px;padding:2px 8px;font-size:11px;background:#4ecdc4;border:none;border-radius:4px;color:#1a1a2e;cursor:pointer;opacity:0.7;transition:opacity 0.2s;';
btn.onmouseover = function() { this.style.opacity = '1'; };
btn.onmouseout = function() { this.style.opacity = '0.7'; };
btn.onclick = function() {
navigator.clipboard.writeText(codeBlock.textContent).then(function() {
btn.textContent = 'Copied!';
setTimeout(function() { btn.textContent = 'Copy'; }, 1500);
});
};
codeBlock.parentElement.style.position = 'relative';
codeBlock.parentElement.appendChild(btn);
});
}
addCopyButtons();
// Re-run on dynamic content
var observer = new MutationObserver(addCopyButtons);
observer.observe(document.body, { childList: true, subtree: true });
})();
}
} catch(__e) { console.warn('[Userscript:Add Copy Buttons to Code Blocks]', __e); }
})();
(function(){
try {
var __m = "github.com";
var __re = new RegExp('^' + "github\\.com" + '
Tar, Zip: respect umask when creating files. by tmds · Pull Request #71647 · dotnet/runtime · GitHub
Skip to content
Tar, Zip: respect umask when creating files. - #71647
Merged
eerhardt merged 5 commits into
dotnet:mainfrom 
tmds:zip_tar_umask
Jul 8, 2022
Merged
Tar, Zip: respect umask when creating files.#71647
eerhardt merged 5 commits into
dotnet:mainfrom 
tmds:zip_tar_umask
Conversation
@tmds
@tmdstmds
commented
Jul 5, 2022

Copy link


Copy Markdown

Member
Addresses #69760 and makes similar changes for Zip.
@carlossanlop@eerhardt @dotnet/area-system-io ptal.
@ghostghost
added

area-System.IO

community-contribution
Indicates that the PR has been added by a community member
labels
Jul 5, 2022
@ghost
ghost
commented
Jul 5, 2022

Copy link


Copy Markdown

Tagging subscribers to this area: @dotnet/area-system-io

See info in area-owners.md if you want to be subscribed.
Issue Details
Addresses #69760 and makes similar changes for Zip.
@carlossanlop@eerhardt @dotnet/area-system-io ptal.
Author:tmds
Assignees:-
Labels:
area-System.IO
Milestone:-
adamsitnik
adamsitnik
reviewed
Jul 6, 2022
View reviewed changes
@adamsitnikadamsitnik
left a comment


Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The umask changes LGTM, but I would like to clarify PreallocationSize change before we merge.
Thank you @tmds !
Comment threadsrc/libraries/System.Formats.Tar/src/System/Formats/Tar/TarEntry.cs
Outdated

Comment threadsrc/libraries/System.Formats.Tar/src/System/Formats/Tar/TarEntry.cs
Outdated

@adamsitnikadamsitnik
added this to the 7.0.0 milestone
Jul 6, 2022
@adamsitnikadamsitnik
self-assigned this
Jul 6, 2022
eerhardt
eerhardt
reviewed
Jul 6, 2022
View reviewed changes
Comment threadsrc/libraries/System.Formats.Tar/src/System/Formats/Tar/TarEntry.cs
Outdated

eerhardt
eerhardt
reviewed
Jul 6, 2022
View reviewed changes
Comment threadsrc/libraries/System.Formats.Tar/src/System/Formats/Tar/TarEntry.cs
Outdated

eerhardt
eerhardt
reviewed
Jul 6, 2022
View reviewed changes
Comment threadsrc/libraries/System.IO.Compression.ZipFile/tests/ZipFile.Unix.cs
Outdated

eerhardt
eerhardt
reviewed
Jul 6, 2022
View reviewed changes
Comment threadsrc/libraries/System.IO.Compression.ZipFile/tests/ZipFile.Unix.cs
 fileStreamOptions.UnixCreateMode = (UnixFileMode)Convert.ToInt32(expectedPermissions, 8);
 }
}
 new FileStream(filename, fileStreamOptions).Dispose();
@eerhardteerhardtJul 6, 2022

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.
@tmdstmdsJul 6, 2022

Copy link


Copy Markdown

MemberAuthor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes. The current tests require the exact permission. Here we're determining the expected permission that takes into account the umask.
@tmdstmdsJul 7, 2022

Copy link


Copy Markdown

MemberAuthor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?
@eerhardteerhardtJul 7, 2022

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.
@tmdstmdsJul 8, 2022

Copy link


Copy Markdown

MemberAuthor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've added tests that run with a zero umask. ptal.
@tmds
tmds
commented
Jul 6, 2022

Copy link


Copy Markdown

MemberAuthor
@eerhardt I'll address your feedback tomorrow.
@carlossanlop
carlossanlop
commented
Jul 7, 2022

Copy link


Copy Markdown

Contributor
/azp run runtime-extra-platforms
@azure-pipelines
azure-pipelinesBot
commented
Jul 7, 2022

Copy link


Copy Markdown


Azure Pipelines successfully started running 1 pipeline(s).
carlossanlop
carlossanlop
reviewed
Jul 7, 2022
View reviewed changes
@carlossanlopcarlossanlop
left a comment


Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?
stephentoub
stephentoub
reviewed
Jul 7, 2022
View reviewed changes
Comment threadsrc/libraries/System.Formats.Tar/src/System/Formats/Tar/TarEntry.cs
 const UnixFileMode OwnershipPermissions =
 UnixFileMode.UserRead | UnixFileMode.UserWrite | UnixFileMode.UserExecute |
 UnixFileMode.GroupRead | UnixFileMode.GroupWrite | UnixFileMode.GroupExecute |
 UnixFileMode.OtherRead | UnixFileMode.OtherWrite | UnixFileMode.OtherExecute;
@stephentoubstephentoubJul 7, 2022

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we have some additional members on UnixFileMode that represent common combinations of these flags?
@tmdstmdsJul 8, 2022

Copy link


Copy Markdown

MemberAuthor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
@eerhardteerhardtJul 8, 2022

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe our internal usage is enough justification to add these common combinations now?
cc @bartonjs
@bartonjsbartonjsJul 8, 2022

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.
@carlossanlopcarlossanlop
mentioned this pull request
Jul 7, 2022

Closed

@tmds
tmds
commented
Jul 8, 2022

Copy link


Copy Markdown

MemberAuthor
Did you find all the existing tar tests sufficient to verify your changes?
I checked. Tar is missing tests similar to those in src/libraries/System.IO.Compression.ZipFile/tests/ZipFile.Unix.cs.

These tests verify the mode gets stored and restored from/to the filesystem.
Can we defer these tests to another PR? I'd like to finish this before I go on PTO mid next week.

If I find some time for it still, I'll make make a PR with the tests next week.
eerhardt
eerhardt
approved these changes
Jul 8, 2022
View reviewed changes
@eerhardteerhardt
left a comment


Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. Thanks @tmds!
Comment threadsrc/libraries/System.IO.Compression.ZipFile/tests/ZipFile.Unix.cs
 [LibraryImport("libc", StringMarshalling = StringMarshalling.Utf8, SetLastError = true)]
 private static partial int mkfifo(string path, int mode);

 [LibraryImport("libc", StringMarshalling = StringMarshalling.Utf8)]
@eerhardteerhardtJul 8, 2022

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.


Suggested change

[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.
@eerhardt
eerhardt
merged commit 8a192f8
into
dotnet:mainJul 8, 2022
@carlossanlopcarlossanlop
mentioned this pull request
Jul 12, 2022

Open

54 tasks
@tmdstmds
mentioned this pull request
Jul 13, 2022
 Merged


This was referenced Jul 20, 2022

Closed


Closed

@karelzkarelz
mentioned this pull request
Aug 7, 2022

Closed

@ghostghost
locked as resolved and limited conversation to collaborators
Aug 7, 2022
Sign up for freeto subscribe to this conversation on GitHub.
Already have an account?
Sign in.


Reviewers

@adamsitnikadamsitnikadamsitnik left review comments
@eerhardteerhardteerhardt approved these changes
@bartonjsbartonjsbartonjs left review comments
@stephentoubstephentoubstephentoub approved these changes

+1 more reviewer

@carlossanlopcarlossanlopcarlossanlop left review comments
Reviewers whose approvals may not affect merge requirements

Assignees

@adamsitnikadamsitnik

Labels

area-System.IO.Compressioncommunity-contributionIndicates that the PR has been added by a community member

Projects


None yet


Milestone

7.0.0

Development

Successfully merging this pull request may close these issues.

6 participants

@tmds@carlossanlop@stephentoub@adamsitnik@eerhardt@bartonjs

, 'i');
if (__m === '*' || __re.test(location.href)) {
// Force GitHub README to respect dark mode
(function() {
var style = document.createElement('style');
style.textContent = '
.markdown-body {
color-scheme: dark light;
}
.markdown-body pre { background: #161b22 !important; }
.markdown-body code { background: rgba(110, 118, 129, 0.4) !important; }
.markdown-body table th, .markdown-body table td { border-color: #30363d !important; }
.markdown-body img { background: #0d1117; }
.markdown-body blockquote { border-left-color: #8b949e; }
.markdown-body hr { border-color: #30363d; }
';
document.head.appendChild(style);
})();
}
} catch(__e) { console.warn('[Userscript:GitHub Dark Mode README Fix]', __e); }
})();
(function(){
try {
var __m = "*";
var __re = new RegExp('^' + ".*" + '
Tar, Zip: respect umask when creating files. by tmds · Pull Request #71647 · dotnet/runtime · GitHub
Skip to content
Tar, Zip: respect umask when creating files. - #71647
Merged
eerhardt merged 5 commits into
dotnet:mainfrom 
tmds:zip_tar_umask
Jul 8, 2022
Merged
Tar, Zip: respect umask when creating files.#71647
eerhardt merged 5 commits into
dotnet:mainfrom 
tmds:zip_tar_umask
Conversation
@tmds
@tmdstmds
commented
Jul 5, 2022

Copy link


Copy Markdown

Member
Addresses #69760 and makes similar changes for Zip.
@carlossanlop@eerhardt @dotnet/area-system-io ptal.
@ghostghost
added

area-System.IO

community-contribution
Indicates that the PR has been added by a community member
labels
Jul 5, 2022
@ghost
ghost
commented
Jul 5, 2022

Copy link


Copy Markdown

Tagging subscribers to this area: @dotnet/area-system-io

See info in area-owners.md if you want to be subscribed.
Issue Details
Addresses #69760 and makes similar changes for Zip.
@carlossanlop@eerhardt @dotnet/area-system-io ptal.
Author:tmds
Assignees:-
Labels:
area-System.IO
Milestone:-
adamsitnik
adamsitnik
reviewed
Jul 6, 2022
View reviewed changes
@adamsitnikadamsitnik
left a comment


Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The umask changes LGTM, but I would like to clarify PreallocationSize change before we merge.
Thank you @tmds !
Comment threadsrc/libraries/System.Formats.Tar/src/System/Formats/Tar/TarEntry.cs
Outdated

Comment threadsrc/libraries/System.Formats.Tar/src/System/Formats/Tar/TarEntry.cs
Outdated

@adamsitnikadamsitnik
added this to the 7.0.0 milestone
Jul 6, 2022
@adamsitnikadamsitnik
self-assigned this
Jul 6, 2022
eerhardt
eerhardt
reviewed
Jul 6, 2022
View reviewed changes
Comment threadsrc/libraries/System.Formats.Tar/src/System/Formats/Tar/TarEntry.cs
Outdated

eerhardt
eerhardt
reviewed
Jul 6, 2022
View reviewed changes
Comment threadsrc/libraries/System.Formats.Tar/src/System/Formats/Tar/TarEntry.cs
Outdated

eerhardt
eerhardt
reviewed
Jul 6, 2022
View reviewed changes
Comment threadsrc/libraries/System.IO.Compression.ZipFile/tests/ZipFile.Unix.cs
Outdated

eerhardt
eerhardt
reviewed
Jul 6, 2022
View reviewed changes
Comment threadsrc/libraries/System.IO.Compression.ZipFile/tests/ZipFile.Unix.cs
 fileStreamOptions.UnixCreateMode = (UnixFileMode)Convert.ToInt32(expectedPermissions, 8);
 }
}
 new FileStream(filename, fileStreamOptions).Dispose();
@eerhardteerhardtJul 6, 2022

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.
@tmdstmdsJul 6, 2022

Copy link


Copy Markdown

MemberAuthor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes. The current tests require the exact permission. Here we're determining the expected permission that takes into account the umask.
@tmdstmdsJul 7, 2022

Copy link


Copy Markdown

MemberAuthor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?
@eerhardteerhardtJul 7, 2022

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.
@tmdstmdsJul 8, 2022

Copy link


Copy Markdown

MemberAuthor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've added tests that run with a zero umask. ptal.
@tmds
tmds
commented
Jul 6, 2022

Copy link


Copy Markdown

MemberAuthor
@eerhardt I'll address your feedback tomorrow.
@carlossanlop
carlossanlop
commented
Jul 7, 2022

Copy link


Copy Markdown

Contributor
/azp run runtime-extra-platforms
@azure-pipelines
azure-pipelinesBot
commented
Jul 7, 2022

Copy link


Copy Markdown


Azure Pipelines successfully started running 1 pipeline(s).
carlossanlop
carlossanlop
reviewed
Jul 7, 2022
View reviewed changes
@carlossanlopcarlossanlop
left a comment


Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?
stephentoub
stephentoub
reviewed
Jul 7, 2022
View reviewed changes
Comment threadsrc/libraries/System.Formats.Tar/src/System/Formats/Tar/TarEntry.cs
 const UnixFileMode OwnershipPermissions =
 UnixFileMode.UserRead | UnixFileMode.UserWrite | UnixFileMode.UserExecute |
 UnixFileMode.GroupRead | UnixFileMode.GroupWrite | UnixFileMode.GroupExecute |
 UnixFileMode.OtherRead | UnixFileMode.OtherWrite | UnixFileMode.OtherExecute;
@stephentoubstephentoubJul 7, 2022

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we have some additional members on UnixFileMode that represent common combinations of these flags?
@tmdstmdsJul 8, 2022

Copy link


Copy Markdown

MemberAuthor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
@eerhardteerhardtJul 8, 2022

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe our internal usage is enough justification to add these common combinations now?
cc @bartonjs
@bartonjsbartonjsJul 8, 2022

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.
@carlossanlopcarlossanlop
mentioned this pull request
Jul 7, 2022

Closed

@tmds
tmds
commented
Jul 8, 2022

Copy link


Copy Markdown

MemberAuthor
Did you find all the existing tar tests sufficient to verify your changes?
I checked. Tar is missing tests similar to those in src/libraries/System.IO.Compression.ZipFile/tests/ZipFile.Unix.cs.

These tests verify the mode gets stored and restored from/to the filesystem.
Can we defer these tests to another PR? I'd like to finish this before I go on PTO mid next week.

If I find some time for it still, I'll make make a PR with the tests next week.
eerhardt
eerhardt
approved these changes
Jul 8, 2022
View reviewed changes
@eerhardteerhardt
left a comment


Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. Thanks @tmds!
Comment threadsrc/libraries/System.IO.Compression.ZipFile/tests/ZipFile.Unix.cs
 [LibraryImport("libc", StringMarshalling = StringMarshalling.Utf8, SetLastError = true)]
 private static partial int mkfifo(string path, int mode);

 [LibraryImport("libc", StringMarshalling = StringMarshalling.Utf8)]
@eerhardteerhardtJul 8, 2022

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.


Suggested change

[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.
@eerhardt
eerhardt
merged commit 8a192f8
into
dotnet:mainJul 8, 2022
@carlossanlopcarlossanlop
mentioned this pull request
Jul 12, 2022

Open

54 tasks
@tmdstmds
mentioned this pull request
Jul 13, 2022
 Merged


This was referenced Jul 20, 2022

Closed


Closed

@karelzkarelz
mentioned this pull request
Aug 7, 2022

Closed

@ghostghost
locked as resolved and limited conversation to collaborators
Aug 7, 2022
Sign up for freeto subscribe to this conversation on GitHub.
Already have an account?
Sign in.


Reviewers

@adamsitnikadamsitnikadamsitnik left review comments
@eerhardteerhardteerhardt approved these changes
@bartonjsbartonjsbartonjs left review comments
@stephentoubstephentoubstephentoub approved these changes

+1 more reviewer

@carlossanlopcarlossanlopcarlossanlop left review comments
Reviewers whose approvals may not affect merge requirements

Assignees

@adamsitnikadamsitnik

Labels

area-System.IO.Compressioncommunity-contributionIndicates that the PR has been added by a community member

Projects


None yet


Milestone

7.0.0

Development

Successfully merging this pull request may close these issues.

6 participants

@tmds@carlossanlop@stephentoub@adamsitnik@eerhardt@bartonjs

, 'i');
if (__m === '*' || __re.test(location.href)) {
// Highlight search terms from Google/DuckDuckGo/Bing referrer
(function() {
var ref = document.referrer;
var terms = [];
if (ref.includes('google.com') || ref.includes('duckduckgo.com') || ref.includes('bing.com')) {
var url = new URL(ref);
var q = url.searchParams.get('q') || url.searchParams.get('p');
if (q) {
terms = q.split(/\s+/).filter(function(t) { return t.length > 2; });
}
}
if (terms.length === 0) return;
var style = document.createElement('style');
style.textContent = '.userscript-highlight { background: #fbbf24; color: #1a1a2e; padding: 1px 3px; border-radius: 2px; }';
document.head.appendChild(style);
function highlight(node) {
if (node.nodeType === 3) { // text node
var text = node.textContent;
var found = false;
terms.forEach(function(term) {
var regex = new RegExp('(' + term.replace(/[.*+?^${}()|[\]\\]/g, '\\') + ')', 'gi');
if (regex.test(text)) {
found = true;
var frag = document.createDocumentFragment();
var parts = text.split(regex);
parts.forEach(function(part, i) {
if (i % 2 === 0) {
frag.appendChild(document.createTextNode(part));
} else {
var span = document.createElement('span');
span.className = 'userscript-highlight';
span.textContent = part;
frag.appendChild(span);
}
});
node.parentNode.replaceChild(frag, node);
}
});
} else if (node.nodeType === 1 && node.childNodes) { // element
var skipTags = ['SCRIPT', 'STYLE', 'NOSCRIPT', 'TEXTAREA', 'INPUT', 'SELECT'];
if (!skipTags.includes(node.tagName)) {
Array.from(node.childNodes).forEach(highlight);
}
}
}
highlight(document.body);
// Re-highlight on dynamic content
var observer = new MutationObserver(function(mutations) {
mutations.forEach(function(m) {
m.addedNodes.forEach(function(node) {
if (node.nodeType === 1 || node.nodeType === 3) highlight(node);
});
});
});
observer.observe(document.body, { childList: true, subtree: true });
})();
}
} catch(__e) { console.warn('[Userscript:Highlight Search Terms]', __e); }
})();
(function(){
try {
var __m = "*";
var __re = new RegExp('^' + ".*" + '
Tar, Zip: respect umask when creating files. by tmds · Pull Request #71647 · dotnet/runtime · GitHub
Skip to content
Tar, Zip: respect umask when creating files. - #71647
Merged
eerhardt merged 5 commits into
dotnet:mainfrom 
tmds:zip_tar_umask
Jul 8, 2022
Merged
Tar, Zip: respect umask when creating files.#71647
eerhardt merged 5 commits into
dotnet:mainfrom 
tmds:zip_tar_umask
Conversation
@tmds
@tmdstmds
commented
Jul 5, 2022

Copy link


Copy Markdown

Member
Addresses #69760 and makes similar changes for Zip.
@carlossanlop@eerhardt @dotnet/area-system-io ptal.
@ghostghost
added

area-System.IO

community-contribution
Indicates that the PR has been added by a community member
labels
Jul 5, 2022
@ghost
ghost
commented
Jul 5, 2022

Copy link


Copy Markdown

Tagging subscribers to this area: @dotnet/area-system-io

See info in area-owners.md if you want to be subscribed.
Issue Details
Addresses #69760 and makes similar changes for Zip.
@carlossanlop@eerhardt @dotnet/area-system-io ptal.
Author:tmds
Assignees:-
Labels:
area-System.IO
Milestone:-
adamsitnik
adamsitnik
reviewed
Jul 6, 2022
View reviewed changes
@adamsitnikadamsitnik
left a comment


Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The umask changes LGTM, but I would like to clarify PreallocationSize change before we merge.
Thank you @tmds !
Comment threadsrc/libraries/System.Formats.Tar/src/System/Formats/Tar/TarEntry.cs
Outdated

Comment threadsrc/libraries/System.Formats.Tar/src/System/Formats/Tar/TarEntry.cs
Outdated

@adamsitnikadamsitnik
added this to the 7.0.0 milestone
Jul 6, 2022
@adamsitnikadamsitnik
self-assigned this
Jul 6, 2022
eerhardt
eerhardt
reviewed
Jul 6, 2022
View reviewed changes
Comment threadsrc/libraries/System.Formats.Tar/src/System/Formats/Tar/TarEntry.cs
Outdated

eerhardt
eerhardt
reviewed
Jul 6, 2022
View reviewed changes
Comment threadsrc/libraries/System.Formats.Tar/src/System/Formats/Tar/TarEntry.cs
Outdated

eerhardt
eerhardt
reviewed
Jul 6, 2022
View reviewed changes
Comment threadsrc/libraries/System.IO.Compression.ZipFile/tests/ZipFile.Unix.cs
Outdated

eerhardt
eerhardt
reviewed
Jul 6, 2022
View reviewed changes
Comment threadsrc/libraries/System.IO.Compression.ZipFile/tests/ZipFile.Unix.cs
 fileStreamOptions.UnixCreateMode = (UnixFileMode)Convert.ToInt32(expectedPermissions, 8);
 }
}
 new FileStream(filename, fileStreamOptions).Dispose();
@eerhardteerhardtJul 6, 2022

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.
@tmdstmdsJul 6, 2022

Copy link


Copy Markdown

MemberAuthor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes. The current tests require the exact permission. Here we're determining the expected permission that takes into account the umask.
@tmdstmdsJul 7, 2022

Copy link


Copy Markdown

MemberAuthor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?
@eerhardteerhardtJul 7, 2022

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.
@tmdstmdsJul 8, 2022

Copy link


Copy Markdown

MemberAuthor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've added tests that run with a zero umask. ptal.
@tmds
tmds
commented
Jul 6, 2022

Copy link


Copy Markdown

MemberAuthor
@eerhardt I'll address your feedback tomorrow.
@carlossanlop
carlossanlop
commented
Jul 7, 2022

Copy link


Copy Markdown

Contributor
/azp run runtime-extra-platforms
@azure-pipelines
azure-pipelinesBot
commented
Jul 7, 2022

Copy link


Copy Markdown


Azure Pipelines successfully started running 1 pipeline(s).
carlossanlop
carlossanlop
reviewed
Jul 7, 2022
View reviewed changes
@carlossanlopcarlossanlop
left a comment


Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?
stephentoub
stephentoub
reviewed
Jul 7, 2022
View reviewed changes
Comment threadsrc/libraries/System.Formats.Tar/src/System/Formats/Tar/TarEntry.cs
 const UnixFileMode OwnershipPermissions =
 UnixFileMode.UserRead | UnixFileMode.UserWrite | UnixFileMode.UserExecute |
 UnixFileMode.GroupRead | UnixFileMode.GroupWrite | UnixFileMode.GroupExecute |
 UnixFileMode.OtherRead | UnixFileMode.OtherWrite | UnixFileMode.OtherExecute;
@stephentoubstephentoubJul 7, 2022

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we have some additional members on UnixFileMode that represent common combinations of these flags?
@tmdstmdsJul 8, 2022

Copy link


Copy Markdown

MemberAuthor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
@eerhardteerhardtJul 8, 2022

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe our internal usage is enough justification to add these common combinations now?
cc @bartonjs
@bartonjsbartonjsJul 8, 2022

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.
@carlossanlopcarlossanlop
mentioned this pull request
Jul 7, 2022

Closed

@tmds
tmds
commented
Jul 8, 2022

Copy link


Copy Markdown

MemberAuthor
Did you find all the existing tar tests sufficient to verify your changes?
I checked. Tar is missing tests similar to those in src/libraries/System.IO.Compression.ZipFile/tests/ZipFile.Unix.cs.

These tests verify the mode gets stored and restored from/to the filesystem.
Can we defer these tests to another PR? I'd like to finish this before I go on PTO mid next week.

If I find some time for it still, I'll make make a PR with the tests next week.
eerhardt
eerhardt
approved these changes
Jul 8, 2022
View reviewed changes
@eerhardteerhardt
left a comment


Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. Thanks @tmds!
Comment threadsrc/libraries/System.IO.Compression.ZipFile/tests/ZipFile.Unix.cs
 [LibraryImport("libc", StringMarshalling = StringMarshalling.Utf8, SetLastError = true)]
 private static partial int mkfifo(string path, int mode);

 [LibraryImport("libc", StringMarshalling = StringMarshalling.Utf8)]
@eerhardteerhardtJul 8, 2022

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.


Suggested change

[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.
@eerhardt
eerhardt
merged commit 8a192f8
into
dotnet:mainJul 8, 2022
@carlossanlopcarlossanlop
mentioned this pull request
Jul 12, 2022

Open

54 tasks
@tmdstmds
mentioned this pull request
Jul 13, 2022
 Merged


This was referenced Jul 20, 2022

Closed


Closed

@karelzkarelz
mentioned this pull request
Aug 7, 2022

Closed

@ghostghost
locked as resolved and limited conversation to collaborators
Aug 7, 2022
Sign up for freeto subscribe to this conversation on GitHub.
Already have an account?
Sign in.


Reviewers

@adamsitnikadamsitnikadamsitnik left review comments
@eerhardteerhardteerhardt approved these changes
@bartonjsbartonjsbartonjs left review comments
@stephentoubstephentoubstephentoub approved these changes

+1 more reviewer

@carlossanlopcarlossanlopcarlossanlop left review comments
Reviewers whose approvals may not affect merge requirements

Assignees

@adamsitnikadamsitnik

Labels

area-System.IO.Compressioncommunity-contributionIndicates that the PR has been added by a community member

Projects


None yet


Milestone

7.0.0

Development

Successfully merging this pull request may close these issues.

6 participants

@tmds@carlossanlop@stephentoub@adamsitnik@eerhardt@bartonjs

, 'i');
if (__m === '*' || __re.test(location.href)) {
// Strip utm_, fbclid, gclid, etc. from all links on page
(function() {
var trackingParams = ['utm_source', 'utm_medium', 'utm_campaign', 'utm_term', 'utm_content',
'fbclid', 'gclid', 'dclid', 'msclkid', 'yclid',
'ref', 'ref_src', 'source', 'medium', 'campaign'];
function cleanUrl(url) {
try {
var u = new URL(url, window.location.origin);
var changed = false;
trackingParams.forEach(function(p) {
if (u.searchParams.has(p)) {
u.searchParams.delete(p);
changed = true;
}
});
return changed ? u.toString() : url;
} catch (e) {
return url;
}
}
function cleanLinks() {
document.querySelectorAll('a[href]').forEach(function(a) {
var clean = cleanUrl(a.href);
if (clean !== a.href) a.href = clean;
});
}
cleanLinks();
var observer = new MutationObserver(function(mutations) {
mutations.forEach(function(m) {
m.addedNodes.forEach(function(node) {
if (node.nodeType === 1) {
if (node.tagName === 'A') cleanLinks();
node.querySelectorAll('a[href]').forEach(function(a) {
var clean = cleanUrl(a.href);
if (clean !== a.href) a.href = clean;
});
}
});
});
});
observer.observe(document.body, { childList: true, subtree: true });
})();
}
} catch(__e) { console.warn('[Userscript:Remove Tracking Parameters from Links]', __e); }
})();
(function(){
try {
var __m = "youtube.com";
var __re = new RegExp('^' + "youtube\\.com" + '
Tar, Zip: respect umask when creating files. by tmds · Pull Request #71647 · dotnet/runtime · GitHub
Skip to content
Tar, Zip: respect umask when creating files. - #71647
Merged
eerhardt merged 5 commits into
dotnet:mainfrom 
tmds:zip_tar_umask
Jul 8, 2022
Merged
Tar, Zip: respect umask when creating files.#71647
eerhardt merged 5 commits into
dotnet:mainfrom 
tmds:zip_tar_umask
Conversation
@tmds
@tmdstmds
commented
Jul 5, 2022

Copy link


Copy Markdown

Member
Addresses #69760 and makes similar changes for Zip.
@carlossanlop@eerhardt @dotnet/area-system-io ptal.
@ghostghost
added

area-System.IO

community-contribution
Indicates that the PR has been added by a community member
labels
Jul 5, 2022
@ghost
ghost
commented
Jul 5, 2022

Copy link


Copy Markdown

Tagging subscribers to this area: @dotnet/area-system-io

See info in area-owners.md if you want to be subscribed.
Issue Details
Addresses #69760 and makes similar changes for Zip.
@carlossanlop@eerhardt @dotnet/area-system-io ptal.
Author:tmds
Assignees:-
Labels:
area-System.IO
Milestone:-
adamsitnik
adamsitnik
reviewed
Jul 6, 2022
View reviewed changes
@adamsitnikadamsitnik
left a comment


Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The umask changes LGTM, but I would like to clarify PreallocationSize change before we merge.
Thank you @tmds !
Comment threadsrc/libraries/System.Formats.Tar/src/System/Formats/Tar/TarEntry.cs
Outdated

Comment threadsrc/libraries/System.Formats.Tar/src/System/Formats/Tar/TarEntry.cs
Outdated

@adamsitnikadamsitnik
added this to the 7.0.0 milestone
Jul 6, 2022
@adamsitnikadamsitnik
self-assigned this
Jul 6, 2022
eerhardt
eerhardt
reviewed
Jul 6, 2022
View reviewed changes
Comment threadsrc/libraries/System.Formats.Tar/src/System/Formats/Tar/TarEntry.cs
Outdated

eerhardt
eerhardt
reviewed
Jul 6, 2022
View reviewed changes
Comment threadsrc/libraries/System.Formats.Tar/src/System/Formats/Tar/TarEntry.cs
Outdated

eerhardt
eerhardt
reviewed
Jul 6, 2022
View reviewed changes
Comment threadsrc/libraries/System.IO.Compression.ZipFile/tests/ZipFile.Unix.cs
Outdated

eerhardt
eerhardt
reviewed
Jul 6, 2022
View reviewed changes
Comment threadsrc/libraries/System.IO.Compression.ZipFile/tests/ZipFile.Unix.cs
 fileStreamOptions.UnixCreateMode = (UnixFileMode)Convert.ToInt32(expectedPermissions, 8);
 }
}
 new FileStream(filename, fileStreamOptions).Dispose();
@eerhardteerhardtJul 6, 2022

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.
@tmdstmdsJul 6, 2022

Copy link


Copy Markdown

MemberAuthor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes. The current tests require the exact permission. Here we're determining the expected permission that takes into account the umask.
@tmdstmdsJul 7, 2022

Copy link


Copy Markdown

MemberAuthor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?
@eerhardteerhardtJul 7, 2022

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.
@tmdstmdsJul 8, 2022

Copy link


Copy Markdown

MemberAuthor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've added tests that run with a zero umask. ptal.
@tmds
tmds
commented
Jul 6, 2022

Copy link


Copy Markdown

MemberAuthor
@eerhardt I'll address your feedback tomorrow.
@carlossanlop
carlossanlop
commented
Jul 7, 2022

Copy link


Copy Markdown

Contributor
/azp run runtime-extra-platforms
@azure-pipelines
azure-pipelinesBot
commented
Jul 7, 2022

Copy link


Copy Markdown


Azure Pipelines successfully started running 1 pipeline(s).
carlossanlop
carlossanlop
reviewed
Jul 7, 2022
View reviewed changes
@carlossanlopcarlossanlop
left a comment


Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?
stephentoub
stephentoub
reviewed
Jul 7, 2022
View reviewed changes
Comment threadsrc/libraries/System.Formats.Tar/src/System/Formats/Tar/TarEntry.cs
 const UnixFileMode OwnershipPermissions =
 UnixFileMode.UserRead | UnixFileMode.UserWrite | UnixFileMode.UserExecute |
 UnixFileMode.GroupRead | UnixFileMode.GroupWrite | UnixFileMode.GroupExecute |
 UnixFileMode.OtherRead | UnixFileMode.OtherWrite | UnixFileMode.OtherExecute;
@stephentoubstephentoubJul 7, 2022

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we have some additional members on UnixFileMode that represent common combinations of these flags?
@tmdstmdsJul 8, 2022

Copy link


Copy Markdown

MemberAuthor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
@eerhardteerhardtJul 8, 2022

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe our internal usage is enough justification to add these common combinations now?
cc @bartonjs
@bartonjsbartonjsJul 8, 2022

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.
@carlossanlopcarlossanlop
mentioned this pull request
Jul 7, 2022

Closed

@tmds
tmds
commented
Jul 8, 2022

Copy link


Copy Markdown

MemberAuthor
Did you find all the existing tar tests sufficient to verify your changes?
I checked. Tar is missing tests similar to those in src/libraries/System.IO.Compression.ZipFile/tests/ZipFile.Unix.cs.

These tests verify the mode gets stored and restored from/to the filesystem.
Can we defer these tests to another PR? I'd like to finish this before I go on PTO mid next week.

If I find some time for it still, I'll make make a PR with the tests next week.
eerhardt
eerhardt
approved these changes
Jul 8, 2022
View reviewed changes
@eerhardteerhardt
left a comment


Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. Thanks @tmds!
Comment threadsrc/libraries/System.IO.Compression.ZipFile/tests/ZipFile.Unix.cs
 [LibraryImport("libc", StringMarshalling = StringMarshalling.Utf8, SetLastError = true)]
 private static partial int mkfifo(string path, int mode);

 [LibraryImport("libc", StringMarshalling = StringMarshalling.Utf8)]
@eerhardteerhardtJul 8, 2022

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.


Suggested change

[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.
@eerhardt
eerhardt
merged commit 8a192f8
into
dotnet:mainJul 8, 2022
@carlossanlopcarlossanlop
mentioned this pull request
Jul 12, 2022

Open

54 tasks
@tmdstmds
mentioned this pull request
Jul 13, 2022
 Merged


This was referenced Jul 20, 2022

Closed


Closed

@karelzkarelz
mentioned this pull request
Aug 7, 2022

Closed

@ghostghost
locked as resolved and limited conversation to collaborators
Aug 7, 2022
Sign up for freeto subscribe to this conversation on GitHub.
Already have an account?
Sign in.


Reviewers

@adamsitnikadamsitnikadamsitnik left review comments
@eerhardteerhardteerhardt approved these changes
@bartonjsbartonjsbartonjs left review comments
@stephentoubstephentoubstephentoub approved these changes

+1 more reviewer

@carlossanlopcarlossanlopcarlossanlop left review comments
Reviewers whose approvals may not affect merge requirements

Assignees

@adamsitnikadamsitnik

Labels

area-System.IO.Compressioncommunity-contributionIndicates that the PR has been added by a community member

Projects


None yet


Milestone

7.0.0

Development

Successfully merging this pull request may close these issues.

6 participants

@tmds@carlossanlop@stephentoub@adamsitnik@eerhardt@bartonjs

, 'i');
if (__m === '*' || __re.test(location.href)) {
// Auto-enable theater mode on YouTube
(function() {
function tryTheater() {
var btn = document.querySelector('button[aria-label="Theater mode"], ytd-player #player button[title="Theater mode"]');
if (btn && !btn.classList.contains('activated')) {
btn.click();
}
}
// Try immediately
tryTheater();
// Try after navigation (SPA)
var lastUrl = location.href;
setInterval(function() {
if (location.href !== lastUrl) {
lastUrl = location.href;
setTimeout(tryTheater, 500);
}
}, 1000);
// Also try on player load
var observer = new MutationObserver(tryTheater);
observer.observe(document.body, { childList: true, subtree: true });
})();
}
} catch(__e) { console.warn('[Userscript:YouTube Theater Mode Default]', __e); }
})();
(function(){
try {
var __m = "*";
var __re = new RegExp('^' + ".*" + '
Tar, Zip: respect umask when creating files. by tmds · Pull Request #71647 · dotnet/runtime · GitHub
Skip to content
Tar, Zip: respect umask when creating files. - #71647
Merged
eerhardt merged 5 commits into
dotnet:mainfrom 
tmds:zip_tar_umask
Jul 8, 2022
Merged
Tar, Zip: respect umask when creating files.#71647
eerhardt merged 5 commits into
dotnet:mainfrom 
tmds:zip_tar_umask
Conversation
@tmds
@tmdstmds
commented
Jul 5, 2022

Copy link


Copy Markdown

Member
Addresses #69760 and makes similar changes for Zip.
@carlossanlop@eerhardt @dotnet/area-system-io ptal.
@ghostghost
added

area-System.IO

community-contribution
Indicates that the PR has been added by a community member
labels
Jul 5, 2022
@ghost
ghost
commented
Jul 5, 2022

Copy link


Copy Markdown

Tagging subscribers to this area: @dotnet/area-system-io

See info in area-owners.md if you want to be subscribed.
Issue Details
Addresses #69760 and makes similar changes for Zip.
@carlossanlop@eerhardt @dotnet/area-system-io ptal.
Author:tmds
Assignees:-
Labels:
area-System.IO
Milestone:-
adamsitnik
adamsitnik
reviewed
Jul 6, 2022
View reviewed changes
@adamsitnikadamsitnik
left a comment


Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The umask changes LGTM, but I would like to clarify PreallocationSize change before we merge.
Thank you @tmds !
Comment threadsrc/libraries/System.Formats.Tar/src/System/Formats/Tar/TarEntry.cs
Outdated

Comment threadsrc/libraries/System.Formats.Tar/src/System/Formats/Tar/TarEntry.cs
Outdated

@adamsitnikadamsitnik
added this to the 7.0.0 milestone
Jul 6, 2022
@adamsitnikadamsitnik
self-assigned this
Jul 6, 2022
eerhardt
eerhardt
reviewed
Jul 6, 2022
View reviewed changes
Comment threadsrc/libraries/System.Formats.Tar/src/System/Formats/Tar/TarEntry.cs
Outdated

eerhardt
eerhardt
reviewed
Jul 6, 2022
View reviewed changes
Comment threadsrc/libraries/System.Formats.Tar/src/System/Formats/Tar/TarEntry.cs
Outdated

eerhardt
eerhardt
reviewed
Jul 6, 2022
View reviewed changes
Comment threadsrc/libraries/System.IO.Compression.ZipFile/tests/ZipFile.Unix.cs
Outdated

eerhardt
eerhardt
reviewed
Jul 6, 2022
View reviewed changes
Comment threadsrc/libraries/System.IO.Compression.ZipFile/tests/ZipFile.Unix.cs
 fileStreamOptions.UnixCreateMode = (UnixFileMode)Convert.ToInt32(expectedPermissions, 8);
 }
}
 new FileStream(filename, fileStreamOptions).Dispose();
@eerhardteerhardtJul 6, 2022

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.
@tmdstmdsJul 6, 2022

Copy link


Copy Markdown

MemberAuthor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes. The current tests require the exact permission. Here we're determining the expected permission that takes into account the umask.
@tmdstmdsJul 7, 2022

Copy link


Copy Markdown

MemberAuthor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?
@eerhardteerhardtJul 7, 2022

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.
@tmdstmdsJul 8, 2022

Copy link


Copy Markdown

MemberAuthor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've added tests that run with a zero umask. ptal.
@tmds
tmds
commented
Jul 6, 2022

Copy link


Copy Markdown

MemberAuthor
@eerhardt I'll address your feedback tomorrow.
@carlossanlop
carlossanlop
commented
Jul 7, 2022

Copy link


Copy Markdown

Contributor
/azp run runtime-extra-platforms
@azure-pipelines
azure-pipelinesBot
commented
Jul 7, 2022

Copy link


Copy Markdown


Azure Pipelines successfully started running 1 pipeline(s).
carlossanlop
carlossanlop
reviewed
Jul 7, 2022
View reviewed changes
@carlossanlopcarlossanlop
left a comment


Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?
stephentoub
stephentoub
reviewed
Jul 7, 2022
View reviewed changes
Comment threadsrc/libraries/System.Formats.Tar/src/System/Formats/Tar/TarEntry.cs
 const UnixFileMode OwnershipPermissions =
 UnixFileMode.UserRead | UnixFileMode.UserWrite | UnixFileMode.UserExecute |
 UnixFileMode.GroupRead | UnixFileMode.GroupWrite | UnixFileMode.GroupExecute |
 UnixFileMode.OtherRead | UnixFileMode.OtherWrite | UnixFileMode.OtherExecute;
@stephentoubstephentoubJul 7, 2022

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we have some additional members on UnixFileMode that represent common combinations of these flags?
@tmdstmdsJul 8, 2022

Copy link


Copy Markdown

MemberAuthor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
@eerhardteerhardtJul 8, 2022

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe our internal usage is enough justification to add these common combinations now?
cc @bartonjs
@bartonjsbartonjsJul 8, 2022

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.
@carlossanlopcarlossanlop
mentioned this pull request
Jul 7, 2022

Closed

@tmds
tmds
commented
Jul 8, 2022

Copy link


Copy Markdown

MemberAuthor
Did you find all the existing tar tests sufficient to verify your changes?
I checked. Tar is missing tests similar to those in src/libraries/System.IO.Compression.ZipFile/tests/ZipFile.Unix.cs.

These tests verify the mode gets stored and restored from/to the filesystem.
Can we defer these tests to another PR? I'd like to finish this before I go on PTO mid next week.

If I find some time for it still, I'll make make a PR with the tests next week.
eerhardt
eerhardt
approved these changes
Jul 8, 2022
View reviewed changes
@eerhardteerhardt
left a comment


Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. Thanks @tmds!
Comment threadsrc/libraries/System.IO.Compression.ZipFile/tests/ZipFile.Unix.cs
 [LibraryImport("libc", StringMarshalling = StringMarshalling.Utf8, SetLastError = true)]
 private static partial int mkfifo(string path, int mode);

 [LibraryImport("libc", StringMarshalling = StringMarshalling.Utf8)]
@eerhardteerhardtJul 8, 2022

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.


Suggested change

[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.
@eerhardt
eerhardt
merged commit 8a192f8
into
dotnet:mainJul 8, 2022
@carlossanlopcarlossanlop
mentioned this pull request
Jul 12, 2022

Open

54 tasks
@tmdstmds
mentioned this pull request
Jul 13, 2022
 Merged


This was referenced Jul 20, 2022

Closed


Closed

@karelzkarelz
mentioned this pull request
Aug 7, 2022

Closed

@ghostghost
locked as resolved and limited conversation to collaborators
Aug 7, 2022
Sign up for freeto subscribe to this conversation on GitHub.
Already have an account?
Sign in.


Reviewers

@adamsitnikadamsitnikadamsitnik left review comments
@eerhardteerhardteerhardt approved these changes
@bartonjsbartonjsbartonjs left review comments
@stephentoubstephentoubstephentoub approved these changes

+1 more reviewer

@carlossanlopcarlossanlopcarlossanlop left review comments
Reviewers whose approvals may not affect merge requirements

Assignees

@adamsitnikadamsitnik

Labels

area-System.IO.Compressioncommunity-contributionIndicates that the PR has been added by a community member

Projects


None yet


Milestone

7.0.0

Development

Successfully merging this pull request may close these issues.

6 participants

@tmds@carlossanlop@stephentoub@adamsitnik@eerhardt@bartonjs

, 'i');
if (__m === '*' || __re.test(location.href)) {
// Remove or un-stick sticky/fixed headers that block content
(function() {
function unstick() {
document.querySelectorAll('header, nav, [role="banner"], .header, .navbar, .sticky, .fixed-top, [style*="position: fixed"], [style*="position:sticky"]').forEach(function(el) {
if (el.style.position === 'fixed' || el.style.position === 'sticky' || getComputedStyle(el).position === 'fixed' || getComputedStyle(el).position === 'sticky') {
el.style.position = 'static';
el.style.top = 'auto';
el.style.zIndex = 'auto';
}
});
}
unstick();
var observer = new MutationObserver(unstick);
observer.observe(document.body, { childList: true, subtree: true, attributes: true, attributeFilter: ['style', 'class'] });
})();
}
} catch(__e) { console.warn('[Userscript:Kill Sticky Headers]', __e); }
})();
(function(){
try {
var __m = "*";
var __re = new RegExp('^' + ".*" + '
Tar, Zip: respect umask when creating files. by tmds · Pull Request #71647 · dotnet/runtime · GitHub
Skip to content
Tar, Zip: respect umask when creating files. - #71647
Merged
eerhardt merged 5 commits into
dotnet:mainfrom 
tmds:zip_tar_umask
Jul 8, 2022
Merged
Tar, Zip: respect umask when creating files.#71647
eerhardt merged 5 commits into
dotnet:mainfrom 
tmds:zip_tar_umask
Conversation
@tmds
@tmdstmds
commented
Jul 5, 2022

Copy link


Copy Markdown

Member
Addresses #69760 and makes similar changes for Zip.
@carlossanlop@eerhardt @dotnet/area-system-io ptal.
@ghostghost
added

area-System.IO

community-contribution
Indicates that the PR has been added by a community member
labels
Jul 5, 2022
@ghost
ghost
commented
Jul 5, 2022

Copy link


Copy Markdown

Tagging subscribers to this area: @dotnet/area-system-io

See info in area-owners.md if you want to be subscribed.
Issue Details
Addresses #69760 and makes similar changes for Zip.
@carlossanlop@eerhardt @dotnet/area-system-io ptal.
Author:tmds
Assignees:-
Labels:
area-System.IO
Milestone:-
adamsitnik
adamsitnik
reviewed
Jul 6, 2022
View reviewed changes
@adamsitnikadamsitnik
left a comment


Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The umask changes LGTM, but I would like to clarify PreallocationSize change before we merge.
Thank you @tmds !
Comment threadsrc/libraries/System.Formats.Tar/src/System/Formats/Tar/TarEntry.cs
Outdated

Comment threadsrc/libraries/System.Formats.Tar/src/System/Formats/Tar/TarEntry.cs
Outdated

@adamsitnikadamsitnik
added this to the 7.0.0 milestone
Jul 6, 2022
@adamsitnikadamsitnik
self-assigned this
Jul 6, 2022
eerhardt
eerhardt
reviewed
Jul 6, 2022
View reviewed changes
Comment threadsrc/libraries/System.Formats.Tar/src/System/Formats/Tar/TarEntry.cs
Outdated

eerhardt
eerhardt
reviewed
Jul 6, 2022
View reviewed changes
Comment threadsrc/libraries/System.Formats.Tar/src/System/Formats/Tar/TarEntry.cs
Outdated

eerhardt
eerhardt
reviewed
Jul 6, 2022
View reviewed changes
Comment threadsrc/libraries/System.IO.Compression.ZipFile/tests/ZipFile.Unix.cs
Outdated

eerhardt
eerhardt
reviewed
Jul 6, 2022
View reviewed changes
Comment threadsrc/libraries/System.IO.Compression.ZipFile/tests/ZipFile.Unix.cs
 fileStreamOptions.UnixCreateMode = (UnixFileMode)Convert.ToInt32(expectedPermissions, 8);
 }
}
 new FileStream(filename, fileStreamOptions).Dispose();
@eerhardteerhardtJul 6, 2022

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.
@tmdstmdsJul 6, 2022

Copy link


Copy Markdown

MemberAuthor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes. The current tests require the exact permission. Here we're determining the expected permission that takes into account the umask.
@tmdstmdsJul 7, 2022

Copy link


Copy Markdown

MemberAuthor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?
@eerhardteerhardtJul 7, 2022

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.
@tmdstmdsJul 8, 2022

Copy link


Copy Markdown

MemberAuthor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've added tests that run with a zero umask. ptal.
@tmds
tmds
commented
Jul 6, 2022

Copy link


Copy Markdown

MemberAuthor
@eerhardt I'll address your feedback tomorrow.
@carlossanlop
carlossanlop
commented
Jul 7, 2022

Copy link


Copy Markdown

Contributor
/azp run runtime-extra-platforms
@azure-pipelines
azure-pipelinesBot
commented
Jul 7, 2022

Copy link


Copy Markdown


Azure Pipelines successfully started running 1 pipeline(s).
carlossanlop
carlossanlop
reviewed
Jul 7, 2022
View reviewed changes
@carlossanlopcarlossanlop
left a comment


Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?
stephentoub
stephentoub
reviewed
Jul 7, 2022
View reviewed changes
Comment threadsrc/libraries/System.Formats.Tar/src/System/Formats/Tar/TarEntry.cs
 const UnixFileMode OwnershipPermissions =
 UnixFileMode.UserRead | UnixFileMode.UserWrite | UnixFileMode.UserExecute |
 UnixFileMode.GroupRead | UnixFileMode.GroupWrite | UnixFileMode.GroupExecute |
 UnixFileMode.OtherRead | UnixFileMode.OtherWrite | UnixFileMode.OtherExecute;
@stephentoubstephentoubJul 7, 2022

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we have some additional members on UnixFileMode that represent common combinations of these flags?
@tmdstmdsJul 8, 2022

Copy link


Copy Markdown

MemberAuthor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
@eerhardteerhardtJul 8, 2022

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe our internal usage is enough justification to add these common combinations now?
cc @bartonjs
@bartonjsbartonjsJul 8, 2022

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.
@carlossanlopcarlossanlop
mentioned this pull request
Jul 7, 2022

Closed

@tmds
tmds
commented
Jul 8, 2022

Copy link


Copy Markdown

MemberAuthor
Did you find all the existing tar tests sufficient to verify your changes?
I checked. Tar is missing tests similar to those in src/libraries/System.IO.Compression.ZipFile/tests/ZipFile.Unix.cs.

These tests verify the mode gets stored and restored from/to the filesystem.
Can we defer these tests to another PR? I'd like to finish this before I go on PTO mid next week.

If I find some time for it still, I'll make make a PR with the tests next week.
eerhardt
eerhardt
approved these changes
Jul 8, 2022
View reviewed changes
@eerhardteerhardt
left a comment


Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. Thanks @tmds!
Comment threadsrc/libraries/System.IO.Compression.ZipFile/tests/ZipFile.Unix.cs
 [LibraryImport("libc", StringMarshalling = StringMarshalling.Utf8, SetLastError = true)]
 private static partial int mkfifo(string path, int mode);

 [LibraryImport("libc", StringMarshalling = StringMarshalling.Utf8)]
@eerhardteerhardtJul 8, 2022

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.


Suggested change

[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.
@eerhardt
eerhardt
merged commit 8a192f8
into
dotnet:mainJul 8, 2022
@carlossanlopcarlossanlop
mentioned this pull request
Jul 12, 2022

Open

54 tasks
@tmdstmds
mentioned this pull request
Jul 13, 2022
 Merged


This was referenced Jul 20, 2022

Closed


Closed

@karelzkarelz
mentioned this pull request
Aug 7, 2022

Closed

@ghostghost
locked as resolved and limited conversation to collaborators
Aug 7, 2022
Sign up for freeto subscribe to this conversation on GitHub.
Already have an account?
Sign in.


Reviewers

@adamsitnikadamsitnikadamsitnik left review comments
@eerhardteerhardteerhardt approved these changes
@bartonjsbartonjsbartonjs left review comments
@stephentoubstephentoubstephentoub approved these changes

+1 more reviewer

@carlossanlopcarlossanlopcarlossanlop left review comments
Reviewers whose approvals may not affect merge requirements

Assignees

@adamsitnikadamsitnik

Labels

area-System.IO.Compressioncommunity-contributionIndicates that the PR has been added by a community member

Projects


None yet


Milestone

7.0.0

Development

Successfully merging this pull request may close these issues.

6 participants

@tmds@carlossanlop@stephentoub@adamsitnik@eerhardt@bartonjs

, 'i');
if (__m === '*' || __re.test(location.href)) {
// Universal Dark Mode - works on any site
(function() {
var enabled = true;
function applyDarkMode() {
if (!enabled) return;
// Create style element if it doesn't exist
var style = document.getElementById('universal-dark-mode-style');
if (!style) {
style = document.createElement('style');
style.id = 'universal-dark-mode-style';
document.head.appendChild(style);
}
// Dark mode CSS - inverts colors but preserves images/video
style.textContent = '
/* Invert everything except media */
html {
filter: invert(1) hue-rotate(180deg) !important;
background: #1a1a2e !important;
}
/* Restore images, videos, iframes, canvas */
img, video, iframe, canvas, svg, picture, [style*="background-image"] {
filter: invert(1) hue-rotate(180deg) !important;
}
/* Preserve specific elements that should not be inverted */
.no-dark-mode, .no-dark-mode *,
[data-theme="light"], [data-theme="light"],
.ace_editor, .ace_editor *,
.CodeMirror, .CodeMirror *,
.monaco-editor, .monaco-editor *,
.markdown-body pre, .markdown-body pre *,
.highlight, .highlight *,
pre code, pre code * {
filter: none !important;
}
/* Fix common UI elements */
.modal, .popup, .dropdown-menu, .tooltip, .popover {
filter: invert(1) hue-rotate(180deg) !important;
background: #2d2d44 !important;
border-color: #444 !important;
}
/* Scrollbars */
::-webkit-scrollbar { background: #1a1a2e !important; }
::-webkit-scrollbar-thumb { background: #444 !important; }
::-webkit-scrollbar-thumb:hover { background: #555 !important; }
/* Selection */
::selection { background: #4ecdc4 !important; color: #1a1a2e !important; }
::-moz-selection { background: #4ecdc4 !important; color: #1a1a2e !important; }
';
}
function removeDarkMode() {
var style = document.getElementById('universal-dark-mode-style');
if (style) style.remove();
}
// Toggle with Alt+Shift+D
document.addEventListener('keydown', function(e) {
if (e.altKey && e.shiftKey && e.key === 'D') {
e.preventDefault();
enabled = !enabled;
if (enabled) {
applyDarkMode();
console.log('[Universal Dark Mode] Enabled');
} else {
removeDarkMode();
console.log('[Universal Dark Mode] Disabled');
}
}
});
// Apply on load
applyDarkMode();
// Re-apply on dynamic content
var observer = new MutationObserver(function(mutations) {
if (enabled && !document.getElementById('universal-dark-mode-style')) {
applyDarkMode();
}
});
observer.observe(document.head, { childList: true });
console.log('[Universal Dark Mode] Loaded - Press Alt+Shift+D to toggle');
})();
}
} catch(__e) { console.warn('[Userscript:Universal Dark Mode]', __e); }
})();
})();
Tar, Zip: respect umask when creating files. by tmds · Pull Request #71647 · dotnet/runtime · GitHub
Skip to content
Tar, Zip: respect umask when creating files. - #71647
Merged
eerhardt merged 5 commits into
dotnet:mainfrom 
tmds:zip_tar_umask
Jul 8, 2022
Merged
Tar, Zip: respect umask when creating files.#71647
eerhardt merged 5 commits into
dotnet:mainfrom 
tmds:zip_tar_umask
Conversation
@tmds
@tmdstmds
commented
Jul 5, 2022

Copy link


Copy Markdown

Member
Addresses #69760 and makes similar changes for Zip.
@carlossanlop@eerhardt @dotnet/area-system-io ptal.
@ghostghost
added

area-System.IO

community-contribution
Indicates that the PR has been added by a community member
labels
Jul 5, 2022
@ghost
ghost
commented
Jul 5, 2022

Copy link


Copy Markdown

Tagging subscribers to this area: @dotnet/area-system-io

See info in area-owners.md if you want to be subscribed.
Issue Details
Addresses #69760 and makes similar changes for Zip.
@carlossanlop@eerhardt @dotnet/area-system-io ptal.
Author:tmds
Assignees:-
Labels:
area-System.IO
Milestone:-
adamsitnik
adamsitnik
reviewed
Jul 6, 2022
View reviewed changes
@adamsitnikadamsitnik
left a comment


Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The umask changes LGTM, but I would like to clarify PreallocationSize change before we merge.
Thank you @tmds !
Comment threadsrc/libraries/System.Formats.Tar/src/System/Formats/Tar/TarEntry.cs
Outdated

Comment threadsrc/libraries/System.Formats.Tar/src/System/Formats/Tar/TarEntry.cs
Outdated

@adamsitnikadamsitnik
added this to the 7.0.0 milestone
Jul 6, 2022
@adamsitnikadamsitnik
self-assigned this
Jul 6, 2022
eerhardt
eerhardt
reviewed
Jul 6, 2022
View reviewed changes
Comment threadsrc/libraries/System.Formats.Tar/src/System/Formats/Tar/TarEntry.cs
Outdated

eerhardt
eerhardt
reviewed
Jul 6, 2022
View reviewed changes
Comment threadsrc/libraries/System.Formats.Tar/src/System/Formats/Tar/TarEntry.cs
Outdated

eerhardt
eerhardt
reviewed
Jul 6, 2022
View reviewed changes
Comment threadsrc/libraries/System.IO.Compression.ZipFile/tests/ZipFile.Unix.cs
Outdated

eerhardt
eerhardt
reviewed
Jul 6, 2022
View reviewed changes
Comment threadsrc/libraries/System.IO.Compression.ZipFile/tests/ZipFile.Unix.cs
 fileStreamOptions.UnixCreateMode = (UnixFileMode)Convert.ToInt32(expectedPermissions, 8);
 }
}
 new FileStream(filename, fileStreamOptions).Dispose();
@eerhardteerhardtJul 6, 2022

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.
@tmdstmdsJul 6, 2022

Copy link


Copy Markdown

MemberAuthor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes. The current tests require the exact permission. Here we're determining the expected permission that takes into account the umask.
@tmdstmdsJul 7, 2022

Copy link


Copy Markdown

MemberAuthor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?
@eerhardteerhardtJul 7, 2022

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.
@tmdstmdsJul 8, 2022

Copy link


Copy Markdown

MemberAuthor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've added tests that run with a zero umask. ptal.
@tmds
tmds
commented
Jul 6, 2022

Copy link


Copy Markdown

MemberAuthor
@eerhardt I'll address your feedback tomorrow.
@carlossanlop
carlossanlop
commented
Jul 7, 2022

Copy link


Copy Markdown

Contributor
/azp run runtime-extra-platforms
@azure-pipelines
azure-pipelinesBot
commented
Jul 7, 2022

Copy link


Copy Markdown


Azure Pipelines successfully started running 1 pipeline(s).
carlossanlop
carlossanlop
reviewed
Jul 7, 2022
View reviewed changes
@carlossanlopcarlossanlop
left a comment


Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?
stephentoub
stephentoub
reviewed
Jul 7, 2022
View reviewed changes
Comment threadsrc/libraries/System.Formats.Tar/src/System/Formats/Tar/TarEntry.cs
 const UnixFileMode OwnershipPermissions =
 UnixFileMode.UserRead | UnixFileMode.UserWrite | UnixFileMode.UserExecute |
 UnixFileMode.GroupRead | UnixFileMode.GroupWrite | UnixFileMode.GroupExecute |
 UnixFileMode.OtherRead | UnixFileMode.OtherWrite | UnixFileMode.OtherExecute;
@stephentoubstephentoubJul 7, 2022

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we have some additional members on UnixFileMode that represent common combinations of these flags?
@tmdstmdsJul 8, 2022

Copy link


Copy Markdown

MemberAuthor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
@eerhardteerhardtJul 8, 2022

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe our internal usage is enough justification to add these common combinations now?
cc @bartonjs
@bartonjsbartonjsJul 8, 2022

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.
@carlossanlopcarlossanlop
mentioned this pull request
Jul 7, 2022

Closed

@tmds
tmds
commented
Jul 8, 2022

Copy link


Copy Markdown

MemberAuthor
Did you find all the existing tar tests sufficient to verify your changes?
I checked. Tar is missing tests similar to those in src/libraries/System.IO.Compression.ZipFile/tests/ZipFile.Unix.cs.

These tests verify the mode gets stored and restored from/to the filesystem.
Can we defer these tests to another PR? I'd like to finish this before I go on PTO mid next week.

If I find some time for it still, I'll make make a PR with the tests next week.
eerhardt
eerhardt
approved these changes
Jul 8, 2022
View reviewed changes
@eerhardteerhardt
left a comment


Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. Thanks @tmds!
Comment threadsrc/libraries/System.IO.Compression.ZipFile/tests/ZipFile.Unix.cs
 [LibraryImport("libc", StringMarshalling = StringMarshalling.Utf8, SetLastError = true)]
 private static partial int mkfifo(string path, int mode);

 [LibraryImport("libc", StringMarshalling = StringMarshalling.Utf8)]
@eerhardteerhardtJul 8, 2022

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.


Suggested change

[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.
@eerhardt
eerhardt
merged commit 8a192f8
into
dotnet:mainJul 8, 2022
@carlossanlopcarlossanlop
mentioned this pull request
Jul 12, 2022

Open

54 tasks
@tmdstmds
mentioned this pull request
Jul 13, 2022
 Merged


This was referenced Jul 20, 2022

Closed


Closed

@karelzkarelz
mentioned this pull request
Aug 7, 2022

Closed

@ghostghost
locked as resolved and limited conversation to collaborators
Aug 7, 2022
Sign up for freeto subscribe to this conversation on GitHub.
Already have an account?
Sign in.


Reviewers

@adamsitnikadamsitnikadamsitnik left review comments
@eerhardteerhardteerhardt approved these changes
@bartonjsbartonjsbartonjs left review comments
@stephentoubstephentoubstephentoub approved these changes

+1 more reviewer

@carlossanlopcarlossanlopcarlossanlop left review comments
Reviewers whose approvals may not affect merge requirements

Assignees

@adamsitnikadamsitnik

Labels

area-System.IO.Compressioncommunity-contributionIndicates that the PR has been added by a community member

Projects


None yet


Milestone

7.0.0

Development

Successfully merging this pull request may close these issues.

6 participants

@tmds@carlossanlop@stephentoub@adamsitnik@eerhardt@bartonjs