535090 - partial update mar creation doesn't preserve +x bit on new binaries (auto-update from 2009-12-14 to 2009-12-15 m-c nightly creates new file "mozilla-runtime" as non-executable)

Status: RESOLVED FIXED

(Release Engineering :: General, defect)

Description

[Daniel Holbert [:dholbert]]

STEPS TO REPRODUCE:
 1. Download & extract the mozilla-central nightly build from 2009-12-14 [1]
 2. Run the build in a fresh profile, and do Help | Check for Updates
 3. Download the update when prompted, and restart Firefox to allow it to install
 4. Examine the permissions on the new file "mozilla-runtime"

ACTUAL RESULTS:
 mozilla-runtime is *not* executable

EXPECTED RESULTS:
 mozilla-runtime should be executable, as it is in a 2009-12-15 build.[2]


[1] http://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/2009/12/2009-12-14-03-mozilla-central/firefox-3.7a1pre.en-US.linux-i686.tar.bz2

[2] http://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/2009/12/2009-12-15-03-mozilla-central/firefox-3.7a1pre.en-US.linux-i686.tar.bz2

This issue is one cause of the hang in bug 535077, since exec() fails for non-executable files.

Comment 1

[Daniel Holbert [:dholbert]]

Note: I'm assuming this is a bug in our update code path (either on the server side, generating the updates, or on the client side, receiving and applying the updates).

Not sure what the correct component is for it.  CCing ted and rob_strong, in the hopes that one of them might know.

Comment 2

[Robert Strong (they/them - no direct email)]

Is it non-executable in the tar.bz2 as well?

Comment 3

[Daniel Holbert [:dholbert]]

In link [2] from comment 0, it's executable.

In other words, a freshly-obtained mozilla-central nightly has it as executable.  But an old nightly, updated via "Help | Check for Updates", gets a non-executable copy.  (They're identical in every other way, though -- "diff" says they're the same)

Comment 4

[Robert Strong (they/them - no direct email)]

So, link [1] does not have the executable bit set?

Complete updates and new files should retain the permissions set when packaged whereas partials retain the existing permission. I believe I have this covered for both Mac and Linux using xpcshell tests and the updater binary. So, if the first update containing that file didn't have the executable bit set the updater wouldn't set it. Not sure if this is in fact what happened but if it did one way to fix this is to have releng force a complete copy of the binary in the next update or only offer a complete update.

Comment 5

[Robert Strong (they/them - no direct email)]

(In reply to comment #4)
> So, link [1] does not have the executable bit set?
Sorry, I thought that build had the mozilla-runtime as well.

Comment 6

[Daniel Holbert [:dholbert]]

Sorry, I should have been clearer in original description. "mozilla-runtime" is a new file added for out-of-process plugins.  It first appears in the 2009-12-15 nightly.

Comment 7

[Robert Strong (they/them - no direct email)]

nah, I knew that but didn't notice the date on link 1.

cc'ing nthomas and bsmedberg if for no other reason than to give them a heads up.

btw: update xpcshell test_0110_general.js tests permissions on file replacement (new files with 0755 and 0644 that replace existing files with different perms) which I believe is the same for new files and update xpcshell test_0110_general.js tests adding a new file with 0644.

I would also expect to see this happening on Mac OS X if it were update code.

Comment 8

[Nick Thomas [:nthomas] (UTC+12)]

http://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/2009/12/2009-12-15-03-mozilla-central/firefox-3.7a1pre.en-US.linux-i686.complete.mar gives mode 755 for mozilla-runtime

http://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/2009/12/2009-12-14-03-mozilla-central/firefox-3.7a1pre.en-US.linux-i686.partial.20091214030830-20091215030948.mar gives mode 644.

So it's a bug in the partial generation. In fact, I'll assert the problem is that bzip2 doesn't preserve the permissions at 
 http://hg.mozilla.org/mozilla-central/file/tip/tools/update-packaging/make_incremental_update.sh#l168
In place compression would preserve, redirecting like that doesn't. So all new files will get the 644 mode, and in the same partial libmozsqlite3.so is also affected.

Comment 9

[Nick Thomas [:nthomas] (UTC+12)]

This is probably a suitable hack to fit into the style of this file
  chmod `stat -c %a "$newdir/$f"` "$workdir/$f"
The stat arguments may not be very portable though.

Comment 10

[Daniel Holbert [:dholbert]]

IMHO this bug needs to block 1.9.3 (or whatever next release includes OOP plugins).  But there aren't any flags that I can use to request that right now...

Comment 11

[Benjamin Smedberg]

It blocks bug 531142, so you're covered.

Comment 12

[Justin Dolske [:Dolske]]

I'll see if the fix suggested in comment 8 is all that's needed here.

Comment 13

[Chris AtLee [:catlee]]

test -x "$newdir/$f" && chmod +x "$workdir/$f"

might work if all we care about is the executable bit

chmod --reference="$newdir/$f" "$workdir/$f"

might also work

Comment 14

[John O'Duinn [:joduinn] (please use "needinfo?" flag)]

(In reply to comment #8)
> So it's a bug in the partial generation. In fact, I'll assert the problem is
> that bzip2 doesn't preserve the permissions at 
> http://hg.mozilla.org/mozilla-central/file/tip/tools/update-packaging/make_incremental_update.sh#l168
bsmedberg reports that this is blocking OOP work in bug#531142. Urgh - there
are only 3 changes to that file during 2009 - how long ago exactly did that
break, or has this always been a problem? 


> In place compression would preserve, redirecting like that doesn't. So all new
> files will get the 644 mode, and in the same partial libmozsqlite3.so is also
> affected.

bsmedberg/dholbert: if you do a new install m-c nightly from *after*
2009-12-15, is this still a recurring problem, or is there something specific
to the 2009-12-15 date?

Comment 15

[Daniel Holbert [:dholbert]]

(In reply to comment #14)
> bsmedberg/dholbert: if you do a new install m-c nightly from *after*
> 2009-12-15, is this still a recurring problem, or is there something specific
> to the 2009-12-15 date?

joduinn: see comment 3.  The significance of 2009-12-15 is: that's the nightly where this file ("mozilla-runtime") first appears.  Auto-updates from before that to after that will create this file as non-executable, and that's the problem here.

A fresh install after that date will create the file just fine (executable), as noted in comment 3.

Comment 16

[Benjamin Smedberg]

It's specific to a partial update, so the nightly from 2009-12-14 to 2009-12-15. But it will reproduce when we do the first beta and the first release and have partials for those.

Comment 17

[Nick Thomas [:nthomas] (UTC+12)]

We use different code for releases, eg
 http://hg.mozilla.org/releases/mozilla-1.9.2/file/UPDATE_PACKAGING_R10/tools/update-packaging/make_incremental_updates.py
For new files, create_add_patch_for_file() calls to copy_file(), which shutil.copy2 to duplicate the (compressed) file from the complete mar. Since that preserves the metadata I don't think we'll hit this issue for releases.

This bug affects new executables in nightly builds only.

Comment 18

[Nick Thomas [:nthomas] (UTC+12)]

This does not block bug 535077.

Comment 19

[Daniel Holbert [:dholbert]]

Ok -- marking that bug as "see also", then, since that bug is one reason this bug matters.

Comment 20

[Nochum Sossonko [:Natch]]

(In reply to comment #18 & #19)

I removed it from the "See Also" because this bugzilla installation's bugs don't really belong there per the description of that field. It's meant for other bugzilla installations, this one's belong in the "Depends" or "Blocks" field...

Comment 21

[Benjamin Smedberg]

ping? We're coming up on an alpha/beta with possible updates next week, I'd like to get this landed and baked somehow.

Comment 22

[Nick Thomas [:nthomas] (UTC+12)]

Attachment: Fix nightly partial generation

Using this patch I can create a partial between 2009-12-14/15 nightlies that preserves the 755 permission on mozilla-runtime and libmozsqlite3.so. There are no differences in mode or file content between the 14th build updated with that partial and the 15th build.

Looks like more work than you'd really need to do, except that stat isn't available in MozillaBuild, and linux & mac differ sufficiently to make it a pain to use them; calling out to perl/python defeats the point of doing this in shell originally.

Comment 23

[Justin Dolske [:Dolske]]

Bah, I'm too slow. :)

FWIW, bug 529464 will be changing the updater to always treat the .mar files as the canonical source for file permissions. But AIUI the only problem right now is that the mars are getting the wrong permissions for ADD items in the manifest?

Comment 24

[Nick Thomas [:nthomas] (UTC+12)]

(In reply to comment #17)
> ... I don't think we'll hit this issue for releases.

Confirmed by calling make_incremental_updates.py against the mars for the 14th/15th, applying the mar and comparing files and modes.

(In reply to comment #23)
> FWIW, bug 529464 will be changing the updater to always treat the .mar files as
> the canonical source for file permissions. 

Currently the patch files within the mar are 644, so that might cause some interesting issues.

> But AIUI the only problem right now
> is that the mars are getting the wrong permissions for ADD items in the
> manifest?

As filed it's when binary files are added to the package. Forcing a file is also an ADD in the manifest, or if the patch ends up larger than the file (small files + bz2 header). And sure enough there are other instances of the code pattern I'm changing, will update the patch.

Comment 25

[Nick Thomas [:nthomas] (UTC+12)]

Attachment: Fix nightly partial generation, v2

Covers the other two cases as well.

Comment 26

[Chris Cooper [:coop] (he/him)]

Comment on attachment 422945 [details] [diff] [review]
Fix nightly partial generation, v2

Worthwhile putting this into a function, since it is used in 3 places?

Also, does the python variant of this script have the same issue?

Comment 27

[Nick Thomas [:nthomas] (UTC+12)]

Attachment: Fix nightly partial generation, v3

Apologies for the churn here but I noticed that when we create a complete mar (for both releases & nightlies) we call make_full_update.sh, which uses copy_perm() in common.sh. That sets mode 755 or 644 in mar depending on the executable bit of the source file. I think we should use the the same mechanism for nightly partial creation too, just to keep the handling the same.

The release case (make_incremental_updates.py) is verified unaffected at comment #24.

Comment 28

[Daniel Holbert [:dholbert]]

Looks like IPC plugins just switched to default-enabled...
   http://hg.mozilla.org/mozilla-central/rev/f54bb3222492
...which means tonight's nightly update will expose a whole swath of nightly testers to this bug, unless the patch here lands ASAP.

nthomas, can you check this in and make sure it gets used in generating tonight's nightly update?  (if you haven't already)

Comment 29

[Robert Strong (they/them - no direct email)]

I suspect that the nightly might also need to do a force add of the new file since we preserve existing permissions for partial updates or only offer complete update to fix this for all nightly users.

Comment 30

[Benjamin Smedberg]

Since this was added I hope/expect that most nightly users have taken a day off somewhere, which would give them a full update instead of a partial, and we've done multiple nightlies on a couple days. I really don't think many people are going to see this on the nightly channel now.

Comment 31

[Robert Strong (they/them - no direct email)]

I agree... just pointing out the possibility and that a simple fix would be to offer only a complete update for the one nightly.

Comment 32

[Nick Thomas [:nthomas] (UTC+12)]

I'll get it landed today. 

Only offering a complete isn't straightforward, but we could we just force mozilla-runtime in the partial. Also libmozsqlite3.so although +x doesn't seem to be required, so it would just be for consistency.

Comment 33

[Nick Thomas [:nthomas] (UTC+12)]

Attachment: As landed in CVS

Checking in make_incremental_update.sh;
/cvsroot/mozilla/tools/update-packaging/make_incremental_update.sh,v  <--  make_incremental_update.sh
new revision: 1.16; previous revision: 1.15
done

Updated prometheus-vm and staging-nightly-updates (including the README at /builds/nightly-partial-generation/mozilla-checkout). I'll back out the change to requested_forced_updates tomorrow.

Landings on the branches to follow.

Comment 34

[Nick Thomas [:nthomas] (UTC+12)]

Comment on attachment 423901 [details] [diff] [review]
As landed in CVS

>Index: make_incremental_update.sh
>-requested_forced_updates='components/components.list Contents/MacOS/components/components.list'
>+requested_forced_updates='components/components.list Contents/MacOS/components/components.list mozilla-runtime libmozsqlite3.so'

Reverted this change in rev 1.17, and partial generators updated.

Comment 35

[Nick Thomas [:nthomas] (UTC+12)]

Comment on attachment 423287 [details] [diff] [review]
Fix nightly partial generation, v3

Checked into active branches:
http://hg.mozilla.org/releases/mozilla-1.9.1/rev/0555a0269dca
http://hg.mozilla.org/releases/mozilla-1.9.2/rev/ce136686a20a

m-c is closed for the Alpha1 freeze.

Comment 36

[Nick Thomas [:nthomas] (UTC+12)]

Comment on attachment 423287 [details] [diff] [review]
Fix nightly partial generation, v3

http://hg.mozilla.org/mozilla-central/rev/d72639947a60

Comment 37

[Mike Shaver (:shaver emeritus)]

Did this get a test added in the partial-update test setup?  I don't see it in the patches, but I don't really know what I'm looking for tbqh.

We're looking at a change to the naming of the executable, and just want to make sure that we don't trip over old bugs again when we do so.

Comment 38

[Robert Strong (they/them - no direct email)]

Not sure about the tests for mar creation but there are xpcshell tests that verify that the client side preserves the permissions provided by the mar.