From 4d767a25f6169648580c4435cb5b7366e7ff5ee0 Mon Sep 17 00:00:00 2001 From: Mark Hahnenberg Date: Mon, 29 Sep 2014 18:13:42 +0200 Subject: (un)shiftCountWithAnyIndexingType will start over in the middle of copying if it sees a hole https://bugs.webkit.org/show_bug.cgi?id=121717 Reviewed by Oliver Hunt. Source/JavaScriptCore: This bug caused the array to become corrupted. We now check for holes before we start moving things, and start moving things only once we've determined that there are none. * runtime/JSArray.cpp: (JSC::JSArray::shiftCountWithAnyIndexingType): (JSC::JSArray::unshiftCountWithAnyIndexingType): Change-Id: I9948bfa2c9b4a345076f7f2b4e50a566f521b6fe git-svn-id: http://svn.webkit.org/repository/webkit/trunk@156214 268f45cc-cd09-0410-ab3c-d52691b4dbfc Reviewed-by: Jocelyn Turcotte --- Source/JavaScriptCore/runtime/JSArray.cpp | 60 +++++++++++++++++++------------ 1 file changed, 37 insertions(+), 23 deletions(-) (limited to 'Source/JavaScriptCore/runtime/JSArray.cpp') diff --git a/Source/JavaScriptCore/runtime/JSArray.cpp b/Source/JavaScriptCore/runtime/JSArray.cpp index f97ccedcd..bc6f73672 100644 --- a/Source/JavaScriptCore/runtime/JSArray.cpp +++ b/Source/JavaScriptCore/runtime/JSArray.cpp @@ -755,21 +755,21 @@ bool JSArray::shiftCountWithAnyIndexingType(ExecState* exec, unsigned startIndex // so only if it's not horribly slow. if (oldLength - (startIndex + count) >= MIN_SPARSE_ARRAY_INDEX) return shiftCountWithArrayStorage(startIndex, count, ensureArrayStorage(exec->vm())); - + + // Storing to a hole is fine since we're still having a good time. But reading from a hole + // is totally not fine, since we might have to read from the proto chain. + // We have to check for holes before we start moving things around so that we don't get halfway + // through shifting and then realize we should have been in ArrayStorage mode. unsigned end = oldLength - count; for (unsigned i = startIndex; i < end; ++i) { - // Storing to a hole is fine since we're still having a good time. But reading - // from a hole is totally not fine, since we might have to read from the proto - // chain. JSValue v = m_butterfly->contiguous()[i + count].get(); - if (UNLIKELY(!v)) { - // The purpose of this path is to ensure that we don't make the same - // mistake in the future: shiftCountWithArrayStorage() can't do anything - // about holes (at least for now), but it can detect them quickly. So - // we convert to array storage and then allow the array storage path to - // figure it out. + if (UNLIKELY(!v)) return shiftCountWithArrayStorage(startIndex, count, ensureArrayStorage(exec->vm())); - } + } + + for (unsigned i = startIndex; i < end; ++i) { + JSValue v = m_butterfly->contiguous()[i + count].get(); + ASSERT(v); // No need for a barrier since we're just moving data around in the same vector. // This is in line with our standing assumption that we won't have a deletion // barrier. @@ -790,21 +790,21 @@ bool JSArray::shiftCountWithAnyIndexingType(ExecState* exec, unsigned startIndex // so only if it's not horribly slow. if (oldLength - (startIndex + count) >= MIN_SPARSE_ARRAY_INDEX) return shiftCountWithArrayStorage(startIndex, count, ensureArrayStorage(exec->vm())); - + + // Storing to a hole is fine since we're still having a good time. But reading from a hole + // is totally not fine, since we might have to read from the proto chain. + // We have to check for holes before we start moving things around so that we don't get halfway + // through shifting and then realize we should have been in ArrayStorage mode. unsigned end = oldLength - count; for (unsigned i = startIndex; i < end; ++i) { - // Storing to a hole is fine since we're still having a good time. But reading - // from a hole is totally not fine, since we might have to read from the proto - // chain. double v = m_butterfly->contiguousDouble()[i + count]; - if (UNLIKELY(v != v)) { - // The purpose of this path is to ensure that we don't make the same - // mistake in the future: shiftCountWithArrayStorage() can't do anything - // about holes (at least for now), but it can detect them quickly. So - // we convert to array storage and then allow the array storage path to - // figure it out. + if (UNLIKELY(v != v)) return shiftCountWithArrayStorage(startIndex, count, ensureArrayStorage(exec->vm())); - } + } + + for (unsigned i = startIndex; i < end; ++i) { + double v = m_butterfly->contiguousDouble()[i + count]; + ASSERT(v == v); // No need for a barrier since we're just moving data around in the same vector. // This is in line with our standing assumption that we won't have a deletion // barrier. @@ -889,11 +889,18 @@ bool JSArray::unshiftCountWithAnyIndexingType(ExecState* exec, unsigned startInd return unshiftCountWithArrayStorage(exec, startIndex, count, ensureArrayStorage(exec->vm())); ensureLength(exec->vm(), oldLength + count); - + + // We have to check for holes before we start moving things around so that we don't get halfway + // through shifting and then realize we should have been in ArrayStorage mode. for (unsigned i = oldLength; i-- > startIndex;) { JSValue v = m_butterfly->contiguous()[i].get(); if (UNLIKELY(!v)) return unshiftCountWithArrayStorage(exec, startIndex, count, ensureArrayStorage(exec->vm())); + } + + for (unsigned i = oldLength; i-- > startIndex;) { + JSValue v = m_butterfly->contiguous()[i].get(); + ASSERT(v); m_butterfly->contiguous()[i + count].setWithoutWriteBarrier(v); } @@ -915,10 +922,17 @@ bool JSArray::unshiftCountWithAnyIndexingType(ExecState* exec, unsigned startInd ensureLength(exec->vm(), oldLength + count); + // We have to check for holes before we start moving things around so that we don't get halfway + // through shifting and then realize we should have been in ArrayStorage mode. for (unsigned i = oldLength; i-- > startIndex;) { double v = m_butterfly->contiguousDouble()[i]; if (UNLIKELY(v != v)) return unshiftCountWithArrayStorage(exec, startIndex, count, ensureArrayStorage(exec->vm())); + } + + for (unsigned i = oldLength; i-- > startIndex;) { + double v = m_butterfly->contiguousDouble()[i]; + ASSERT(v == v); m_butterfly->contiguousDouble()[i + count] = v; } -- cgit v1.2.1