Closed Bug 1438310 Opened 2 years ago Closed 2 years ago

UBSan: member call on address which does not point to an object of type 'js::MatchPairs' in /js/src/builtin/RegExp.cpp

Categories

(Core :: JavaScript Engine: JIT, defect, P1)

60 Branch
defect

Tracking

()

RESOLVED FIXED
mozilla60
Tracking Status
firefox60 --- fixed

People

(Reporter: tsmith, Assigned: jandem)

Details

(Keywords: csectype-undefined)

Attachments

(1 file)

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>)
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.
Attached patch PatchSplinter Review
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...
Assignee: nobody → jdemooij
Status: NEW → ASSIGNED
Attachment #8951193 - Flags: review?(jwalden+bmo)
Priority: -- → P1
"stack-allocated MatchPairs without a vtable"

wat
(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..
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|?
Attachment #8951193 - Flags: review?(jwalden+bmo) → review+
(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.
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
https://hg.mozilla.org/mozilla-central/rev/41857846ad59
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla60
You need to log in before you can comment on or make changes to this bug.