Closed
Bug 291378
Opened 19 years ago
Closed 19 years ago
crashes on XPCOM calls with more than 7 arguments
Categories
(Core :: XPCOM, defect)
Tracking
()
RESOLVED
FIXED
People
(Reporter: davidm, Assigned: davidm)
References
Details
(Keywords: crash)
Attachments
(1 file, 3 obsolete files)
108.54 KB,
text/plain
|
Details |
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.
Assignee | ||
Comment 1•19 years ago
|
||
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)
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?)
Assignee | ||
Comment 3•19 years ago
|
||
(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-
Assignee | ||
Comment 5•19 years ago
|
||
(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.
Assignee | ||
Comment 6•19 years ago
|
||
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 7•19 years ago
|
||
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+
Updated•19 years ago
|
Assignee: dougt → davidm
Status: UNCONFIRMED → NEW
Ever confirmed: true
Comment 9•19 years ago
|
||
Fix checked in. David, thank you for the patch!
Status: NEW → RESOLVED
Closed: 19 years ago
Resolution: --- → FIXED
Comment 10•19 years ago
|
||
*** Bug 276955 has been marked as a duplicate of this bug. ***
Comment 11•19 years ago
|
||
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 → ---
Comment 12•19 years ago
|
||
Assignee | ||
Comment 13•19 years ago
|
||
(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
Comment 14•19 years ago
|
||
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.
Comment 16•19 years ago
|
||
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)
Comment 17•19 years ago
|
||
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+
Comment 20•19 years ago
|
||
The patch was checked in by timeless (trunk).
Status: REOPENED → RESOLVED
Closed: 19 years ago → 19 years ago
Resolution: --- → FIXED
Updated•19 years ago
|
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
Updated•19 years ago
|
Attachment #182385 -
Attachment description: revised patch → revised patch
[Checked in: Comment 9]
Attachment #182385 -
Attachment is obsolete: true
Updated•19 years ago
|
Whiteboard: [checkin needed]
You need to log in
before you can comment on or make changes to this bug.
Description
•