1693462 - Avoid quarantine attribute being set when using nsILocalFile.copyTo* on macOS

Status: RESOLVED FIXED

(Core :: XPCOM, task)

Description

[Mike Hommey [:glandium]]

A bunch of unit tests are failing on experimental arm64 workers (bug 1693134) because they copy some executables around (for good reasons) before executing the copies, but the copies receive the quarantine attribute because of LSFileQuarantineEnabled being set at the Firefox level (https://searchfox.org/mozilla-central/rev/d504494eab038fd1bbc82eb4191bb334b4de2c8d/browser/app/macbuild/Contents/Info.plist.in#237).

I think a reasonable way to handle the situation to make the copyTo* methods of nsILocalFile avoid setting the quaranting attribute when the file that is being copied doesn't have it.

There are three ways I can see us solving this:

• calling getxattr(old_path, "com.apple.quarantine", nullptr, 0, 0 , 0) and if that returns ENOATTR, call removexattr(new_path, "com.apple.quarantine")
• similarly-ish, calling fgetxattr(...) and fremovexattr(...)`, but that requires getting into NSPR internals that are not exposed (which we kind of do on Windows for the append mode since bug 331453)
• use copyfile() instead of NSPR on mac.

Nika, what do you think?

Comment 1

[Mike Hommey [:glandium]]

There are variations around copyfile, where it can be used to just copy the ACL/extended attributes, but also to copy the data, and there's fcopyfile too, which does the same thing with fds, so the 2 first ways above could also use copyfile/fcopyfile. ETOOMANYCHOICES.

Comment 2

[Nika Layzell [:nika] (ni? for response)]

Given that apple actively maintains a copyfile library with their OS for this use-case, I think our best option for forward-compatibility is probably to switch nsLocalFile::CopyToNative to shell out to copyfile under the hood, rather than maintaining our own implementation on top of NSPR. I'm sure there are other xattrs which we're failing to copy over properly as well, and by using their method it's more likely stuff will continue to work in the future. I'm optimistic we won't run into too many compatibility issues if we switch over, but perhaps we can reconsider if we do.

Comment 3

[Joel Maher ( :jmaher ) (UTC -8) (PTO back normal Nov 17)]

I admit to not knowing much about nsLocalFile, but I did attempt to use OSX copyfile and this isn't working so well:
https://treeherder.mozilla.org/jobs?repo=try&revision=f93632f6d04c52b5b5aea9c3ad2c832ad6409655

I was hoping to hack something together as a proof of concept, I have tried a few flag combinations, but possibly I am failing there to do what is right.

happy to hack more if this is the right direction.

Comment 4

[Mike Hommey [:glandium]]

(In reply to Joel Maher ( :jmaher ) (UTC -0800) from comment #3)

I admit to not knowing much about nsLocalFile, but I did attempt to use OSX copyfile and this isn't working so well:
https://treeherder.mozilla.org/jobs?repo=try&revision=f93632f6d04c52b5b5aea9c3ad2c832ad6409655

I was hoping to hack something together as a proof of concept, I have tried a few flag combinations, but possibly I am failing there to do what is right.

happy to hack more if this is the right direction.

Your patch is using COPYFILE_MOVE, which is not what you'd want for a function that copies a file.

Comment 5

[Joel Maher ( :jmaher ) (UTC -8) (PTO back normal Nov 17)]

quick update, adjusting some flags AND identifying a directory then CopyDirectoryTo(), seems to solve most of the problems- I was testing on OSX 10.14 where things work, more data and I should know if I have something which could welcome more feedback.

I would think I should have two different methodsinstead of hacking inside the same method:

#if defined(XP_MACOSX)
nsLocalFile::CopyToNative(nsIFile* aNewParent, const nsACString& aNewName) {
  // copyfile()
}
#else
nsLocalFile::CopyToNative(nsIFile* aNewParent, const nsACString& aNewName) {
  // existing implementation
}
#endif

there are also some references to: #ifdef DEBUG_blizzard that appear to be 20+ years old, should those be removed?

Comment 6

[Joel Maher ( :jmaher ) (UTC -8) (PTO back normal Nov 17)]

ok, this doesn't work on the apple_silicon machines:
https://treeherder.mozilla.org/jobs?repo=try&selectedTaskRun=CS_MRbIMTDKb8DHTvrhQGA.0&revision=0b7dd4d8198a142d84c5d7c1abb9d698c4c7b445

the same aus related tests are still failing.

Comment 7

[Gian-Carlo Pascutto [:gcp]]

I'm guessing a bit here, but based on comment 2 talking about extended attributes, and seeing:

 copyfile_flags_t flags = COPYFILE_DATA;

versus

https://developer.apple.com/library/archive/documentation/System/Conceptual/ManPages_iPhoneOS/man3/copyfile.3.html

we might want COPYFILE_ALL instead?

Comment 8

[Gian-Carlo Pascutto [:gcp]]

Joel clarified on Slack that COPYFILE_ALL is what he tried first.

Comment 9

[Haik Aftandilian [:haik]]

So we expect this to mean that LSFileQuarantineEnabled overrides the copyfile arguments and we still end up with a quarantined file.

We could use getxattr/fgetxattr to log the file attributes before and after the copy operation to confirm if the quarantine attribute is still being set.

As an alternative to this more automatic approach, we could have test code remove the quarantined attribute before trying to execute the binary.

Comment 10

[Mike Hommey [:glandium]]

(In reply to Haik Aftandilian [:haik] from comment #9)

So we expect this to mean that LSFileQuarantineEnabled overrides the copyfile arguments and we still end up with a quarantined file.

We could use getxattr/fgetxattr to log the file attributes before and after the copy operation to confirm if the quarantine attribute is still being set.

I confirmed this is what happens.

As an alternative to this more automatic approach, we could have test code remove the quarantined attribute before trying to execute the binary.

I started doing that, and in face of the number there are, I filed this bug. I guess the simpler thing to do is the first proposal in comment 0.

Comment 11

[Mike Hommey [:glandium]]

Attachment: Bug 1693462 - Avoid quarantine attribute being set when using nsILocalFile.copyTo* on macOS.

Comment 12

[Mike Hommey [:glandium]]

Joel, can you give the attached patch a spin?

Comment 13

[Joel Maher ( :jmaher ) (UTC -8) (PTO back normal Nov 17)]

here is a try push:
https://treeherder.mozilla.org/jobs?repo=try&revision=5a51c1059f33b556ac7a6474b49a3378bb85e489&selectedTaskRun=KzYCxuvqQg6aTbpXHhJPkg.0

overall I think this is working, the errors I get are much different and it seems that shippable isn't required for the aus tests. here is a reference from a few weeks ago:
https://treeherder.mozilla.org/jobs?repo=try&searchStr=macosx1100-64%2Cxpcshell&revision=4eed037ddb35c7e316d418a0d4a459648e0e8d49

Comment 14

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/5aec36bc42a3
Avoid quarantine attribute being set when using nsILocalFile.copyTo* on macOS. r=nika

Comment 15

[Cristian Brindusan [:cbrindusan]]

https://hg.mozilla.org/mozilla-central/rev/5aec36bc42a3