rdar://problem/36286370
Created attachment 332074 [details] the patch
<rdar://problem/36793736>
<rdar://problem/36286370>
Comment on attachment 332074 [details] the patch View in context: https://bugs.webkit.org/attachment.cgi?id=332074&action=review r=me with comments. > Source/JavaScriptCore/ftl/FTLLowerDFGToB3.cpp:3987 > + LValue limit = m_out.sub(originalLimit, m_out.int32One); Nit: I would make this LValue thisSize = m_out.int32One; LValue limit = m_out.sub(originalLimit, thisSize); I feel like I'll forget that the -1 is for this. > Source/JavaScriptCore/ftl/FTLLowerDFGToB3.cpp:4009 > + index = m_out.add(index, m_out.constInt32(1)); Nit: m_out.constInt32(1) => m_out.int32One. Also, ditto on the thisSize. > Source/JavaScriptCore/ftl/FTLLowerDFGToB3.cpp:4017 > + index = m_out.bitAnd( > + index, > + m_out.aShr( > + m_out.sub( > + index, > + m_out.opaque(originalLimit)), > + m_out.constInt32(31))); Nit: I would do this math on 64-bit values. That avoids people passing very large 32-bit indices and causes issues here. E.g. arguments[UINT_MAX - epsilon] would produce a non-zero mask.
(In reply to Keith Miller from comment #4) > Comment on attachment 332074 [details] > the patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=332074&action=review > > r=me with comments. > > > Source/JavaScriptCore/ftl/FTLLowerDFGToB3.cpp:3987 > > + LValue limit = m_out.sub(originalLimit, m_out.int32One); > > Nit: I would make this > > LValue thisSize = m_out.int32One; > LValue limit = m_out.sub(originalLimit, thisSize); > > I feel like I'll forget that the -1 is for this. > > > Source/JavaScriptCore/ftl/FTLLowerDFGToB3.cpp:4009 > > + index = m_out.add(index, m_out.constInt32(1)); > > Nit: m_out.constInt32(1) => m_out.int32One. Oops. Fixed. > > Also, ditto on the thisSize. > > > Source/JavaScriptCore/ftl/FTLLowerDFGToB3.cpp:4017 > > + index = m_out.bitAnd( > > + index, > > + m_out.aShr( > > + m_out.sub( > > + index, > > + m_out.opaque(originalLimit)), > > + m_out.constInt32(31))); > > Nit: I would do this math on 64-bit values. That avoids people passing very > large 32-bit indices and causes issues here. > > E.g. arguments[UINT_MAX - epsilon] would produce a non-zero mask. Good call. I'll try that.
Landed in https://trac.webkit.org/changeset/227462/webkit
Comment on attachment 332074 [details] the patch View in context: https://bugs.webkit.org/attachment.cgi?id=332074&action=review > Source/JavaScriptCore/ftl/FTLLowerDFGToB3.cpp:3990 > + if (m_node->numberOfArgumentsToSkip()) > + limit = m_out.sub(limit, m_out.constInt32(m_node->numberOfArgumentsToSkip())); How did this fix an overflow situation? It introduced an overflow situation. Both limit and numberOfArugmnetsToSkip are user controlled. For example: ``` function foo(a, b, ...args) { return args[0]; } foo(10); ``` here, limit = 0 argsToSkip = 2 limit - argsToSkip = bad time
(In reply to Saam Barati from comment #7) > Comment on attachment 332074 [details] > the patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=332074&action=review > > > Source/JavaScriptCore/ftl/FTLLowerDFGToB3.cpp:3990 > > + if (m_node->numberOfArgumentsToSkip()) > > + limit = m_out.sub(limit, m_out.constInt32(m_node->numberOfArgumentsToSkip())); > > How did this fix an overflow situation? It introduced an overflow situation. > Both limit and numberOfArugmnetsToSkip are user controlled. For example: > > ``` > function foo(a, b, ...args) { > return args[0]; > } > foo(10); > ``` > > here, > limit = 0 > argsToSkip = 2 > limit - argsToSkip = bad time What we need here is to go back to the old code, but to perform an overflow check on the original add. Or we need to do some branch before this sub. Either would work.
(In reply to Saam Barati from comment #8) > > here, > > limit = 0 > > argsToSkip = 2 > > limit - argsToSkip = bad time > > What we need here is to go back to the old code, but to perform an overflow > check on the original add. Or we need to do some branch before this sub. > Either would work. We're fixing this in https://bugs.webkit.org/show_bug.cgi?id=182419.