Closed Bug 330890 Opened 19 years ago Closed 19 years ago

unify does not preserve the execute bit


(Firefox Build System :: General, defect)

Not set


(Not tracked)



(Reporter: mark, Assigned: mark)


(Keywords: fixed1.8.0.2, fixed1.8.1)


(1 file, 1 obsolete file)

When producing a universal binary, unify does not preserve the execute bit on non-Mach-O files that it creates.  It MUST.

This bug is causing un-executable scripts to be packaged for,, etc.
Flags: blocking1.8.0.2?
Severity: normal → critical
File::Copy::copy doesn't preserve the x-bit when it makes a copy to a new file like the "cp" command does.  This change makes unify set all of the x-bits allowed by the umask for a file that's being copied if any x-bit is set on either source file.  The structure of this change also strengthens unify against a possible race condition (which would have never occurred in our use case) and prevents it from deleting files it didn't create.
Attachment #215459 - Flags: review?(preed)
Don't worry about the permissive modes, it's got the same level of w-bits that we were already getting implicitly.  The tinderboxen run with umask 0, but we don't actually ship anything like that, the dmg packager (pkg-dmg) fixes up permissions in a private copy it makes of the files before putting anything into a dmg.
Comment is wrong:

+  # Wraps $FileAttrCache->lstat(), returning the type bits from the st_mode

should say "mode bits"
Tested successfully on maya-1.8.0-Uni.
Comment on attachment 215459 [details] [diff] [review]
Preserve the x-bit when copying files

Is there a reason you create an empty file over just chmod()ing (as appropriate) the file after you've File::copy()ed it?
Attached patch v2Splinter Review
There's a good reason:

Doing the open early with O_CREAT|O_EXCL guarantees that unify won't trample existing files.  This was the intended behavior from the very beginning, but I cheated for regular files and implemented the check as a stat call to see if the target file existed.  This is the race I talked about above and in the code section that I'm removing.

The only functional change in this version of the patch is that I've added the lIsExecutable() check in makeUniversalFile(), because I realized that it's possible to have Mach-O files that properly don't have any x-bits set: object and static archive files.  I also noticed that lipo sometimes ignores the umask, so I've added a late chmod($mode & ~umask()) for universal files too.  Now, entire trees created by unify should have the appropriate bits set and respect the umask.
Attachment #215459 - Attachment is obsolete: true
Attachment #215470 - Flags: review?
Attachment #215459 - Flags: review?(preed)
Attachment #215470 - Flags: review? → review?(preed)
Comment on attachment 215470 [details] [diff] [review]

I'm sold.

Ship it.
Attachment #215470 - Flags: review?(preed) → review+
Attachment #215470 - Flags: approval-branch-1.8.1+
Attachment #215470 - Flags: approval1.8.0.2?
Checked in on trunk.

Not checked in on 1.8 yet because a "slushy freeze" is in effect on that branch, and this is not critical there unless 2a1 will ship as a universal binary.

Awaiting approval1.8.0.2 for the 1.8.0 branch.
Closed: 19 years ago
Resolution: --- → FIXED
Comment on attachment 215470 [details] [diff] [review]

approved for, a=dveditz for drivers
Attachment #215470 - Flags: approval1.8.0.2? → approval1.8.0.2+
Flags: blocking1.8.0.2? → blocking1.8.0.2+
Fixed on 1_8_0.

preed, it's now in your hands to move the tag on unify or whatever.
Keywords: fixed1.8.0.2
*** Bug 331280 has been marked as a duplicate of this bug. ***
331280 is not a dup of this bug.
I landed this on 1_8 earlier today after talking with preed, in case y'all decide to push a universal 2a1.
Keywords: fixed1.8.1
Product: Core → Firefox Build System
You need to log in before you can comment on or make changes to this bug.