Tyson Smith [:tsmith] Reporter
	Description • 7 years ago

These are triggered on launch.

Found in mozilla-central changeset: 403581:375d162649d2. Built with -fsanitize=vptr

/js/src/builtin/RegExp.cpp:1049:39: runtime error: member call on address 0x7ffc1c3677c8 which does not point to an object of type 'js::MatchPairs'
0x7ffc1c3677c8: note: object has invalid vptr
 ff 0f 00 00  fc ce 86 83 ff 0f 00 00  01 00 00 00 fc 7f 00 00  e0 77 36 1c fc 7f 00 00  ff ff ff ff
              ^~~~~~~~~~~~~~~~~~~~~~~
              invalid vptr
    #0 0x7fe622fd5e05 in js::RegExpMatcherRaw(JSContext*, JS::Handle<JSObject*>, JS::Handle<JSString*>, int, js::MatchPairs*, JS::MutableHandle<JS::Value>) /js/src/builtin/RegExp.cpp:1049:39
    #1 0x7fe5c5e3cbca  (<unknown module>)

------ SNIP! -------

/js/src/vm/MatchPairs.h:103:61: runtime error: member access within address 0x7ffc1c3677c8 which does not point to an object of type 'js::MatchPairs'
0x7ffc1c3677c8: note: object has invalid vptr
 ff 0f 00 00  fc ce 86 83 ff 0f 00 00  01 00 00 00 fc 7f 00 00  e0 77 36 1c fc 7f 00 00  ff ff ff ff
              ^~~~~~~~~~~~~~~~~~~~~~~
              invalid vptr
    #0 0x7fe622fd5e2b in pairsRaw /js/src/vm/MatchPairs.h:103:61
    #1 0x7fe622fd5e2b in js::RegExpMatcherRaw(JSContext*, JS::Handle<JSObject*>, JS::Handle<JSString*>, int, js::MatchPairs*, JS::MutableHandle<JS::Value>) /js/src/builtin/RegExp.cpp:1049
    #2 0x7fe5c5e3cbca  (<unknown module>)

------ SNIP! -------

=================================================================
==36723==ERROR: AddressSanitizer: SEGV on unknown address 0x000000000000 (pc 0x7fe6384f6436 bp 0x000000000000 sp 0x7ffc1c34e210 T0)
==36723==The signal is caused by a READ memory access.
==36723==Hint: address points to the zero page.
    #0 0x7fe6384f6435 in __dynamic_cast (/usr/lib/x86_64-linux-gnu/libstdc++.so.6+0x8e435)
    #1 0x515bbe in __ubsan::checkDynamicType(void*, void*, unsigned long) (/objdir-ff-vptr/dist/bin/firefox+0x515bbe)
    #2 0x513d82 in HandleDynamicTypeCacheMiss(__ubsan::DynamicTypeCacheMissData*, unsigned long, unsigned long, __ubsan::ReportOptions) (/objdir-ff-vptr/dist/bin/firefox+0x513d82)
    #3 0x514510 in __ubsan_handle_dynamic_type_cache_miss (/objdir-ff-vptr/dist/bin/firefox+0x514510)
    #4 0x7fe622fd5e05 in js::RegExpMatcherRaw(JSContext*, JS::Handle<JSObject*>, JS::Handle<JSString*>, int, js::MatchPairs*, JS::MutableHandle<JS::Value>) /js/src/builtin/RegExp.cpp:1049:39
    #5 0x7fe5c5e3cbca  (<unknown module>)

	Jan de Mooij [:jandem] Assignee
	Comment 1 • 7 years ago

Yes this is a bit fishy. MatchPairs is virtual and AFAICS we're passing a stack-allocated MatchPairs without a vtable to RegExpMatcherRaw.

I think we should just try to devirtualize MatchPairs.

	Jan de Mooij [:jandem] Assignee
	Comment 2 • 7 years ago

Here's a patch to remove ScopedMatchPairs and then we no longer need the virtual function.

I think using VectorMatchPairs instead of ScopedMatchPairs is okay (the Vector has inline capacity); I don't see any perf difference on Octane-regexp. We could also use MaybeOneOf in VectorMatchPairs...

	Jeff Walden [:Waldo]
	Comment 3 • 7 years ago

"stack-allocated MatchPairs without a vtable"

wat

	Jan de Mooij [:jandem] Assignee
	Comment 4 • 7 years ago

(In reply to Jeff Walden [:Waldo] from comment #3)
> "stack-allocated MatchPairs without a vtable"
> 
> wat

I should have said "garbage vtable". Not that it's better..

	Jeff Walden [:Waldo]
	Comment 5 • 7 years ago

Comment on attachment 8951193 [details] [diff] [review]
Patch

Review of attachment 8951193 [details] [diff] [review]:
-----------------------------------------------------------------

For uncertain reasons, I'm having a difficult time wrapping my head around this patch, but it seems to check out.

If I read this correctly, |MatchPairs| exists only as base class of |VectorMatchPairs|, right?  Could you combine the two classes into one in a followup patch?

::: js/src/vm/RegExpObject.cpp
@@ +87,4 @@
>  VectorMatchPairs::allocOrExpandArray(size_t pairCount)
>  {
>      if (!vec_.resizeUninitialized(sizeof(MatchPair) * pairCount))
>          return false;

Given that |vec_| is of MatchPair, why is this multiplication here?  Why not use simply |pairCount|?

	Jan de Mooij [:jandem] Assignee
	Comment 6 • 7 years ago

(In reply to Jeff Walden [:Waldo] from comment #5)
> If I read this correctly, |MatchPairs| exists only as base class of
> |VectorMatchPairs|, right?

JIT code can pass a stack-allocated MatchPairs that's not a VectorMatchPairs (JIT code allocating its own MatchPairs caused this bug in the first place). I can add a comment to the MatchPairs class.

> Given that |vec_| is of MatchPair, why is this multiplication here?  Why not
> use simply |pairCount|?

Hm pre-existing. Good find, I didn't notice that but I agree it looks bogus.

	Pulsebot
	Comment 7 • 6 years ago

Pushed by jandemooij@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/41857846ad59
Remove ScopedMatchPairs and devirtualize MatchPairs to avoid triggering undefined behavior. r=jwalden

	Andreea Pavel [:apavel]
	Comment 8 • 6 years ago
bugherder

https://hg.mozilla.org/mozilla-central/rev/41857846ad59