Closed Bug 1050258 Opened 5 years ago Closed 5 years ago

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

Categories

(Core :: XPCOM, defect, critical)

32 Branch
ARM
Linux
defect
Not set
critical

Tracking

()

RESOLVED FIXED
mozilla36
Tracking Status
firefox34 --- wontfix
firefox35 --- fixed
firefox36 --- fixed
firefox-esr31 35+ fixed

People

(Reporter: jbramley, Assigned: mjrosenb)

References

Details

Attachments

(2 files, 1 obsolete file)

Attached patch firefox-arm-xpcom.patch (obsolete) — Splinter Review
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.
Attachment #8469247 - Flags: review?(mrosenberg)
Assignee: nobody → mrosenberg
Duplicate of this bug: 1049746
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.
Attachment #8469247 - Flags: review?(mrosenberg) → review+
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
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.
+1 for speeding up resolution of this bug. It is very frustrating to use firefox on my samsung
chromebook because of this bug.
Any update on this? Firefox on armhf is unusable without this patch.
Please fix this quickly. Lots of important features including downloads don't work in armhf without this fix.
jbramley, is anything keeping you from landing this patch? Sounds like it'd be quite useful :)
Flags: needinfo?(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?
Flags: needinfo?(Jacob.Bramley)
Rebase. Author is jbramley. Carry forward r+. Try run: https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=7f7225b4ad7f
Attachment #8469247 - Attachment is obsolete: true
Attachment #8518718 - Flags: review+
https://hg.mozilla.org/mozilla-central/rev/96e78a235b14
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla36
Do you know if this should be uplifting to older Firefox versions? For example, does it affect ESR 31 and is Debian using ESR31?
Flags: needinfo?(gbh)
(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).
Flags: needinfo?(gbh)
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?
Keywords: checkin-needed
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.
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
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.
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 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]:
Attachment #8518718 - Flags: approval-mozilla-beta?
Attachment #8518718 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
"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 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
Attachment #8523413 - Flags: approval-mozilla-esr31?
I'm not sure why 31 was marked wontfix, so I'm going to reset it.
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?
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.
(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.
Attachment #8523413 - Flags: approval-mozilla-esr31? → approval-mozilla-esr31-
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."
(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?
Flags: needinfo?(bkerensa)
(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.
(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?
Flags: needinfo?(bkerensa)
Ubuntu also does Firefox builds on armhf.
And, in fact, Fedora does too.
Doesn't this affect Android too? They use armhf these days, I believe.
(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)
(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.
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 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.
Attachment #8523413 - Flags: approval-mozilla-esr31- → approval-mozilla-esr31+
(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.
(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.
You need to log in before you can comment on or make changes to this bug.