System.IO.Compression.Tests.ZipFile_Unix.UnixCreateSetsPermissionsInExternalAttributes fail on OSX #68293

Activity

1. ghost added

   area-System.IO.Compression

   on Apr 20, 2022
2. ghost added

   untriagedNew issue has not been triaged by the area ownerNew issue has not been triaged by the area owner

   on Apr 20, 2022
4. danmoseley commented on Apr 20, 2022

   

   danmoseley

   on Apr 20, 2022

   Contributor

   More actions

   The earliest build listed above was 4/20/2022, 11:00:15 AM. I checked in my zip bug fix (not that I can see how it was related) at ~12 PST.
5. elinor-fung commented on Apr 20, 2022

   

   elinor-fung

   on Apr 20, 2022

   Member

   More actions

   Expected: 3968 -> 7600, which is S_ISUID | S_ISGID | S_ISVTX
   Actual: 2944 -> 5600, which is S_ISUID | S_ISVTX

   Lots of hits today. Marking as blocking-clean-ci.
6. elinor-fung

   added

   blocking-clean-ciBlocking PR or rolling runs of 'runtime' or 'runtime-extra-platforms'Blocking PR or rolling runs of 'runtime' or 'runtime-extra-platforms'

   on Apr 20, 2022
7. danmoseley commented on Apr 20, 2022

   

   danmoseley

   on Apr 20, 2022

   Contributor

   More actions

   @dotnet/dnceng did anything change in the OSX 12 image?

   @eerhardt you recall this code -- any idea?

   this line may be relevant, but it suggests that we should expect these bits to be cleared?
   

   runtime/src/libraries/System.IO.Compression.ZipFile/src/System/IO/Compression/ZipFileExtensions.ZipArchiveEntry.Extract.Unix.cs

   Line 11 in c78bf2f

   // S_ISUID, S_ISGID, and S_ISVTX bits. This matches unzip's default behavior.
8. MattGal commented on Apr 20, 2022

   

   MattGal

   on Apr 20, 2022

   Member

   More actions

   @dotnet/dnceng did anything change in the OSX 12 image?

   @eerhardt you recall this code -- any idea?

   this line may be relevant, but it suggests that we should expect these bits to be cleared?

   runtime/src/libraries/System.IO.Compression.ZipFile/src/System/IO/Compression/ZipFileExtensions.ZipArchiveEntry.Extract.Unix.cs

   Line 11 in c78bf2f

   // S_ISUID, S_ISGID, and S_ISVTX bits. This matches unzip's default behavior.

   Hi Dan! Can you comment on time? The only interesting thing to change here was moving from 12.0 to 12.3 a while back because the GA 12.0 kept timing out. That wasn't particularly recently though, so your time range would be useful to know.
9. MattGal commented on Apr 20, 2022

   

   MattGal

   on Apr 20, 2022

   Member

   More actions

   @danmoseley I confirmed the update (12.0 -> 12.3 latest-at-the-time) was on 3/29, about 3 weeks ago. Beyond this, these machines have done nothing but execute test work items.
10. danmoseley commented on Apr 21, 2022

    

    danmoseley

    on Apr 21, 2022

    Contributor

    More actions

    @eerhardt thoughts? does this pass on your mac hardware (I assume it's 12.3 or you can update it)
11. danmoseley commented on Apr 21, 2022

    

    danmoseley

    on Apr 21, 2022

    Contributor

    More actions

    oh, he's out til Monday. I'll disable the test meantime (I should have done this already)
12. danmoseley

    added

    disabled-testThe test is disabled in source code against the issueThe test is disabled in source code against the issue

    and removed

    blocking-clean-ciBlocking PR or rolling runs of 'runtime' or 'runtime-extra-platforms'Blocking PR or rolling runs of 'runtime' or 'runtime-extra-platforms'

    on Apr 22, 2022
13. danmoseley

    assigned

    eerhardt

    on Apr 22, 2022
14. jozkee

    added this to the 7.0.0 milestone on May 3, 2022
15. jozkee

    removed

    untriagedNew issue has not been triaged by the area ownerNew issue has not been triaged by the area owner

    on May 3, 2022
16. carlossanlop commented on May 4, 2022

    

    carlossanlop

    on May 4, 2022

    Last edited by carlossanlop

    Contributor

    More actions

    Also affecting the release/6.0 branch. I'm disabling the tests there too: #68867

    When this issue gets fixed, the fix needs to get backported to release/6.0.
17. ghost added

    in-prThere is an active PR which will close this issue when it is mergedThere is an active PR which will close this issue when it is merged

    on May 19, 2022
18. eerhardt commented on May 19, 2022

    

    eerhardt

    on May 19, 2022

    Member

    More actions

    I finally got back to this. I've created #69583 to fix this issue.

    The problem, as far as I can tell, is that the macOS devices used in CI must have changed which group they run under, or somehow the user configuration on the machines changed. The tests were failing because they expected the S_ISGID bit to be set on the files, but that bit wasn't be set. Reading https://linux.die.net/man/3/chmod, it says:

    If the calling process does not have appropriate privileges, and if the group ID of the file does not match the effective group ID or one of the supplementary group IDs and if the file is a regular file, bit S_ISGID (set-group-ID on execution) in the file's mode shall be cleared upon successful return from chmod().

    So, what was happening here is that the test was calling chmod with S_ISGID being set, but the OS was clearing the bit. To fix this I changed the test to read the file mode after calling chmod and using that as the expected file mode. On "normal dev" unix environments (including my local macOS 12.3 machine), the bit isn't being cleared, and the tests pass successfully without modification. But to allow CI to work, the tests will now handle this case.
19. carlossanlop commented on May 20, 2022

    

    carlossanlop

    on May 20, 2022

    Last edited by carlossanlop

    Contributor

    More actions

    Thanks for the investigation, @eerhardt. Coincidentally, I still have to fix a bug in the tar tests related to chmod to make sure it is working properly, and after the fix I need to make sure they don't hit the same failure.
20. jozkee

    closed this as completedin #69583on May 20, 2022
21. jozkee

    added a commit that references this issue on May 20, 2022

    ZipFile UnixCreateSetsPermissionsInExternalAttributes test fails on A…

    ...

    This issue will close once commit 6069680 is merged into the 'main' branch.

    6069680
22. ghost removed

    in-prThere is an active PR which will close this issue when it is mergedThere is an active PR which will close this issue when it is merged

    on May 20, 2022
23. ghost locked as resolved and limited conversation to collaborators on Jun 19, 2022