Closed Bug 689916 Opened 8 years ago Closed 8 years ago

Firefox 7/Thunderbird 7 + AdBlockPlus 1.3.10 (Solaris 10/SPARC): Crash while parsing filter patterns

Categories

(Core :: JavaScript Engine, defect, critical)

7 Branch
Sun
Solaris
defect
Not set
critical

Tracking

()

RESOLVED FIXED
mozilla10

People

(Reporter: Andre.Schneider, Assigned: ginnchen+exoracle)

Details

(Keywords: crash, regression, Whiteboard: sg:critical (on SPARC))

Attachments

(4 files, 1 obsolete file)

User Agent: Mozilla/5.0 (X11; SunOS sun4u; rv:6.0) Gecko/20100101 Firefox/6.0
Build ID: 20110812093811

Steps to reproduce:

If I remove the adblockplus directory (inside the Firefox profile directory ~/.mozilla/firefox/.../ ) Firefox starts without any problems.

If I add a new Filter Subscription via AdBlockPlus properties and press OK button Firefox crashes immediately. (see above)

If I restart Firefox the browser crashes immediately. (see above)
Now if I remove the adblockplus directory Firefox starts again without any problems ...

Same problems with Thunderbird 7


Actual results:

FF7 crashes after adding a new filter subscription with "Segmentation fault (core dumped) Exit 139"
Yarr should be JS Engine
Assignee: nobody → general
Severity: normal → critical
Component: General → JavaScript Engine
Keywords: crash
QA Contact: general → general
I assume this didn't happen in Firefox 6? We _did_ take a YARR update in Firefox 7 which could have a bug in it.

Which filter subscription did you add? Or did the same thing happen with multiple different subscriptions?

