Trim file type bits from mode header by am11 · Pull Request #77151 · dotnet/runtime

am11 · 2022-10-18T06:09:23Z

~~Depends on dotnet/runtime-assets#279.~~

ghost · 2022-10-18T06:09:37Z

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

Issue Details

Author:	am11
Assignees:	-
Labels:	`area-System.IO`
Milestone:	-

tmds · 2022-10-18T06:29:52Z

Fixes #77107.

I think you meant to link to: #77096

am11 · 2022-10-18T06:31:45Z

I think you meant to link to: #77096

Yes, fixed. Too many tabs and PRs are opened. 😅

adamsitnik · 2022-10-18T07:34:26Z

Depends on dotnet/runtime-assets#279.

Merged ;)

tmds

lgtm, thanks!

adamsitnik

LGTM, thank you very much for your help @am11 !

…-tar

am11 · 2022-10-19T12:32:24Z

@danmoseley, can we backport to 7.0? (bugfix in new library)

danmoseley · 2022-10-19T13:16:18Z

@am11 I moved to the ASPNET team so it's not my call. Generally test coverage doesn't need any special approvals to back port though. 7.0 branch might be closed for a bit for GA.

danmoseley · 2022-10-19T13:21:03Z

Oh this is a product change. It would need all the regular process. The bar is roughly the servicing bar right now. It would be up to the folks in this repo though.

am11 · 2022-10-19T14:56:02Z

@danmoseley this is the fix for the issue #77096 in which you were coordinating with the author. Seeing other fixes being backported to 7.0, so there may still be some time.

danmoseley · 2022-10-19T15:09:13Z

Yup, but this is @adamsitnik / @jeffhandley area now.

The test failure is another hit on #77087

00 (Inline) -------- coreclr!WKS::gc_heap::mark_array_marked+0x1a [D:\a_work\1\s\src\coreclr\gc\gc.cpp @ 8471]
01 (Inline) -------- coreclr!WKS::gc_heap::background_mark1+0x1a [D:\a_work\1\s\src\coreclr\gc\gc.cpp @ 23743]
02 1273f8e0 738482f8 coreclr!WKS::gc_heap::background_mark_simple+0x1b [D:\a_work\1\s\src\coreclr\gc\gc.cpp @ 25142]
03 (Inline) -------- coreclr!WKS::gc_heap::background_mark_object+0x22 [D:\a_work\1\s\src\coreclr\gc\gc.cpp @ 25162]
04 1273f8e0 73847b01 coreclr!WKS::gc_heap::revisit_written_page+0x3c8 [D:\a_work\1\s\src\coreclr\gc\gc.cpp @ 35631]
05 1273f938 73885ffc coreclr!WKS::gc_heap::revisit_written_pages+0xf1 [D:\a_work\1\s\src\coreclr\gc\gc.cpp @ 35832]
06 1273f980 7383195b coreclr!WKS::gc_heap::background_mark_phase+0x1b2 [D:\a_work\1\s\src\coreclr\gc\gc.cpp @ 35093]
07 1273f9b8 738a0d4c coreclr!WKS::gc_heap::gc1+0x49e [D:\a_work\1\s\src\coreclr\gc\gc.cpp @ 21184]
08 1273f9d0 738a0cef coreclr!WKS::gc_heap::bgc_thread_function+0x5a [D:\a_work\1\s\src\coreclr\gc\gc.cpp @ 36283]
09 1273f9f8 738b8e50 coreclr!WKS::gc_heap::bgc_thread_stub+0x1f [D:\a_work\1\s\src\coreclr\gc\gc.cpp @ 34225]
0a 1273f9f8 75a46709 coreclr!<lambda_410a0441ee61a5b8ed4f18f53fb141bc>::operator()+0x70 [D:\a_work\1\s\src\coreclr\vm\gcenv.ee.cpp @ 1415]

jozkee

LGTM!

