[armhf] segfaults when trying to save a file

[dann frazier]

I reproduced by:

1) starting firefox on an armhf system
2) browsing to http://us.archive.ubuntu.com
3) right clicking on the "folder" image, clicking "Save Image As..."
4) then selecting a folder.

ubuntu@hb06-22:/tmp$ firefox

(process:23506): GLib-CRITICAL **: g_slice_set_config: assertion 'sys_page_size == 0' failed
Gtk-Message: Failed to load module "canberra-gtk-module"

(firefox:23506): LIBDBUSMENU-GLIB-WARNING **: Unable to get session bus: Failed to execute child process "dbus-launch" (No such file or directory)

### THIS IS WHERE I SELECT SAVE IMAGE AS ###

 1417625318797 GMPInstallManager.simpleCheckAndInstall INFO Last check was: 1417625319 seconds ago, minimum seconds: 86400
1417625318798 GMPInstallManager._getURL INFO Using url: https://aus4.mozilla.org/update/3/GMP/%VERSION%/%BUILD_ID%/%BUILD_TARGET%/%LOCALE%/%CHANNEL%/%OS_VERSION%/%DISTRIBUTION%/%DISTRIBUTION_VERSION%/update.xml
1417625318800 GMPInstallManager._getURL INFO Using url (with replacement): https://aus4.mozilla.org/update/3/GMP/33.0/20141013200607/Linux_arm-eabi-gcc3/en-US/release/Linux%203.13.0-37-generic%20(GTK%202.24.23)/canonical/1.0/update.xml
1417625318805 GMPInstallManager.checkForAddons INFO sending request to: https://aus4.mozilla.org/update/3/GMP/33.0/20141013200607/Linux_arm-eabi-gcc3/en-US/release/Linux%203.13.0-37-generic%20(GTK%202.24.23)/canonical/1.0/update.xml

(firefox:23506): Gtk-WARNING **: Attempting to store changes into `/home/ubuntu/.local/share/recently-used.xbel', but failed: Failed to create file '/home/ubuntu/.local/share/recently-used.xbel.ECJTPX': No such file or directory

(firefox:23506): Gtk-WARNING **: Attempting to set the permissions of `/home/ubuntu/.local/share/recently-used.xbel', but failed: No such file or directory
ubuntu@hb06-22:/tmp$

[Jacob Bramley (jacob-bramley)]

Created attachment 8469247
firefox-arm-xpcom.patch

This issue was reported on the Debian project: https://bugs.debian.org/cgi-bin/bugreport.cgi?bug=756426

I haven't reproduced this myself, and I don't have a suitable test environment set up at the moment. The attached patch was written and tested by a colleague (Steve Capper), with the following explanation:

==================================================================

NS_InvokeByIndex extracts arguments to an XPCOM method and places them
either in registers or on the stack as defined by the ARM calling
convention.

Unfortunately there is a bug when we have a 64-bit quantity passed
to the fourth argument, such as:

NS_IMETHODIMP
History::AddDownload(nsIURI* aSource, nsIURI* aReferrer,
                     PRTime aStartTime, nsIURI* aDestination)

The function expects arguments 0 (this), 1 (aSource) and 2 (aReferrer)
to be in r0, r1, r2 and arguments 3 (aStartTime) and 4 (aDestination)
to be on the stack.

Due to a counting bug in copy_dword, we get aDestination passed in
r3 rather than the stack, leading to data corruption and a crash.

This patch adjusts the logic in copy_dword s.t. any failed attempts
to fit a parameter in registers prevents further parameters being
placed in registers.

I have tested this patch on Iceweasel 30.0 (FireFox 30.0) on Jessie,
and it appears to be stable.

[Gbh (gbh)]

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

[Mrosenberg-b (mrosenberg-b)]

Comment on attachment 8469247
firefox-arm-xpcom.patch

Strange, my path for this source file is missing the src between xptcall and md.

[Steve Capper (steve-capper)]

Hi,
The discrepancy in directory structure of the patch can probably be accounted for in the following commit:
https://github.com/mozilla/gecko-dev/commit/92ff4be571ab04439d7fac24c60c0ddad793af01

Which I don't think has got through to the Debian package yet.

Cheers,
--
Steve

[Maz-o (maz-o)]

Any chance to get this progressed?

This patch fixes a glaring bug, which makes Firefox mostly unusable on armhf.
I've been using it now for about a month, without any issue.

Thanks,

Marc.

[Gbh (gbh)]

+1 for speeding up resolution of this bug. It is very frustrating to use firefox on my samsung
chromebook because of this bug.

[Gbh (gbh)]

Any update on this? Firefox on armhf is unusable without this patch.

[Gbh (gbh)]

Please fix this quickly. Lots of important features including downloads don't work in armhf without this fix.

[Till-3 (till-3)]

jbramley, is anything keeping you from landing this patch? Sounds like it'd be quite useful :)

[Jacob Bramley (jacob-bramley)]

(In reply to Till Schneidereit [:till] from comment #8)
> jbramley, is anything keeping you from landing this patch? Sounds like it'd
> be quite useful :)

Only that I haven't committed any Mozilla patches for _ages_ and I think the process has changed quite a bit. I assigned it to Marty thinking that he would be able to do it. (I was away for a week or two immediately after filing the patch.) Marty, could you commit this please, or delegate it to someone else?

[Dtc-moz (dtc-moz)]

Created attachment 8518718
Correct argument passing.

Rebase. Author is jbramley. Carry forward r+. Try run: https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=7f7225b4ad7f

[Ryanvm (ryanvm)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/96e78a235b14

[Ryanvm (ryanvm)]

https://hg.mozilla.org/mozilla-central/rev/96e78a235b14

[Dtc-moz (dtc-moz)]

Do you know if this should be uplifting to older Firefox versions? For example, does it affect ESR 31 and is Debian using ESR31?

[Maz-o (maz-o)]

(In reply to Douglas Crosher [:dougc] from comment #13)
> Do you know if this should be uplifting to older Firefox versions? For
> example, does it affect ESR 31 and is Debian using ESR31?

The bug definitely affects older versions, including ESR31, which is currently in the upcoming version of Debian (Jessie).

[Dtc-moz (dtc-moz)]

Created attachment 8523413
Correct argument passing - backport for ESR31

[Dtc-moz (dtc-moz)]

Requesting checkin if appropriate. The first patch applies to 34, and 35. The second patch is a backport to ESR31. This bug does not appear to affect Fennec so perhaps it is does not need approval?

[Ryanvm (ryanvm)]

Needs to get approval before it can land. Given that Fx34 had its first RC build spun yesterday, seems highly unlikely it's going to make it there at this point.

[Steve Capper (steve-capper)]

The symptoms of the bug include data corruption and crashes for ARM, and this can be reproduced quite reliably (downloading a file will quite often trigger it).

Could the somewhat severe nature of the bug and the self-contained nature of the fix (it only affects 32-bit ARM systems) please be taken into account?

Cheers,
--
Steve

[Maz-o (maz-o)]

Seconded. If this patch does not make it into the tree, there is hardly any point in even considering releasing Firefox on armhf.

It is just a waste of disk space and CPU time, and gives users a false sense of usability, which in the end is more damaging to Firefox than anything else.

Thanks,

Marc.

[Gbh (gbh)]

If this kind of bug was in the x86 version, this would have been fixed on day 1. Why this discrimination against us folks using armhf? This bug existed in firefox-armhf ever since i bought my chromebook more than a year ago. As Marc said above, there is no point even releasing firefox for armhf without fixing this bug.

Thanks,
Bharath

[Dtc-moz (dtc-moz)]

Comment on attachment 8518718
Correct argument passing.

Approval Request Comment
[Feature/regressing bug #]: Long standing issue
[User impact if declined]: Web browser crashes
[Describe test coverage new/current, TBPL]: Landed on 36. User confirms it fixes a crash on an affected system.
[Risks and why]: No risk for Mozilla distributions as it only touches code conditional on the ARM Hard-float calling conventions which are not yet exploited by Mozilla distributions.
[String/UUID change made/needed]:

[dann frazier (dannf)]

This was brought to my attention on the Linaro cross-distro list:
  http://lists.linaro.org/pipermail/cross-distro/2014-December/000765.html

[dann frazier (dannf)]

I've verified that the upstream patch applies cleanly to firefox in trusty, and that it does resolve the problem.

[Ryanvm (ryanvm)]

https://hg.mozilla.org/releases/mozilla-beta/rev/a41c48aa538d

[Maz-o (maz-o)]

"status-firefox-esr31: affected → wontfix"

Excellent. Given that Jessie is going to carry this version for the next few years, it effectively means that the only option to get a working, modern browser on the ARM architecture is to run:

apt-get install chromium

Thanks guys.

M.

[Continuation (continuation)]

Comment on attachment 8523413
Correct argument passing - backport for ESR31

ESR just needs a separate approval request.

[Approval Request Comment]
If this is not a sec:{high,crit} bug, please state case for ESR consideration: Crashes on Debian builds of Firefox.
User impact if declined: see previous
Fix Landed on Version: 36
Risk to taking this patch (and alternatives if risky): none. see comment 21
String or UUID changes made by this patch: none

[Continuation (continuation)]

I'm not sure why 31 was marked wontfix, so I'm going to reset it.

[jmp7 (jacobopantoja)]

Hi,

How does this bug affect Thunderbird? I have noticed that the same code is present (without the proper fix) in Thunderbird's XPCOM, but in Thunderbird I can download mails and files attached to them (Samsung Chromebook Snow).

Shall I open also a bug in Thunderbird? Or this code is fed back to Thunderbird in any way?

[Maz-o (maz-o)]

This entirely depends on the type and number of parameters that are passed.

Rule of thumb: as soon as a 64bit parameter appears in the call, it is likely that this code will do the wrong thing. Depending on the use of the parameters, effects could range from nothing to complete crash, with memory corruption in between.

Hint: It probably wouldn't be too hard to turn this bug into an attack vector.

But who really cares about security when such a bug is being ignored, despite having been identified four months ago, with a proper fix submitted for all of that time...

Cheers,

Marc.

[Continuation (continuation)]

(In reply to Jacobo Pantoja from comment #26)
> Or this code is fed back to Thunderbird in any way?

Yes, this will automatically be included in Thunderbird, once it lands on esr, I believe.

[Benjamin Kerensa (bkerensa)]

This does not appear to meet the landing criteria for ESR:
"Security and some major stability fixes when they're landed/merged onto mozilla-beta, or fixes for regressions specific to the ESR."

[Continuation (continuation)]

(In reply to Benjamin Kerensa [:bkerensa] from comment #29)
> This does not appear to meet the landing criteria for ESR:
> "Security and some major stability fixes when they're landed/merged onto
> mozilla-beta, or fixes for regressions specific to the ESR."

This patch fixes a crash that makes Firefox crash every time you save a file on the armhf architecture, which would to me qualify as a major stability fix. This patch has already landed landed on beta. What part of the landing criteria does it not meet?

[Maz-o (maz-o)]

(In reply to Benjamin Kerensa [:bkerensa] from comment #29)
> This does not appear to meet the landing criteria for ESR:
> "Security and some major stability fixes when they're landed/merged onto
> mozilla-beta, or fixes for regressions specific to the ESR."

Ah, I think I get your drift:
- "Security": evidently, using random arguments cannot ever lead to any form of security issue.
- "Stability": repeatable crashes for the most basic operations obviously don't qualify.
- "merged in beta": v35 and v36 are just a figment of my own imagination.

*blink*.

I'm slightly lost for words, and going back to reading Kafka. There is obviously more hope over there.

Marc.

[Benjamin Kerensa (bkerensa)]

(In reply to Andrew McCreight [:mccr8] from comment #30)
> (In reply to Benjamin Kerensa [:bkerensa] from comment #29)
> > This does not appear to meet the landing criteria for ESR:
> > "Security and some major stability fixes when they're landed/merged onto
> > mozilla-beta, or fixes for regressions specific to the ESR."
>
> This patch fixes a crash that makes Firefox crash every time you save a file
> on the armhf architecture, which would to me qualify as a major stability
> fix. This patch has already landed landed on beta. What part of the
> landing criteria does it not meet?

I can discuss this more with the team but one thing is we do not support Iceweasel which is the armf builds on Debian you are talking about?

[Mh+mozilla (mh+mozilla)]

Ubuntu also does Firefox builds on armhf.

[Mh+mozilla (mh+mozilla)]

And, in fact, Fedora does too.

[Jacob Bramley (jacob-bramley)]

Doesn't this affect Android too? They use armhf these days, I believe.

[Benjamin Kerensa (bkerensa)]

(In reply to Mike Hommey [:glandium] from comment #33)
> Ubuntu also does Firefox builds on armhf.

Mike,

Right but Ubuntu and Fedora's builds are not off ESR they are off mainline so they would get this patch through the normal release train (http://packages.ubuntu.com/trusty/firefox and https://apps.fedoraproject.org/packages/firefox)

[Benjamin Kerensa (bkerensa)]

(In reply to Jacob Bramley [:jbramley] from comment #35)
> Doesn't this affect Android too? They use armhf these days, I believe.

It does but we do not build an ESR for android the channels we do build for android will get this patch.

[jmp7 (jacobopantoja)]

IMHO, apply it to ESR31 as well, so that anyone, whoever it is, using ESR gets this corrected. Otherwise, ESR channel will remain unusable until next ESR version.

Note that this patch is fixing both a security hole and a huge usability caveat.

Regards,
JPantoja

[Benjamin Kerensa (bkerensa)]

Comment on attachment 8523413
Correct argument passing - backport for ESR31

Review of attachment 8523413:
-----------------------------------------------------------------

We will take this but are making an exception in doing so because this will not impact our supported releases because
this only affects armhf an unsupported platform.

[Ryanvm (ryanvm)]

(In reply to Benjamin Kerensa [:bkerensa] from comment #37)
> It does but we do not build an ESR for android the channels we do build for
> android will get this patch.

We are shipping Armv6 builds off esr31, FWIW (since support for them was dropped on the mainline branches). Not sure if they'd be affected by this bug, but FYI for any future approval requests you may come across.

[Ryanvm (ryanvm)]

https://hg.mozilla.org/releases/mozilla-esr31/rev/8db992f04318

[Mh+mozilla (mh+mozilla)]

(In reply to Jacob Bramley [:jbramley] from comment #35)
> Doesn't this affect Android too? They use armhf these days, I believe.

Android builds use softfp afaik.

[Bryan Quigley (bryanquigley)]

Fixed in Firefox 35 which should be in all releases long ago.