Bug 118878

Summary:

fourthTier: Structure should be able to tell you if it's valid to load at a given offset from any object with that structure

Product:

WebKit

Reporter:

Filip Pizlo <fpizlo>

Component:

JavaScriptCore

Assignee:

Filip Pizlo <fpizlo>

Status:

RESOLVED FIXED

Severity:

Normal

CC:

barraclough, ggaren, mark.lam, mhahnenberg, msaboff, oliver, sam

Priority:

Version:

528+ (Nightly build)

Hardware:

All

OS:

All

Bug Depends on:

Bug Blocks:

118866

Attachments:

Description	Flags
the patch	none
the patch	none
the patch	oliver: review+

Filip Pizlo

Reported 2013-07-18 18:53:19 PDT

Patch forthcoming.

Attachments
the patch (12.79 KB, patch) 2013-07-18 20:31 PDT, Filip Pizlo	no flags	Details Formatted Diff Diff
the patch (14.46 KB, patch) 2013-07-19 09:57 PDT, Filip Pizlo	no flags	Details Formatted Diff Diff
the patch (14.66 KB, patch) 2013-07-19 11:09 PDT, Filip Pizlo	oliver: review+	Details Formatted Diff Diff
Show Obsolete (2) View All Add attachment proposed patch, testcase, etc.

Filip Pizlo

Comment 1 2013-07-18 19:22:08 PDT

Thus, if we have a GetByOffset with a given offset, we can check if the object was proved to have a structure that has that offset; if it does, then the GetByOffset can proceed.

Filip Pizlo

Comment 2 2013-07-18 20:31:00 PDT

Created attachment 207054 [details] the patch

Filip Pizlo

Comment 3 2013-07-19 09:57:27 PDT

Created attachment 207110 [details] the patch

Filip Pizlo

Comment 4 2013-07-19 11:09:43 PDT

Created attachment 207125 [details] the patch Just improved the changelog.

Oliver Hunt

Comment 5 2013-07-19 11:11:22 PDT

Comment on attachment 207125 [details] the patch View in context: https://bugs.webkit.org/attachment.cgi?id=207125&action=review > Source/JavaScriptCore/ChangeLog:35 > + that could transitively try to compute the hash from the source code. The source > + code is a string that may be lazily computed, and that involves all manner of thread > + unsafe things. Do we have any assertions that would catch this?

Filip Pizlo

Comment 6 2013-07-19 11:12:06 PDT

(In reply to comment #5) > (From update of attachment 207125 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=207125&action=review > > > Source/JavaScriptCore/ChangeLog:35 > > + that could transitively try to compute the hash from the source code. The source > > + code is a string that may be lazily computed, and that involves all manner of thread > > + unsafe things. > > Do we have any assertions that would catch this? Yes, that's how I caught it. StringImpl's ref/deref assert that you're not in the compiler thread.

Geoffrey Garen

Comment 7 2013-07-19 11:12:34 PDT

Comment on attachment 207125 [details] the patch View in context: https://bugs.webkit.org/attachment.cgi?id=207125&action=review > Source/JavaScriptCore/runtime/Structure.h:228 > bool isValidOffset(PropertyOffset offset) const > { > - return offset >= firstValidOffset() > - && offset <= lastValidOffset(); > + return JSC::isValidOffset(offset) > + && (offset < m_inlineCapacity > + || (offset >= firstOutOfLineOffset && offset <= m_offset)); > } I think it's worth a comment here explaining that a "valid" offset isn't necessarily a valid JSValue -- it's just an offset that, if you load from it, you won't crash.

Filip Pizlo

Comment 8 2013-07-19 11:14:55 PDT

Landed in http://trac.webkit.org/changeset/152909

Filip Pizlo

Comment 9 2013-07-19 11:15:58 PDT

(In reply to comment #7) > (From update of attachment 207125 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=207125&action=review > > > Source/JavaScriptCore/runtime/Structure.h:228 > > bool isValidOffset(PropertyOffset offset) const > > { > > - return offset >= firstValidOffset() > > - && offset <= lastValidOffset(); > > + return JSC::isValidOffset(offset) > > + && (offset < m_inlineCapacity > > + || (offset >= firstOutOfLineOffset && offset <= m_offset)); > > } > > I think it's worth a comment here explaining that a "valid" offset isn't necessarily a valid JSValue -- it's just an offset that, if you load from it, you won't crash. Interesting. While that's not what I intended for this method, its users are indeed file with the "you won't crash" part. I'll change it though, in a separate bug.

Filip Pizlo

Comment 10 2013-07-19 11:21:24 PDT

(In reply to comment #9) > (In reply to comment #7) > > (From update of attachment 207125 [details] [details]) > > View in context: https://bugs.webkit.org/attachment.cgi?id=207125&action=review > > > > > Source/JavaScriptCore/runtime/Structure.h:228 > > > bool isValidOffset(PropertyOffset offset) const > > > { > > > - return offset >= firstValidOffset() > > > - && offset <= lastValidOffset(); > > > + return JSC::isValidOffset(offset) > > > + && (offset < m_inlineCapacity > > > + || (offset >= firstOutOfLineOffset && offset <= m_offset)); > > > } > > > > I think it's worth a comment here explaining that a "valid" offset isn't necessarily a valid JSValue -- it's just an offset that, if you load from it, you won't crash. > > Interesting. While that's not what I intended for this method, its users are indeed file with the "you won't crash" part. I'll change it though, in a separate bug. https://bugs.webkit.org/show_bug.cgi?id=118911

Note You need to log in before you can comment on or make changes to this bug.