jozkee · 2022-10-20T04:29:03Z

 set
 {
- if ((int)value is < 0 or > 4095) // 4095 in decimal is 7777 in octal
+ if ((value & ~TarHelpers.ValidUnixFileModes) != 0) // throw on invalid UnixFileModes


nit: There's another reference to 4095 on the tests which we could change to a const UnixFileMode ValidUnixFileModes.
runtime/src/libraries/System.Formats.Tar/tests/TarWriter/TarWriter.File.Base.Unix.cs
Line 38 in ccd9d16
UnixFileModeexpectedMode=(UnixFileMode)(status.Mode&4095);// First 12 bits

Currently we don't have access to the helpers in the tests.

I was suggesting copying the const to the tests, not making the one on src accessible to tests. Is just a nit, so you can disregard it.

jozkee · 2022-10-20T04:30:45Z

+ // Some paths do not use the setter, and we want to return valid UnixFileMode.
+ // This mask only keeps the least significant 12 bits.


I wonder if not using the setter everywhere is correct. it seems somewhat inconsistent and we could've thrown earlier.

Good point, I am not sure if this is intentional. I didn't wanted to go for larger refactoring at this point. There is also an existing helper:
runtime/src/libraries/System.Formats.Tar/src/System/Formats/Tar/TarHelpers.Unix.cs
Line 55 in 88aea31
internalstaticvoidCreateDirectory(stringfullPath,UnixFileMode?mode,SortedDictionary<string,UnixFileMode>?pendingModes)
which takes care of permissions but for some reason we don't use it in the path OP was hitting the exception.

jozkee · 2022-10-20T04:33:19Z

 }

+ [Fact]
+ public async Task ExtractEntry_DockerImageTarWithFileTypeInDirectoriesInMode_SuccessfullyExtracts_Async()


I would also include a version of this test using a compressed archive since that's what the customer reported. Also, add the sync test. But this is up to you, not a big deal tbh.

That will require setting up token to fetch gzipped tar archive from docker api and posting a PR like dotnet/runtime-assets#279. I don't have access to those tokens (and don't think it is super important for 7.0 minimal fix).

jozkee · 2022-10-20T18:52:18Z

Thanks.

am11 · 2022-10-21T02:54:05Z

@jozkee, thanks. Can you please backport it to 7.0? This is matching the behavior of other tar reader implementations (tar xf docker-hello-world.tar on mac or linux works).

jeffhandley · 2022-10-28T19:04:05Z

@am11 I'm going to initiate the backport. This will put the fix into consideration for a servicing release--most of the other backports you've observed have also been for the first servicing release. Thanks for this fix!

jeffhandley · 2022-10-28T19:04:15Z

/backport to release/7.0

github-actions · 2022-10-28T19:04:27Z

Started backporting to release/7.0: https://github.com/dotnet/runtime/actions/runs/3347871693

github-actions · 2022-10-28T19:06:21Z

@jeffhandley backporting to release/7.0 failed, the patch most likely resulted in conflicts:

$ git am --3way --ignore-whitespace --keep-non-patch changes.patch
Applying: Trim file type bits from mode header
Applying: Define and use ValidUnixFileModes
Applying: Rev System.Formats.Tar.TestData version
Using index info to reconstruct a base tree...
M	eng/Version.Details.xml
M	eng/Versions.props
Falling back to patching base and 3-way merge...
Auto-merging eng/Versions.props
CONFLICT (content): Merge conflict in eng/Versions.props
Auto-merging eng/Version.Details.xml
CONFLICT (content): Merge conflict in eng/Version.Details.xml
error: Failed to merge in the changes.
hint: Use 'git am --show-current-patch=diff' to see the failed patch
Patch failed at 0003 Rev System.Formats.Tar.TestData version
When you have resolved this problem, run "git am --continue".
If you prefer to skip this patch, run "git am --skip" instead.
To restore the original branch and stop patching, run "git am --abort".
Error: The process '/usr/bin/git' failed with exit code 128

Please backport manually!

* Trim file type bits from mode header * Define and use ValidUnixFileModes * Rev System.Formats.Tar.TestData version * Move ValidUnixFileModes to shared helpers

