292021 - Software Update self-contained executable

Status: RESOLVED FIXED

(Toolkit Graveyard :: Security, defect)

Description

[Benjamin Smedberg]

This bug is for the self-contained executable for the Firefox 1.1 update plan.
It needs to be small, support binary patching, etc.

http://wiki.mozilla.org/Software_Update
http://wiki.mozilla.org/Firefox:1.1_Software_Update_Upgrades

I am attaching what I've got here. It builds and runs on mac. The binary
patching code is untested, because I (or someone) needs to update "bsdiff" to
add the checksum and remove bzip2, and make everything 32bit. But it's a start,
and it's small enough to make me happy.

Comment 1

[Benjamin Smedberg]

Attachment: Self-container updater, draft

The updater/ bits of this patch build on mac, and appear to basically work.
The other-licenses/bsdiff parts do not build.

The first priority here is to get bsdiff working with the same header/32bit
types as bspatch. Since I'm going to be having a new baby tomorrow, and this is
fairly urgent, I welcome assistance with that task. If you do start on
something, please comment to let me know, so that we don't duplicate activity.

Comment 2

[Darin Fisher]

FYI, I'm working on an revised version of this patch while bsmedberg is on
vacation.  For the most part, I'm very happy with the current patch.

I'm adding the following pieces:

 * Recover better from errors from Execute steps:
   1)  Add Finish step to each Action after all Execute steps have run.
   2)  Make backup copies of all files before removing any files.
   3)  Restore backups in Finish step if any Execute step failed.

 * Implement UI.  For this task, my plan is as follows:
   1)  Move realmain onto a background thread.
   2)  Record state of realmain in some global progress variables.
   3)  Protect global progress variables in a mutex.
   4)  Implement native progress UI that polls progress variables.

I also had to remove the submanifest action because I don't have an
implementation of fork, though I could probably spawn a thread for that given
the architecture described above.  I'm just not sure that there is any need for
a submanifest action at this point.

Comment 3

[Darin Fisher]

>    1)  Move realmain onto a background thread.

Actually, nevermind that.  I'm going start off by avoiding any multithreading. 
That's probably overkill for this application.  We should have the opportunity
to update the UI often enough such that this sort of thing would be unnecessary.

Another TODO item is adding code to the updater to make it wait for its parent
process to exit.  That way we can be sure that firefox.exe is not running when
we start patching it.  I'm thinking of passing the process ID of the parent as a
command line flag to the updater.

Comment 4

[Benjamin Smedberg]

> Another TODO item is adding code to the updater to make it wait for its parent
> process to exit.  That way we can be sure that firefox.exe is not running when

For *nix/windows this is not necessary... we can just execv Firefox into the
update executable. On mac this will require more testing, and perhaps the
parent-waiting code you describe.

Comment 5

[Darin Fisher]

> For *nix/windows this is not necessary... we can just execv Firefox into the
> update executable. On mac this will require more testing, and perhaps the
> parent-waiting code you describe.

Ah, yeah.. that does make sense.  By the way, I have windows progress UI
working.  I did end up putting realmain on a background thread for simplicity --
yeah, it made things simpler.  The resulting updater.exe is less than 20k in an
optimized build.

Comment 6

[Darin Fisher]

Attachment: Revised patch

OK, here's a revised version of the updater.  It has UI for Windows and GTK2. 
I've only verified the ADD function.  I verified that I was able to implement
"cp -r" using this tool ;-)

Remaining work:

1) Support re-launching firefox with command line flag

2) Add OSX progress UI

3) Test patching and file removal

4) Make the progress increments make sense.  Right now, I slice it into thirds:
1/3 for Prepare, 1/3 for Execute, and 1/3 for Finish, but Execute dominates in
terms of time, so the progress meter progresses in a funny way.

Comment 7

[Darin Fisher]

> For *nix/windows this is not necessary... we can just execv Firefox into the
> update executable. On mac this will require more testing, and perhaps the
> parent-waiting code you describe.

So, I think that MS's execv might not work the way we think or hope that it
does.  It appears to call CreateProcess and then call _exit(0) on the process
that called execv.  Therefore, I think there would be a race between the new
process's WinMain and the call to _exit(0), which I presume is what would
release "firefox.exe" to allow it to be overwritten.  It may be difficult to
lose this race in practice, but it might bite us if we aren't careful.  Under
UNIX, it would be nice to use execv because it allows people running firefox-bin
to wait on the process and have that correctly correspond to waiting for firefox
"the application" to close.  But, that is not really needed on Windows, so if we
need to do the --wait-pid=PID thing on Windows, then so be it.

