Bug 202312 – operationSwitchCharWithUnknownKeyType failed to handle OOME when resolving rope string.

• Status: RESOLVED
• Resolution: FIXED
• Component: JavaScriptCore

[Insu Yun]

Created attachment 379724 [details]
poc program

Hi, during testing, I found this crash input.
I tested it with commit ca690fd881e742d539636921af00cf8bc2e3987e in Linux.

Current PoC triggers NULL dereference.
I don't know this can cause serious security problems like code execution,
but I filed it as security at least it can cause some memory corruption.

Here is a stack trace from gdb.

```
Thread 1 "jsc" received signal SIGSEGV, Segmentation fault.
[----------------------------------registers-----------------------------------]
RAX: 0x0
RBX: 0x0
RCX: 0x0
RDX: 0x8
RSI: 0x0
RDI: 0x0
RBP: 0x7fffffffbc60 --> 0x7fffffffbce0 --> 0x7fffffffbd30 --> 0x7fffffffbda0 --> 0x7fffffffbe20 --> 0x7fffffffbef0 --> 0x7fffffffd530 --> 0x7fffffffd640 --> 0x7fffffffd9b0 --> 0x7fffffffd9e0 --> 0x7fffffffdc30 --> 0x7fffffffdce0 --> 0x7ff
fffffdd10 --> 0x1cf0c70 (<__libc_csu_init>:     push   r15)
RSP: 0x7fffffffbc60 --> 0x7fffffffbce0 --> 0x7fffffffbd30 --> 0x7fffffffbda0 --> 0x7fffffffbe20 --> 0x7fffffffbef0 --> 0x7fffffffd530 --> 0x7fffffffd640 --> 0x7fffffffd9b0 --> 0x7fffffffd9e0 --> 0x7fffffffdc30 --> 0x7fffffffdce0 --> 0x7ff
fffffdd10 --> 0x1cf0c70 (<__libc_csu_init>:     push   r15)
RIP: 0x421f4c (<WTF::StringImpl::length() const+12>:    mov    eax,DWORD PTR [rdi+0x4])
R8 : 0x0
R9 : 0x7ffff7ffc680 --> 0x0
R10: 0x7ffff7dd5000 --> 0x10102464c457f
R11: 0x7ffff4e89260 --> 0x400000 --> 0x10102464c457f
R12: 0x7fffb3bfad10 --> 0x68006800680068 ('h')
R13: 0x7ffff3fcccc0 --> 0x89a9fbfa41fb3635
R14: 0xfffe000000000000
R15: 0xfffe000000000002
EFLAGS: 0x10206 (carry PARITY adjust zero sign trap INTERRUPT direction overflow)
[-------------------------------------code-------------------------------------]
   0x421f41 <WTF::StringImpl::length() const+1>:        mov    rbp,rsp
   0x421f44 <WTF::StringImpl::length() const+4>:        mov    QWORD PTR [rbp-0x8],rdi
   0x421f48 <WTF::StringImpl::length() const+8>:        mov    rdi,QWORD PTR [rbp-0x8]
=> 0x421f4c <WTF::StringImpl::length() const+12>:       mov    eax,DWORD PTR [rdi+0x4]
   0x421f4f <WTF::StringImpl::length() const+15>:       pop    rbp
   0x421f50 <WTF::StringImpl::length() const+16>:       ret
   0x421f51:    data16 data16 data16 data16 data16 nop WORD PTR cs:[rax+rax*1+0x0]
   0x421f60 <WTF::Ref<WTF::AtomStringImpl, WTF::DumbPtrTraits<WTF::AtomStringImpl> >::Ref(WTF::AtomStringImpl&)>:       push   rbp

Stopped reason: SIGSEGV
WTF::StringImpl::length (this=0x0) at DerivedSources/ForwardingHeaders/wtf/text/StringImpl.h:274
274         unsigned length() const { return m_length; }
gdb-peda$ bt
#0  WTF::StringImpl::length (this=0x0) at DerivedSources/ForwardingHeaders/wtf/text/StringImpl.h:274
#1  0x0000000000fbe34d in operationSwitchCharWithUnknownKeyType (exec=0x7fffffffbd30, encodedKey=0x7fffb3998000, tableIndex=0x0) at ../../Source/JavaScriptCore/jit/JITOperations.cpp:2306
#2  0x00007fffb3eff998 in ?? ()
#3  0x00007fffb3998000 in ?? ()
#4  0x00007fffb39e4000 in ?? ()
#5  0x00007fffb39d4000 in ?? ()
#6  0x00007fffb39d4000 in ?? ()
#7  0x00007fffb3bfad10 in ?? ()
#8  0x00007ffff3fcccc0 in ?? ()
#9  0xfffe000000000000 in ?? ()
#10 0xfffe000000000002 in ?? ()
#11 0x00007fffffffbda0 in ?? ()
#12 0x000000000104faf4 in llint_op_call ()
#13 0x00007fffb39a3d40 in ?? ()
#14 0x00007fffb39ac030 in ?? ()
#15 0x0000001600000001 in ?? ()
#16 0x00007fffb39e4000 in ?? ()
#17 0x00007fffb39ac030 in ?? ()
#18 0x000000000000000a in ?? ()
#19 0x00007fffb39d4000 in ?? ()
#20 0x00007fffb39d4000 in ?? ()
#21 0x00000000004068c0 in ?? ()
#22 0x00007fffffffddf0 in ?? ()
#23 0x0000000000000000 in ?? ()
```

[Radar WebKit Bug Importer]

<rdar://problem/55782280>

[Mark Lam]

Created attachment 381030 [details]
proposed patch.

[Mark Lam]

@Insu, thanks for the bug report.  This is just a null dereference bug due to a missing OOME check.

[Robin Morisset]

Comment on attachment 381030 [details]
proposed patch.

r=me

[Yusuke Suzuki]

Comment on attachment 381030 [details]
proposed patch.

View in context: https://bugs.webkit.org/attachment.cgi?id=381030&action=review

> Source/JavaScriptCore/jit/JITOperations.cpp:2317
> -        StringImpl* value = asString(key)->value(exec).impl();
> -        if (value->length() == 1)
> +        JSString* string = asString(key);
> +        if (string->length() == 1) {
> +            StringImpl* value = string->value(exec).impl();
>              result = jumpTable.ctiForValue((*value)[0]).executableAddress();
> +        }

We can create arbitrary rope with length = 1, including substring IIRC.
So, let's just check exception. And ensure DFG doesGC and mayExit are saying the right thing.

[Mark Lam]

Created attachment 381034 [details]
propose patch.

[Yusuke Suzuki]

Comment on attachment 381034 [details]
propose patch.

View in context: https://bugs.webkit.org/attachment.cgi?id=381034&action=review

r=me with nit.

> Source/JavaScriptCore/jit/JITOperations.cpp:2316
> +            StringImpl* value = string->value(exec).impl();

I suggest doing

String value = string->value(exec);
RETURN_IF_EXCEPTION(throwScope, nullptr);
Use value[0] Or  value.impl()[0]

To make sure that string is live (I think this case is OK, but it is still better I think).

[Mark Lam]

Created attachment 381042 [details]
proposed patch.

[Yusuke Suzuki]

Comment on attachment 381042 [details]
proposed patch.

r=me

[Mark Lam]

Thanks for the review.  Landed in r251178: <http://trac.webkit.org/r251178>.