Closed Bug 291378 Opened 19 years ago Closed 19 years ago

crashes on XPCOM calls with more than 7 arguments

Categories

(Core :: XPCOM, defect)

Other
Linux
defect
Not set
critical

Tracking

()

RESOLVED FIXED

People

(Reporter: davidm, Assigned: davidm)

References

Details

(Keywords: crash)

Attachments

(1 file, 3 obsolete files)

User-Agent:       Mozilla/5.0 (X11; U; Linux ia64; en-US; rv:1.7.6) Gecko/20050418 Firefox/1.0.2 (Debian package 1.0.2-1)
Build Identifier: Mozilla/5.0 (X11; U; Linux ia64; en-US; rv:1.7.6) Gecko/20050418 Firefox/1.0.2 (Debian package 1.0.2-1)

Firefox may crash on ia64 linux on various occasions such as when trying to "Get
More Extensions", "Get More Themes", or when installing a Debian firefox locale
package.  The root cause of these crashes is all the same: when using XPCOM to
invoke a function with more than 7 arguments, XPCOM may end up passing garbage
in arguments 8 and so on.  If one of those arguments happens to be a pointer, a
crash is the likely result.

The problem is due to the fact that xptcstubs_asm_ipf{32,64}.s:SharedStub() has
hardcoded the frame-size of its caller.  However, depending on the compiler and
optimization-level in use, that frame-size may vary and if the hard-coded
constant doesn't match the actual frame-size, the above problem will trigger. 
This happens reliably when using GCC v3.3.

For reference, see these Debian bugreports:

  http://bugs.debian.org/cgi-bin/bugreport.cgi?bug=303515
  http://bugs.debian.org/cgi-bin/bugreport.cgi?bug=304389

Note: do not use the patches in the Debian bugreports.  Those are OK for Debian,
but they do break the HP-UX/IPF support.  I'll attach a proposed patch that
should work for both Linux and HP-UX on IPF shortly.


Reproducible: Always

Steps to Reproduce:
1. Start up mozilla-firefox
2. Click on "Tools -> Extensions -> Get More Extensions"

Actual Results:  
Firefox crashes.

Expected Results:  
Firefox should open up a new window for the addons.update.mozilla.org web site.
Attached patch proposed patch (obsolete) — Splinter Review
This patch fixes the problem by adding an argument to SharedStub().  The new
argument is a pointer to the first in-memory argument of the stub.  The patch
works fine on Linux and should work for HP-UX, but I still need to confirm
that.
Attachment #181471 - Flags: review?(shaver)
Severity: normal → critical
Keywords: crash
Comment on attachment 181471 [details] [diff] [review]
proposed patch

So can you explain to me why we're using PRUint64 for a pointer type (the last
new argument)?	Also: why don't we need to update any other of the
xpcstubsdecl.inc "callers" for this case?

(And does it work on HPUX after all?)
(In reply to comment #2)
> (From update of attachment 181471 [details] [diff] [review] [edit])
> So can you explain to me why we're using PRUint64 for a pointer type (the last
> new argument)?

I suppose it doesn't really matter.  We could just as well use "void *".

> Also: why don't we need to update any other of the
> xpcstubsdecl.inc "callers" for this case?

Probably just my unfamiliarity with the mozilla source code.  Where can I find
those other callers?  Do you mean other platform-specific code?

> (And does it work on HPUX after all?)

I haven't heard back yet.  Let me check.
Comment on attachment 181471 [details] [diff] [review]
proposed patch

Yeah, the other platform-specific code.  If we're now accepting a 7th argument
as a pointer to additional arguments, I would expect the other xptcstubs code
to require updating as well.

http://lxr.mozilla.org/mozilla/source/xpcom/reflect/xptcall/src/md/unix/xptcstu
bs_asm_ipf32.s#17 is one such place, I haven't done a complete census of the
rest of the code yet.  Some of the impls, like the gcc_x86_unixish stuff, seem
to just go off their knowledge of the ABI and paramCount.

As I think of it more now, though, they might just need to have trivial changes
made to their STUB_ENTRY implementations, if they provide explicit mention of
the C++ arguments.  (gcc_x86_unixish doesn't, AFAICT, because it just defines
the stubs in terms of an assembly thunk to sharedstub.)

Why is methodIndex passed before the remaining input arguments?  Is that
required by a quirk of the ABI?  It seems like the extra-arg-indirection would
make more sense between the inline args and methodIndex.  I now understand the
non-pointer nature of a8, though, so you don't need to change it; it's only a
pointer when passed to SharedStub, where it's typed correctly.

