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

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[André Schneider]

Attachment: Stacktrace after crash (Firefox 7, post-mortem debuggung using GDB 7)

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"

Comment 1

[Matthias Versen [:Matti]]

Yarr should be JS Engine

Comment 2

[Daniel Veditz [:dveditz]]

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.

Comment 3

[André Schneider]

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

Comment 4

[:aceman]

Can you post the crash ID from about:crashes? Or is that not available on SunOS?

Comment 5

[André Schneider]

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

Comment 6

[André Schneider]

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

Comment 7

[Ginn Chen]

Bug 673850 ?

Comment 8

[Ginn Chen]

Sorry I didn't notice it is SPARC.
Then it should be Bug 666488.

Comment 9

[Ginn Chen]

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

Comment 10

[Ginn Chen]

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.

Comment 11

[Andreas Gal :gal]

static_cast<unsigned>(-position) <= pos .... that looks like an overflow, hiding just in case, is this Sun only?

Comment 12

[Ginn Chen]

I didn't see this on Solaris x86.
It might be an ENDIAN issue.

On SPARC, make check ALL PASSED for Firefox 7.

Comment 13

[Ginn Chen]

Attachment: testcase

Comment 14

[Ginn Chen]

Attachment: patch

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.

Comment 15

[David Mandelin [: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.

Comment 16

[Daniel Veditz [:dveditz]]

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

Comment 17

[Ginn Chen]

(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. ([e-mail])
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.

Comment 18

[Ginn Chen]

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.

Comment 19

[Ginn Chen]

Attachment: patch

Comment 20

[Ginn Chen]

https://hg.mozilla.org/integration/mozilla-inbound/rev/3e11e1feba3f

Comment 21

[Mike Hommey [:glandium]]

Shouldn't the test case be added somehow?

Comment 22

[Ginn Chen]

Attachment: add to test

Comment 23

[Ginn Chen]

https://hg.mozilla.org/mozilla-central/rev/3e11e1feba3f

Comment 24

[Justin Dolske [:Dolske]]

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.

Comment 25

[Ginn Chen]

testing file committed

http://hg.mozilla.org/mozilla-central/rev/35e13f42ee8a