* Trim file type bits from mode header (#77151) * Trim file type bits from mode header * Define and use ValidUnixFileModes * Rev System.Formats.Tar.TestData version * Move ValidUnixFileModes to shared helpers * Bump package version for TAR test assets Co-authored-by: Adeel Mujahid <3840695+am11@users.noreply.github.com> Co-authored-by: Carlos Sanchez <1175054+carlossanlop@users.noreply.github.com>

Trim file type bits from mode header
db5f423

ghost added area-System.IO community-contribution Indicates that the PR has been added by a community member labels Oct 18, 2022

am11 requested review from adamsitnik, jozkee and tmds October 18, 2022 06:10

tmds reviewed Oct 18, 2022
View reviewed changes

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

tmds reviewed Oct 18, 2022
View reviewed changes

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

Define and use ValidUnixFileModes
04bf3d0

Rev System.Formats.Tar.TestData version
cec85bf

am11 marked this pull request as ready for review October 18, 2022 10:03

Move ValidUnixFileModes to shared helpers
6af0213

tmds approved these changes Oct 18, 2022
View reviewed changes

build-analysisBot mentioned this pull request Oct 18, 2022
Assertion failed: (*card_word)==0 in DynamicGenerics tests #76801
Closed

adamsitnik approved these changes Oct 18, 2022
View reviewed changes

Merge branch 'main' into feature/libs/System.Formats.Tar/docker-image…
1e719fc
…-tar

jozkee approved these changes Oct 20, 2022
View reviewed changes

jozkee merged commit e242819 into dotnet:mainOct 20, 2022

am11 deleted the feature/libs/System.Formats.Tar/docker-image-tar branch October 21, 2022 02:54

jeffhandley pushed a commit that referenced this pull request Oct 28, 2022
Trim file type bits from mode header (#77151)
1d400c2
* Trim file type bits from mode header * Define and use ValidUnixFileModes * Rev System.Formats.Tar.TestData version * Move ValidUnixFileModes to shared helpers

jeffhandley mentioned this pull request Oct 28, 2022
[release/7.0] TAR: Trim file type bits from mode header #77612
Merged

jeffhandley added area-System.Formats.Tar and removed area-System.IO labels Nov 21, 2022

ghost locked as resolved and limited conversation to collaborators Dec 22, 2022

		// Some paths do not use the setter, and we want to return valid UnixFileMode.
		// This mask only keeps the least significant 12 bits.

Uh oh!

Conversation

am11 commented Oct 18, 2022• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ghost commented Oct 18, 2022

Uh oh!

Uh oh!

Uh oh!

tmds commented Oct 18, 2022

Uh oh!

am11 commented Oct 18, 2022

Uh oh!

adamsitnik commented Oct 18, 2022

Uh oh!

tmds left a comment

Choose a reason for hiding this comment

Uh oh!

adamsitnik left a comment

Choose a reason for hiding this comment

Uh oh!

am11 commented Oct 19, 2022

Uh oh!

danmoseley commented Oct 19, 2022• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

danmoseley commented Oct 19, 2022

Uh oh!

am11 commented Oct 19, 2022

Uh oh!

danmoseley commented Oct 19, 2022• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jozkee left a comment

Choose a reason for hiding this comment

Uh oh!

jozkeeOct 20, 2022

Choose a reason for hiding this comment

Uh oh!

am11Oct 20, 2022

Choose a reason for hiding this comment

Uh oh!

jozkeeOct 20, 2022

Choose a reason for hiding this comment

Uh oh!

jozkeeOct 20, 2022

Choose a reason for hiding this comment

Uh oh!

am11Oct 20, 2022• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jozkeeOct 20, 2022

Choose a reason for hiding this comment

Uh oh!

am11Oct 20, 2022

Choose a reason for hiding this comment

Uh oh!

jozkee commented Oct 20, 2022

Uh oh!

am11 commented Oct 21, 2022

Uh oh!

jeffhandley commented Oct 28, 2022

Uh oh!

jeffhandley commented Oct 28, 2022

Uh oh!

github-actionsBot commented Oct 28, 2022

Uh oh!

github-actionsBot commented Oct 28, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

am11 commented Oct 18, 2022•
edited
Loading

danmoseley commented Oct 19, 2022•
edited
Loading

danmoseley commented Oct 19, 2022•
edited
Loading

am11Oct 20, 2022•
edited
Loading