I guess we can hope this is unique to sparc otherwise support is going to see a lot of this due to ABP's popularity.
Keywords: regression
(In reply to Daniel Veditz from comment #2)
> I assume this didn't happen in Firefox 6? We _did_ take a YARR update in
> Firefox 7 which could have a bug in it.

YES, FF6/TB6 + AdBlockPlus 1.3.9 works fine.

> 
> Which filter subscription did you add? Or did the same thing happen with
> multiple different subscriptions?

I added subscriptions of "EasyList (English)" and of "EasyList Germany + EasyList (English)" - in BOTH cases same behavior (-> crash).

> 
> I guess we can hope this is unique to sparc ...

FF7/TB7 + AdBlockPlus 1.3.10 works correctly on Windows 7.
Maybe it's a UNIX/Linux problem ... or a Solaris/SPARC problem?
Can you post the crash ID from about:crashes? Or is that not available on SunOS?
(In reply to aceman from comment #4)
> Can you post the crash ID from about:crashes? Or is that not available on
> SunOS?

Sorry, URL is not available on SunOS.
FF7 + AdBlockPlus 1.3.10 works fine on Ubuntu 11.04 (32bit,linux-i686)

On Solaris 10/SPARC I can reproduce the crash on different machines and in several environments (installation from scratch, clean user environment, empty profile, no further addons, ...).
Bug 673850 ?
Sorry I didn't notice it is SPARC.
Then it should be Bug 666488.
(In reply to Ginn Chen from comment #8)
> Sorry I didn't notice it is SPARC.
> Then it should be Bug 666488.

No, Bug 666488 is fixed in Firefox 7.

I've reproduced this bug with Solaris contrib build of Firefox 7.
So either I missed something or this is a new bug.

I'll try mozilla-central first.
Status: UNCONFIRMED → NEW
Ever confirmed: true
With Firefox 7 debug build
I got:

Assertion failure: static_cast<unsigned>(-position) <= pos
(.../mozilla-release/js/src/yarr/YarrInterpreter.cpp:209 readChecked)

Stack:
 fd1d6fec int JSC::Yarr::Interpreter::InputStream::readChecked(int) (ffbf7d98, ffffffe2, 1, ffffffe1, ffbf7ffc, 1) + e4
 fd1d1738 bool JSC::Yarr::Interpreter::checkCharacter(int,int) (ffbf7d90, 2e, ffffffe2, 29adc10, ffbf7ffc, 1) + 28
 fd1cc34c JSC::Yarr::JSRegExpResult JSC::Yarr::Interpreter::matchDisjunction(JSC::Yarr::ByteDisjunction*,JSC::Yarr::Interpreter::DisjunctionContext*,bool,bool) (ffbf7d90, 1ae0d18, f3f40010, 1, 0, ffbf9260) + 4ac
 fd1d76b8 JSC::Yarr::JSRegExpResult JSC::Yarr::Interpreter::matchNonZeroDisjunction(JSC::Yarr::ByteDisjunction*,JSC::Yarr::Interpreter::DisjunctionContext*,bool) (ffbf7d90, 1ae0d18, f3f40010, 1, ffff0007, 8) + 38
 fd1cf3b0 JSC::Yarr::JSRegExpResult JSC::Yarr::Interpreter::backtrackParentheses(JSC::Yarr::ByteTerm&,JSC::Yarr::Interpreter::DisjunctionContext*) (ffbf7d90, b1d590, f3f40000, 1, 764a18, 4) + 740
 fd1cd490 JSC::Yarr::JSRegExpResult JSC::Yarr::Interpreter::matchDisjunction(JSC::Yarr::ByteDisjunction*,JSC::Yarr::Interpreter::DisjunctionContext*,bool,bool) (ffbf7d90, 202a8d0, f3f40000, 0, 1, ffbf925c) + 15f0
 fd1cad00 int JSC::Yarr::Interpreter::interpret() (ffbf7d90, 262f040, 345ef60, 28eedb0, 0, 2b) + 120
 fd1ca6a8 int JSC::Yarr::interpret(JSC::Yarr::BytecodePattern*,const unsigned short*,unsigned,unsigned,int*) (262f040, 28eedb0, 0, 2b, 345ef60, 4) + 58
 fcd624a0 bool js::RegExp::executeInternal(JSContext*,js::RegExpStatics*,JSString*,unsigned*,bool,js::Value*) (2658468, 38dfb0, c41f80, 29adc10, ffbf7ffc, 1) + 2d8
 fcd57158 bool js::RegExp::execute(JSContext*,js::RegExpStatics*,JSString*,unsigned*,bool,js::Value*) (2658468, 38dfb0, c41f80, 29adc10, ffbf7ffc, 1) + b8
 fcf61680 int ExecuteRegExp(JSContext*,ExecType,unsigned,js::Value*) (38dfb0, 1, 1, f5000250, 3, ffbf9260) + 728
 fcf6188c int js_regexp_test(JSContext*,unsigned,js::Value*) (38dfb0, 1, f5000250, 8, ffff0007, 8) + 2c
 fce702a0 bool js::CallJSNative(JSContext*,int(*)(JSContext*,unsigned,js::Value*),const js::CallArgs&) (38dfb0, fcf61860, ffbf8200, 1, 764a18, 4) + b0
 fce68970 bool js::Invoke(JSContext*,const js::CallArgs&,js::MaybeConstruct) (38dfb0, ffbf8ff4, 0, ffbf926c, ffbf9258, ffbf925c) + 378
 fd283bc4 bool js::Interpret(JSContext*,js::StackFrame*,js::InterpMode) (ffbf925c, 764a18, 0, 0, caa210, 3) + 1f3cc
 fce685cc bool js::RunScript(JSContext*,JSScript*,js::StackFrame*) (38dfb0, caa210, f5000038, 0, ffbfbecc, 8) + 234
 fce68b7c bool js::Invoke(JSContext*,const js::CallArgs&,js::MaybeConstruct) (38dfb0, ffbfbfe8, 0, 1, ffbfc000, 0) + 584
 fcd86160 bool js::Invoke(JSContext*,js::InvokeArgsGuard&,js::MaybeConstruct) (38dfb0, ffbfbfe8, 0, 0, ffbfc688, 0) + 48
 fce69418 bool js::ExternalInvoke(JSContext*,const js::Value&,const js::Value&,unsigned,js::Value*,js::Value*) (38dfb0, ffbfc080, ffbfc470, 3, ffbfc560, ffbfc4f0) + 1c8
 fcd49920 JS_CallFunctionValue (38dfb0, 9ec2c8, ffbfc470, 3, ffbfc560, ffbfc4f0) + 268
...

This issue also reproducible with Firefox 8 or 9.
Firefox 10 has other issues, it basically does not working on SPARC.
Group: core-security
static_cast<unsigned>(-position) <= pos .... that looks like an overflow, hiding just in case, is this Sun only?
I didn't see this on Solaris x86.
It might be an ENDIAN issue.

On SPARC, make check ALL PASSED for Firefox 7.
Attached file testcase
Assignee: general → ginn.chen
Status: NEW → ASSIGNED
Attached patch patch (obsolete) — Splinter Review
Verified with the testcase and make check on both x86 and SPARC.

Only chaning BASE_FRAME_SIZE to currentCallFrameSize +YarrStackSpaceForBackTrackInfoParentheses is necessary.

Other changes are to make the code similar for all branches.
I hope it would be easier to read.
Attachment #564541 - Flags: review?(dmandelin)
Filed https://bugs.webkit.org/show_bug.cgi?id=69448 to ask WebKit people for advice on the patch. I want to know if they want to take this patch, or if they have another way of fixing it. Also if it might be s-s we should let them have a look before landing.
How does this patch fix SPARC only? is there some padding or lack of padding not taken into account in BASE_FRAME_SIZE?  Is this a potentially exploitable crash (that is, could a clever attacker influence the crashing address to be something more useful)?
(In reply to David Mandelin from comment #15)
> Filed https://bugs.webkit.org/show_bug.cgi?id=69448 to ask WebKit people for
> advice on the patch. I want to know if they want to take this patch, or if
> they have another way of fixing it. Also if it might be s-s we should let
> them have a look before landing.

I just created an account on bugs.webkit.org. (ginn.chen@oracle.com)
Can you CC me in that bug?

I don't fully understand the code here.
In Firefox 6, we just use
                     term.inputPosition = currentInputPosition;
                     setupDisjunctionOffsets(term.parentheses.disjunction, 0, currentInputPosition);
                     currentCallFrameSize += YarrStackSpaceForBackTrackInfoParentheses;
here, both check-regexp.js and this case work fine.

In Firefox 7, check-regexp.js crashed if we use
setupDisjunctionOffsets(term.parentheses.disjunction, 0, currentInputPosition);
(the testcase in this bug works)

So Leon Sha changed it to
setupDisjunctionOffsets(term.parentheses.disjunction, BASE_FRAME_SIZE, currentInputPosition);
But it breaks the testcase here.
My patch probably is not the right fix.

The crash happens because we don't have enough space between frames.
We didn't alloc enough space in JSC::Yarr::Interpreter::allocDisjunctionContext() on SPARC, because we have set m_pattern.m_body->m_callFrameSize = 0; in YarrJIT.cpp for SPARC.

We set m_callFrameSize to 0 because we don't need add/sub %sp on SPARC.

So I think we can fix YarrJIT.cpp without hacking m_callFrameSize.
Attachment #564541 - Flags: review?(dmandelin)
Attached patch patchSplinter Review
Attachment #564541 - Attachment is obsolete: true
Attachment #565712 - Flags: review?(dmandelin)
Attachment #565712 - Flags: review?(dmandelin) → review+
Whiteboard: sg:critical (on SPARC)
https://hg.mozilla.org/integration/mozilla-inbound/rev/3e11e1feba3f
Target Milestone: --- → mozilla10
Shouldn't the test case be added somehow?
Attached patch add to testSplinter Review
Attachment #569305 - Flags: review?
https://hg.mozilla.org/mozilla-central/rev/3e11e1feba3f
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Comment on attachment 569305 [details] [diff] [review]
add to test

Seems the code-fix patch landed, but the test never did. Perhaps waiting for review? Flagging dmandelin to decide what to do. :)

I'd suggest either landing the patch, or just clearing the review request if it's no longer relevant.
Attachment #569305 - Flags: review? → review?(dmandelin)
Attachment #569305 - Flags: review?(dmandelin) → review+
testing file committed

http://hg.mozilla.org/mozilla-central/rev/35e13f42ee8a
Group: core-security
You need to log in before you can comment on or make changes to this bug.