Comment 8

[Benjamin Smedberg]

Agreed. By the way, I plan to examine why we had to switch from execv to the
MacLaunchHelper code in the first place... I think that switching back to execv
on the mac (with the appropriate problems fixed) could fix a number of annoying
mac bugs (the disappearing/reapparing bouncy globe, among others).

Comment 9

[Darin Fisher]

Attachment: Revised patch (probably broken on Linux)

This patch is the latest from my tree.	Not sure when bsmedberg may be back to
start thinking about this stuff again, so I wanted to get my latest patch up
here.

I've revised the updater UI to only appear if the update is taking a while to
process.  In other words, for very short updates, we won't show any UI at this
level.	The UI will be shown by the update service if needed.

I've used this code to apply a complete firefox ZIP archive into a directory.

This probably only compiles on Windows.  I need to spend some time on Linux and
OSX making things happy there.

Comment 10

[Benjamin Smedberg]

This looks decent to me. I'm interested in getting something checked in on the
software update branch soon, so that I can play. We also talked about not
extracting the ZIP files, and letting this executable extract them as it uses
them: this means linking against standalone libjar, but that's not a big hit.

Comment 11

[Darin Fisher]

Yeah, I'm close to having the zipreader stuff incorporated.  Not bad so far at
least.  I'll land this on the branch once I have that in shape.

Comment 12

[Darin Fisher]

Attachment: v1 patch

This verison of the updater works on Windows, Linux, and Mac OSX.  Mac OSX is
missing platform specific progress meter UI.  Josh Aas is working on that.  For
now, we just perform the update completely silently on Mac OSX (and any other
platform for that matter).

This patch depends on libmar and libbz2.

Comment 13

[Benjamin Smedberg]

Comment on attachment 185092 [details] [diff] [review]
v1 patch

1) in the GTK AutoFILE impl, you need to null-check fp_ in the destructor:
fclose(0) will crash on some OSes (BSD).

2) in main(), the usage statement should not be #ifdef DEBUG

3) why don't we wait-pid on mac also? We can't use execv there because of the
threading problems so I think we have to use waitpid.

4) Do we have a localization story for updater.ini? It is hardcoding "Firefox".
I think this should be in browser/locales and mail/locales, perhaps with a
hardcoded generic English text if the ini file is missing.

Comment 14

[Darin Fisher]

> 1) in the GTK AutoFILE impl, you need to null-check fp_ in the destructor:
> fclose(0) will crash on some OSes (BSD).

ok


> 2) in main(), the usage statement should not be #ifdef DEBUG

ok... i didn't see any reason to output it in optimized builds.  but, i guess
someone might want to run it from the command line.


> 3) why don't we wait-pid on mac also? We can't use execv there because of the
> threading problems so I think we have to use waitpid.

ok, sounds reasonable.


> 4) Do we have a localization story for updater.ini?

I figured localizing this file would be similar to how we localize the INI files
for the installers.  Why is that not okay?

Comment 15

[Benjamin Smedberg]

We localize the installer.ini files from a makefile in browser/locales (which
calls into toolkit/locales). I really think we should ship this file from that
directory.

Comment 16

[Darin Fisher]

bsmedberg: can you please outline what needs to be done exactly to localize
updater.ini?  i sort of get what's going on, but it looks like i need to do some
preprocessing steps and call iconv as well.  what's the recommended approach
exactly?  can you help with this task?

Comment 17

[Darin Fisher]

> 3) why don't we wait-pid on mac also? We can't use execv there because of the
> threading problems so I think we have to use waitpid.

what threading problem?  clue me in :)

Comment 18

[Benjamin Smedberg]

What charset is the ini file? it looks like utf8 on gtk2 and the "platform"
charset on windows. I'm willing to ignore that issue to get this landed, but
we're going to need to fix it.

I suggest that we just put the ini file in browser/locales/en-US/update.ini and
ship it with the same rule we use for README.txt for the time being.

The reason we use the MacLaunhHelper on mac is because execv fails once the mac
windowing system has been initialized: execv apparently doesn't work on mac if
there are non-main threads still running, and the windowing system uses a
background thread to process events.

Comment 19

[Darin Fisher]

bsmedberg: sounds good.  i will make those changes.

Comment 20

[Darin Fisher]

On the topic of charsets, you are right.  If we assume that localizers are
creating updater.ini using UTF-8, then we need to run it through iconv to
convert to the platform charset before shipping it with a localized Windows
build of Firefox.  I guess that means using the same system that we are using
for installer.inc currently.

Comment 21

[Benjamin Smedberg]

Right now iconv is only a build-time requirement for the installer: we will need
to update the build docs and make the proper noise on the newsgroups and blogs
that iconv is now a build-time requirement for anyone building toolkit apps.

Comment 22

[Christian :Biesinger (don't email me, ping me on IRC)]

it would be great if we could use unicode throughout these apps and use MSLU...

Comment 23

[Darin Fisher]

Perhaps I should create an --enable-updater option to configure.  That will
probably be appreciated by folks who either use Mozilla on a platform where the
updater doesn't build or by folks who simply don't want to build the updater
(hello Minimo!).

Comment 24

[Darin Fisher]

Attachment: v2 patch

OK, this version implements --enable-updater.  It moves updater.ini into
browser/locales/en-US, and adds a rule to browser/locale/Makefile.in to run
updater.ini through iconv.  It only does this if MOZ_UPDATER is defined.

This patch also contains changes to client.mk, Makefile.in, and allmakefiles.sh

to build the various dependencies of the updater system: libmar, libbz2, and
bsdiff.

Comment 25

[Darin Fisher]

Attachment: v2.1 patch

The last patch was missing configure.in changes.

FYI, this entire patch is checked in on the SOFTWARE_UPDATE_20050428_BRANCH.

Comment 26

[Benjamin Smedberg]

Comment on attachment 185541 [details] [diff] [review]
v2.1 patch

In configure.in, you need to set a default value for MOZ_UPDATER: I think it
should be on-by-default for MOZ_XUL_APP.

Be careful not to checkin the client.mk changes.

In Makefile.in, it should be "Cross-compiling the mbsdiff executable is not yet
supported." But, I don't think we should compile it by default at all: it's a
tainted license, and should require an opt-in build rule.

Comment 27

[Darin Fisher]

Attachment: v2.2 patch

OK, revised patch.

Comment 28

[Benjamin Smedberg]

Comment on attachment 185623 [details] [diff] [review]
v2.2 patch

Marking approvals based on previous discussions.

Comment 29

[Darin Fisher]

I've modified updater/Makefile.in to only build progressui_gtk.cpp when
MOZ_ENABLE_GTK2 is defined, because that code makes use of several GTK2 specific
functions, including the new and much saner progressbar API.  If someone wants
to port it to GTK1 they can.  For now, GTK1 builds will just use progressui_null.

Comment 30

[Darin Fisher]

fixed-on-trunk

Comment 31

[jayfromtaiwan]

I'm getting this compilation error (with VS8 Beta2)


updater.cpp
c:/mozilla/trunk/mozilla/toolkit/mozapps/update/src/updater/updater.cpp(240) : e
rror C2440: 'initializing' : cannot convert from 'const char *' to 'char *'
        Conversion loses qualifiers
c:/mozilla/trunk/mozilla/toolkit/mozapps/update/src/updater/updater.cpp(465) : e
rror C2440: 'initializing' : cannot convert from 'const char *' to 'char *'
        Conversion loses qualifiers
make[7]: *** [updater.obj] Error 2

Comment 32

[Darin Fisher]

I checked in a fix for the VC8 Beta 2 build bustage.

Comment 33

[Stephen Walker]

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

Comment 34

[cls]

(In reply to comment #21)
> Right now iconv is only a build-time requirement for the installer: we will need
> to update the build docs and make the proper noise on the newsgroups and blogs
> that iconv is now a build-time requirement for anyone building toolkit apps.

I ran into this today on win32 as well.  You should add a configure check for
this since it's a new mandatory requirement.

Comment 35

[Darin Fisher]

cls: --disable-updater should bypass the iconv requirement.

Comment 36

[Darin Fisher]

Oh,... you're saying that we should make configure complain if --disable-updater
is not specified and iconv cannot be found.  That does sound like a good idea.

Comment 37

[tqh]

Breaks BeOS-builds as it doesn't have implementation.

Comment 38

[tqh]

Attachment: Fixes BeOS bustage.

Comment 39

[Darin Fisher]

Please do not reopen this bug for platform specific work like this.  File a new
bug.  Unsupported platforms should be compiled with --disable-updater.

Comment 40

[tqh]

Sorry, there was no way of knowing that. (It might be good to note things like
that along with the error that stops the build. Saves everyone a lot of trouble.)

Comment 41

[Darin Fisher]

Yeah, sorry about that.  I'm trying to move quickly, and as a result
communication about --disable-updater hasn't been so good.