330890 - unify does not preserve the execute bit

Status: RESOLVED FIXED

(Firefox Build System :: General, defect)

Description

[Mark Mentovai]

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 run-mozilla.sh, redo-prebinding.sh, etc.

Comment 1

[Mark Mentovai]

Attachment: Preserve the x-bit when copying files

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.

Comment 2

[Mark Mentovai]

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 3

[Mark Mentovai]

Comment is wrong:

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

should say "mode bits"

Comment 4

[Mark Mentovai]

Tested successfully on maya-1.8.0-Uni.

Comment 5

[J. Paul Reed [:preed]]

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?

Comment 6

[Mark Mentovai]

Attachment: v2

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.

Comment 7

[J. Paul Reed [:preed]]

Comment on attachment 215470 [details] [diff] [review]
v2

I'm sold.

Ship it.

Comment 8

[Mark Mentovai]

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.

Comment 9

[Daniel Veditz [:dveditz]]

Comment on attachment 215470 [details] [diff] [review]
v2

approved for 1.8.0.2, a=dveditz for drivers

Comment 10

[Mark Mentovai]

Fixed on 1_8_0.

preed, it's now in your hands to move the tag on unify or whatever.

Comment 11

[Dave Liebreich [:davel]]

*** Bug 331280 has been marked as a duplicate of this bug. ***

Comment 12

[Dave Liebreich [:davel]]

331280 is not a dup of this bug.

Comment 13

[Mark Mentovai]

I landed this on 1_8 earlier today after talking with preed, in case y'all decide to push a universal 2a1.