Also, and importantly, you should be modifying the genstubs.pl code, and not
the generated file (as per the comment at the top of the generated file), so
that we don't lose the changes when someone regenerates it.

(Sorry if there are bad edits here; I hate this tiny little comment-on-the-bug
box.)
Attachment #181471 - Flags: review?(shaver) → review-
(In reply to comment #4)

> Yeah, the other platform-specific code.  If we're now accepting a 7th argument
> as a pointer to additional arguments, I would expect the other xptcstubs code
> to require updating as well.

The portion of xptcstubsdecl.inc which the patch modifies is already #ifdef'd on:

  defined(ia64) && (defined(hpux) || defined(linux))

so as long as the IPF/ia64 versions of mozilla work for hp-ux and linux, we
should be fine.  Or am I missing something?

> Why is methodIndex passed before the remaining input arguments?

I don't know.

> Is that required by a quirk of the ABI?

I doubt it.

> It seems like the extra-arg-indirection would
> make more sense between the inline args and methodIndex.

Perhaps.  The patch is trying to be minimal so as to reduce the chance of
introducing unintended side-effects.

> I now understand the
> non-pointer nature of a8, though, so you don't need to change it; it's only a
> pointer when passed to SharedStub, where it's typed correctly.

Yes.

> Also, and importantly, you should be modifying the genstubs.pl code, and not
> the generated file (as per the comment at the top of the generated file), so
> that we don't lose the changes when someone regenerates it.

Oops, I had missed that.  I'll see about creating an updated patch which fixes
this issue.
This patch changes genstubs.pl rather than xptcstubsdecl.inc.
Please remember to rerun "perl genstubs.pl" after applying the patch.
This patch has been tested on ia64 Linux and works fine there.
Attachment #181471 - Attachment is obsolete: true
Attachment #182385 - Flags: review?
Comment on attachment 182385 [details] [diff] [review]
revised patch
[Checked in: Comment 9]

David, you want to request review from a specific person..

shaver, picking on you, since you've looked at this once already.
Attachment #182385 - Flags: review? → review?(shaver)
Comment on attachment 182385 [details] [diff] [review]
revised patch
[Checked in: Comment 9]

r+sr=shaver, and approving for 1.8b2 as portability fix, if there's still time
to make it.
Attachment #182385 - Flags: superreview+
Attachment #182385 - Flags: review?(shaver)
Attachment #182385 - Flags: review+
Attachment #182385 - Flags: approval1.8b2+
Assignee: dougt → davidm
Status: UNCONFIRMED → NEW
Ever confirmed: true
Fix checked in.  David, thank you for the patch!
Status: NEW → RESOLVED
Closed: 19 years ago
Resolution: --- → FIXED
*** Bug 276955 has been marked as a duplicate of this bug. ***
I suspect that this patch breaks my IA64 build of xulrunner of trunk.
I will attach the interesting part of my build.log
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
(In reply to comment #11)
> I suspect that this patch breaks my IA64 build of xulrunner of trunk.
> I will attach the interesting part of my build.log

This looks to me like genstubs.pl didn't run.  Can you verify that
xptcstubsdecl.inc did get regenerated after the patch was applied?

Thanks,

  --david
Thanks David. 
As I understand genstubs.pl is not intended to run at compile time but only
manually. I think the resulting change was not checked in along with your changes.
Wolfgang: can you rerun genstubs.pl and post a diff here for quick review and
approval?  I'll watch for it today.
Please note that this patch is not tested yet.
I will start some builds on different platforms to test it.
Attachment #186198 - Flags: review?(shaver)
I can confirm that the patch works for me and I've built e.g. xulrunner on 
i386, x86-64, s390, s390x and IA64 successfully.
Comment on attachment 186198 [details] [diff] [review]
xptcstubsdecl.inc after running genstubs.pl
[Checked in: Comment 20]

r+a=shaver, thanks.
Attachment #186198 - Flags: review?(shaver)
Attachment #186198 - Flags: review+
Attachment #186198 - Flags: approval1.8b3+
The last patch needs to be checked in.
Whiteboard: [checkin needed]
The patch was checked in by timeless (trunk).
Status: REOPENED → RESOLVED
Closed: 19 years ago19 years ago
Resolution: --- → FIXED
Attachment #186198 - Attachment description: xptcstubsdecl.inc after running genstubs.pl → xptcstubsdecl.inc after running genstubs.pl [Checked in: Comment 20]
Attachment #186198 - Attachment is obsolete: true
Attachment #182385 - Attachment description: revised patch → revised patch [Checked in: Comment 9]
Attachment #182385 - Attachment is obsolete: true
Whiteboard: [checkin needed]
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: