1050258 - ARM hard-float: XPCOM incorrectly places some arguments. [debian #756426]

Status: RESOLVED FIXED

(Core :: XPCOM, defect)

Description

[Jacob Bramley [:jbramley]]

Attachment: 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.

Comment 2

[Marty Rosenberg [:mjrosenb]]

Comment on attachment 8469247 [details] [diff] [review]
firefox-arm-xpcom.patch

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

Comment 3

[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

Comment 4

[Marc Zyngier]

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.

Comment 5

[gbh]

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

Comment 6

[gbh]

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

Comment 7

[gbh]

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

Comment 8

[Till Schneidereit [:till]]

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

Comment 9

[Jacob Bramley [:jbramley]]

(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?

Comment 10

[Douglas Crosher [:dougc]]

Attachment: Correct argument passing.

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

Comment 11

[Ryan VanderMeulen [:RyanVM]]

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

Comment 12

[Ryan VanderMeulen [:RyanVM]]

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

Comment 13

[Douglas Crosher [:dougc]]

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

Comment 14

[Marc Zyngier]

(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).

Comment 15

[Douglas Crosher [:dougc]]

Attachment: Correct argument passing - backport for ESR31

Comment 16

[Douglas Crosher [:dougc]]

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?

Comment 17

[Ryan VanderMeulen [: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.

Comment 18

[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

Comment 19

[Marc Zyngier]

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.

Comment 20

[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

Comment 21

[Douglas Crosher [:dougc]]

Comment on attachment 8518718 [details] [diff] [review]
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]:

Comment 22

[Ryan VanderMeulen [:RyanVM]]

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

Comment 23

[Marc Zyngier]

"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.

Comment 24

[Andrew McCreight [:mccr8]]

Comment on attachment 8523413 [details] [diff] [review]
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

Comment 25

[Andrew McCreight [:mccr8]]

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

Comment 26

[Jacobo Pantoja]

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?

Comment 27

[Marc Zyngier]

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.

Comment 28

[Andrew McCreight [:mccr8]]

(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.

Comment 29

[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."

Comment 30

[Andrew McCreight [:mccr8]]

(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?

Comment 31

[Marc Zyngier]

(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.

Comment 32

[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?

Comment 33

[Mike Hommey [:glandium]]

Ubuntu also does Firefox builds on armhf.

Comment 34

[Mike Hommey [:glandium]]

And, in fact, Fedora does too.

Comment 35

[Jacob Bramley [:jbramley]]

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

Comment 36

[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)

Comment 37

[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.

Comment 38

[Jacobo Pantoja]

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

Comment 39

[Benjamin Kerensa [:bkerensa]]

Comment on attachment 8523413 [details] [diff] [review]
Correct argument passing - backport for ESR31

Review of attachment 8523413 [details] [diff] [review]:
-----------------------------------------------------------------

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.

Comment 40

[Ryan VanderMeulen [: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.

Comment 41

[Ryan VanderMeulen [:RyanVM]]

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

Comment 42

[Mike Hommey [:glandium]]

(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.