Skip to content
Permalink
Browse files

[Re-landing] CachedCall should let GC know to keep its arguments alive.

https://bugs.webkit.org/show_bug.cgi?id=168567
<rdar://problem/30475767>

Reviewed by Saam Barati.

Source/JavaScriptCore:

We fix this by having CachedCall use a MarkedArgumentBuffer to store its
arguments instead of a Vector.

Also declared CachedCall, MarkedArgumentBuffer, and ProtoCallFrame as
WTF_FORBID_HEAP_ALLOCATION because they rely on being stack allocated for
correctness.

Update: the original patch has a bug in MarkedArgumentBuffer::expandCapacity()
where it was copying and calling addMarkSet() on values in m_buffer beyond m_size
(up to m_capacity).  As a result, depending on the pre-existing values in
m_inlineBuffer, this may result in a computed Heap pointer that is wrong, and
subsequently, manifest as a crash.  This is likely to be the cause of the PLT
regression.

I don't have a new test for this fix because the issue relies on sufficiently bad
values randomly showing up in m_inlineBuffer when we do an ensureCapacity() which
calls expandCapacity().

* interpreter/CachedCall.h:
(JSC::CachedCall::CachedCall):
(JSC::CachedCall::call):
(JSC::CachedCall::clearArguments):
(JSC::CachedCall::appendArgument):
(JSC::CachedCall::setArgument): Deleted.
* interpreter/CallFrame.h:
(JSC::ExecState::emptyList):
* interpreter/Interpreter.cpp:
(JSC::Interpreter::prepareForRepeatCall):
* interpreter/Interpreter.h:
* interpreter/ProtoCallFrame.h:
* runtime/ArgList.cpp:
(JSC::MarkedArgumentBuffer::slowEnsureCapacity):
(JSC::MarkedArgumentBuffer::expandCapacity):
(JSC::MarkedArgumentBuffer::slowAppend):
* runtime/ArgList.h:
(JSC::MarkedArgumentBuffer::append):
(JSC::MarkedArgumentBuffer::ensureCapacity):
* runtime/StringPrototype.cpp:
(JSC::replaceUsingRegExpSearch):
* runtime/VM.cpp:
(JSC::VM::VM):
* runtime/VM.h:

Source/WTF:

Added a WTF_FORBID_HEAP_ALLOCATION that will cause a compilation failure if
a class declared with it is malloced.

While this doesn't prevent that class declared WTF_FORBID_HEAP_ALLOCATION from
being embedded in another class that is heap allocated, it does at minimum
document the intent and gives the users of this class a chance to do the
right thing.

* WTF.xcodeproj/project.pbxproj:
* wtf/ForbidHeapAllocation.h: Added.


git-svn-id: http://svn.webkit.org/repository/webkit/trunk@212692 268f45cc-cd09-0410-ab3c-d52691b4dbfc
  • Loading branch information...
mark.lam@apple.com
mark.lam@apple.com committed Feb 21, 2017
1 parent 4982680 commit 7d1b3b9542d9870b8524f284e108bea56397bd3a
@@ -1,3 +1,54 @@
2017-02-20 Mark Lam <mark.lam@apple.com>

[Re-landing] CachedCall should let GC know to keep its arguments alive.
https://bugs.webkit.org/show_bug.cgi?id=168567
<rdar://problem/30475767>

Reviewed by Saam Barati.

We fix this by having CachedCall use a MarkedArgumentBuffer to store its
arguments instead of a Vector.

Also declared CachedCall, MarkedArgumentBuffer, and ProtoCallFrame as
WTF_FORBID_HEAP_ALLOCATION because they rely on being stack allocated for
correctness.

Update: the original patch has a bug in MarkedArgumentBuffer::expandCapacity()
where it was copying and calling addMarkSet() on values in m_buffer beyond m_size
(up to m_capacity). As a result, depending on the pre-existing values in
m_inlineBuffer, this may result in a computed Heap pointer that is wrong, and
subsequently, manifest as a crash. This is likely to be the cause of the PLT
regression.

I don't have a new test for this fix because the issue relies on sufficiently bad
values randomly showing up in m_inlineBuffer when we do an ensureCapacity() which
calls expandCapacity().

* interpreter/CachedCall.h:
(JSC::CachedCall::CachedCall):
(JSC::CachedCall::call):
(JSC::CachedCall::clearArguments):
(JSC::CachedCall::appendArgument):
(JSC::CachedCall::setArgument): Deleted.
* interpreter/CallFrame.h:
(JSC::ExecState::emptyList):
* interpreter/Interpreter.cpp:
(JSC::Interpreter::prepareForRepeatCall):
* interpreter/Interpreter.h:
* interpreter/ProtoCallFrame.h:
* runtime/ArgList.cpp:
(JSC::MarkedArgumentBuffer::slowEnsureCapacity):
(JSC::MarkedArgumentBuffer::expandCapacity):
(JSC::MarkedArgumentBuffer::slowAppend):
* runtime/ArgList.h:
(JSC::MarkedArgumentBuffer::append):
(JSC::MarkedArgumentBuffer::ensureCapacity):
* runtime/StringPrototype.cpp:
(JSC::replaceUsingRegExpSearch):
* runtime/VM.cpp:
(JSC::VM::VM):
* runtime/VM.h:

2017-02-20 Commit Queue <commit-queue@webkit.org>

Unreviewed, rolling out r212618.
@@ -1,5 +1,5 @@
/*
* Copyright (C) 2009, 2013, 2016 Apple Inc. All rights reserved.
* Copyright (C) 2009-2017 Apple Inc. All rights reserved.
*
* Redistribution and use in source and binary forms, with or without
* modification, are permitted provided that the following conditions
@@ -33,10 +33,12 @@
#include "ProtoCallFrame.h"
#include "VMEntryScope.h"
#include "VMInlines.h"
#include <wtf/ForbidHeapAllocation.h>

namespace JSC {
class CachedCall {
WTF_MAKE_NONCOPYABLE(CachedCall); WTF_MAKE_FAST_ALLOCATED;
WTF_MAKE_NONCOPYABLE(CachedCall);
WTF_FORBID_HEAP_ALLOCATION;
public:
CachedCall(CallFrame* callFrame, JSFunction* function, int argumentCount)
: m_valid(false)
@@ -49,8 +51,8 @@ namespace JSC {

ASSERT(!function->isHostFunctionNonInline());
if (UNLIKELY(vm.isSafeToRecurseSoft())) {
m_arguments.resize(argumentCount);
m_closure = m_interpreter->prepareForRepeatCall(function->jsExecutable(), callFrame, &m_protoCallFrame, function, argumentCount + 1, function->scope(), m_arguments.data());
m_arguments.ensureCapacity(argumentCount);
m_closure = m_interpreter->prepareForRepeatCall(function->jsExecutable(), callFrame, &m_protoCallFrame, function, argumentCount + 1, function->scope(), m_arguments);
} else
throwStackOverflowError(callFrame, scope);
m_valid = !scope.exception();
@@ -59,18 +61,21 @@ namespace JSC {
JSValue call()
{
ASSERT(m_valid);
ASSERT(m_arguments.size() == static_cast<size_t>(m_protoCallFrame.argumentCount()));
return m_interpreter->execute(m_closure);
}
void setThis(JSValue v) { m_protoCallFrame.setThisValue(v); }
void setArgument(int n, JSValue v) { m_protoCallFrame.setArgument(n, v); }

void clearArguments() { m_arguments.clear(); }
void appendArgument(JSValue v) { m_arguments.append(v); }

private:
bool m_valid;
Interpreter* m_interpreter;
VM& m_vm;
VMEntryScope m_entryScope;
ProtoCallFrame m_protoCallFrame;
Vector<JSValue> m_arguments;
MarkedArgumentBuffer m_arguments;
CallFrameClosure m_closure;
};
}
@@ -1,7 +1,7 @@
/*
* Copyright (C) 1999-2001 Harri Porten (porten@kde.org)
* Copyright (C) 2001 Peter Kelly (pmk@post.com)
* Copyright (C) 2003, 2007-2008, 2011, 2013-2016 Apple Inc. All rights reserved.
* Copyright (C) 2003-2017 Apple Inc. All rights reserved.
*
* This library is free software; you can redistribute it and/or
* modify it under the terms of the GNU Library General Public
@@ -119,7 +119,7 @@ namespace JSC {

AtomicStringTable* atomicStringTable() const { return vm().atomicStringTable(); }
const CommonIdentifiers& propertyNames() const { return *vm().propertyNames; }
const MarkedArgumentBuffer& emptyList() const { return *vm().emptyList; }
const ArgList& emptyList() const { return *vm().emptyList; }
Interpreter* interpreter() { return vm().interpreter; }
Heap* heap() { return &vm().heap; }

@@ -1,5 +1,5 @@
/*
* Copyright (C) 2008-2010, 2012-2016 Apple Inc. All rights reserved.
* Copyright (C) 2008-2017 Apple Inc. All rights reserved.
* Copyright (C) 2008 Cameron Zwarich <cwzwarich@uwaterloo.ca>
*
* Redistribution and use in source and binary forms, with or without
@@ -1005,7 +1005,7 @@ JSObject* Interpreter::executeConstruct(CallFrame* callFrame, JSObject* construc
return checkedReturn(asObject(result));
}

CallFrameClosure Interpreter::prepareForRepeatCall(FunctionExecutable* functionExecutable, CallFrame* callFrame, ProtoCallFrame* protoCallFrame, JSFunction* function, int argumentCountIncludingThis, JSScope* scope, JSValue* args)
CallFrameClosure Interpreter::prepareForRepeatCall(FunctionExecutable* functionExecutable, CallFrame* callFrame, ProtoCallFrame* protoCallFrame, JSFunction* function, int argumentCountIncludingThis, JSScope* scope, const ArgList& args)
{
VM& vm = *scope->vm();
auto throwScope = DECLARE_THROW_SCOPE(vm);
@@ -1025,7 +1025,7 @@ CallFrameClosure Interpreter::prepareForRepeatCall(FunctionExecutable* functionE

size_t argsCount = argumentCountIncludingThis;

protoCallFrame->init(newCodeBlock, function, jsUndefined(), argsCount, args);
protoCallFrame->init(newCodeBlock, function, jsUndefined(), argsCount, args.data());
// Return the successful closure:
CallFrameClosure result = { callFrame, protoCallFrame, function, functionExecutable, &vm, scope, newCodeBlock->numParameters(), argumentCountIncludingThis };
return result;
@@ -1,5 +1,5 @@
/*
* Copyright (C) 2008, 2013, 2015-2016 Apple Inc. All rights reserved.
* Copyright (C) 2008-2017 Apple Inc. All rights reserved.
* Copyright (C) 2012 Research In Motion Limited. All rights reserved.
*
* Redistribution and use in source and binary forms, with or without
@@ -154,7 +154,7 @@ namespace JSC {
private:
enum ExecutionFlag { Normal, InitializeAndReturn };

CallFrameClosure prepareForRepeatCall(FunctionExecutable*, CallFrame*, ProtoCallFrame*, JSFunction*, int argumentCountIncludingThis, JSScope*, JSValue*);
CallFrameClosure prepareForRepeatCall(FunctionExecutable*, CallFrame*, ProtoCallFrame*, JSFunction*, int argumentCountIncludingThis, JSScope*, const ArgList&);

JSValue execute(CallFrameClosure&);

@@ -1,5 +1,5 @@
/*
* Copyright (C) 2013 Apple Inc. All Rights Reserved.
* Copyright (C) 2013-2017 Apple Inc. All Rights Reserved.
*
* Redistribution and use in source and binary forms, with or without
* modification, are permitted provided that the following conditions
@@ -26,10 +26,13 @@
#pragma once

#include "Register.h"
#include <wtf/ForbidHeapAllocation.h>

namespace JSC {

struct JS_EXPORT_PRIVATE ProtoCallFrame {
WTF_FORBID_HEAP_ALLOCATION;
public:
Register codeBlockValue;
Register calleeValue;
Register argCountAndCodeOriginValue;
@@ -1,5 +1,5 @@
/*
* Copyright (C) 2003, 2004, 2005, 2006, 2007, 2009, 2016 Apple Inc. All rights reserved.
* Copyright (C) 2003-2017 Apple Inc. All rights reserved.
*
* This library is free software; you can redistribute it and/or
* modify it under the terms of the GNU Library General Public
@@ -63,12 +63,24 @@ void MarkedArgumentBuffer::markLists(SlotVisitor& visitor, ListSet& markSet)
}
}

void MarkedArgumentBuffer::slowEnsureCapacity(size_t requestedCapacity)
{
int newCapacity = Checked<int>(requestedCapacity).unsafeGet();
expandCapacity(newCapacity);
}

void MarkedArgumentBuffer::expandCapacity()
{
int newCapacity = (Checked<int>(m_capacity) * 2).unsafeGet();
expandCapacity(newCapacity);
}

void MarkedArgumentBuffer::expandCapacity(int newCapacity)
{
ASSERT(m_capacity < newCapacity);
size_t size = (Checked<size_t>(newCapacity) * sizeof(EncodedJSValue)).unsafeGet();
EncodedJSValue* newBuffer = static_cast<EncodedJSValue*>(fastMalloc(size));
for (int i = 0; i < m_capacity; ++i) {
for (int i = 0; i < m_size; ++i) {
newBuffer[i] = m_buffer[i];
addMarkSet(JSValue::decode(m_buffer[i]));
}
@@ -82,7 +94,8 @@ void MarkedArgumentBuffer::expandCapacity()

void MarkedArgumentBuffer::slowAppend(JSValue v)
{
if (m_size >= m_capacity)
ASSERT(m_size <= m_capacity);
if (m_size == m_capacity)
expandCapacity();

slotFor(m_size) = JSValue::encode(v);
@@ -1,6 +1,6 @@
/*
* Copyright (C) 1999-2001 Harri Porten (porten@kde.org)
* Copyright (C) 2003, 2007, 2008, 2009, 2016 Apple Inc. All rights reserved.
* Copyright (C) 2003-2017 Apple Inc. All rights reserved.
*
* This library is free software; you can redistribute it and/or
* modify it under the terms of the GNU Library General Public
@@ -22,12 +22,14 @@
#pragma once

#include "CallFrame.h"
#include <wtf/ForbidHeapAllocation.h>
#include <wtf/HashSet.h>

namespace JSC {

class MarkedArgumentBuffer {
WTF_MAKE_NONCOPYABLE(MarkedArgumentBuffer);
WTF_FORBID_HEAP_ALLOCATION;
friend class VM;
friend class ArgList;

@@ -73,7 +75,8 @@ class MarkedArgumentBuffer {

void append(JSValue v)
{
if (m_size >= m_capacity || mallocBase())
ASSERT(m_size <= m_capacity);
if (m_size == m_capacity || mallocBase())
return slowAppend(v);

slotFor(m_size) = JSValue::encode(v);
@@ -94,8 +97,16 @@ class MarkedArgumentBuffer {

static void markLists(SlotVisitor&, ListSet&);

void ensureCapacity(size_t requestedCapacity)
{
if (requestedCapacity > static_cast<size_t>(m_capacity))
slowEnsureCapacity(requestedCapacity);
}

private:
void expandCapacity();
void expandCapacity(int newCapacity);
void slowEnsureCapacity(size_t requestedCapacity);

void addMarkSet(JSValue);

@@ -1,6 +1,6 @@
/*
* Copyright (C) 1999-2001 Harri Porten (porten@kde.org)
* Copyright (C) 2004-2008, 2013, 2016 Apple Inc. All rights reserved.
* Copyright (C) 2004-2017 Apple Inc. All rights reserved.
* Copyright (C) 2009 Torch Mobile, Inc.
* Copyright (C) 2015 Jordan Harband (ljharb@gmail.com)
*
@@ -539,18 +539,19 @@ static ALWAYS_INLINE EncodedJSValue replaceUsingRegExpSearch(
OUT_OF_MEMORY(exec, scope);

unsigned i = 0;
cachedCall.clearArguments();
for (; i < regExp->numSubpatterns() + 1; ++i) {
int matchStart = ovector[i * 2];
int matchLen = ovector[i * 2 + 1] - matchStart;

if (matchStart < 0)
cachedCall.setArgument(i, jsUndefined());
cachedCall.appendArgument(jsUndefined());
else
cachedCall.setArgument(i, jsSubstring(&vm, source, matchStart, matchLen));
cachedCall.appendArgument(jsSubstring(&vm, source, matchStart, matchLen));
}

cachedCall.setArgument(i++, jsNumber(result.start));
cachedCall.setArgument(i++, string);
cachedCall.appendArgument(jsNumber(result.start));
cachedCall.appendArgument(string);

cachedCall.setThis(jsUndefined());
JSValue jsResult = cachedCall.call();
@@ -578,18 +579,19 @@ static ALWAYS_INLINE EncodedJSValue replaceUsingRegExpSearch(
OUT_OF_MEMORY(exec, scope);

unsigned i = 0;
cachedCall.clearArguments();
for (; i < regExp->numSubpatterns() + 1; ++i) {
int matchStart = ovector[i * 2];
int matchLen = ovector[i * 2 + 1] - matchStart;

if (matchStart < 0)
cachedCall.setArgument(i, jsUndefined());
cachedCall.appendArgument(jsUndefined());
else
cachedCall.setArgument(i, jsSubstring(&vm, source, matchStart, matchLen));
cachedCall.appendArgument(jsSubstring(&vm, source, matchStart, matchLen));
}

cachedCall.setArgument(i++, jsNumber(result.start));
cachedCall.setArgument(i++, string);
cachedCall.appendArgument(jsNumber(result.start));
cachedCall.appendArgument(string);

cachedCall.setThis(jsUndefined());
JSValue jsResult = cachedCall.call();
@@ -180,7 +180,7 @@ VM::VM(VMType vmType, HeapType heapType)
, topJSWebAssemblyInstance(nullptr)
, m_atomicStringTable(vmType == Default ? wtfThreadData().atomicStringTable() : new AtomicStringTable)
, propertyNames(nullptr)
, emptyList(new MarkedArgumentBuffer)
, emptyList(new ArgList)
, machineCodeBytesPerBytecodeWordForBaselineJIT(std::make_unique<SimpleStats>())
, customGetterSetterFunctionMap(*this)
, stringCache(*this)
@@ -385,7 +385,7 @@ class VM : public ThreadSafeRefCounted<VM>, public DoublyLinkedListNode<VM> {
WTF::SymbolRegistry m_symbolRegistry;
TemplateRegistryKeyTable m_templateRegistryKeytable;
CommonIdentifiers* propertyNames;
const MarkedArgumentBuffer* emptyList; // Lists are supposed to be allocated on the stack to have their elements properly marked, which is not the case here - but this list has nothing to mark.
const ArgList* emptyList;
SmallStrings smallStrings;
NumericStrings numericStrings;
DateInstanceCache dateInstanceCache;
@@ -1,3 +1,22 @@
2017-02-20 Mark Lam <mark.lam@apple.com>

[Re-landing] CachedCall should let GC know to keep its arguments alive.
https://bugs.webkit.org/show_bug.cgi?id=168567
<rdar://problem/30475767>

Reviewed by Saam Barati.

Added a WTF_FORBID_HEAP_ALLOCATION that will cause a compilation failure if
a class declared with it is malloced.

While this doesn't prevent that class declared WTF_FORBID_HEAP_ALLOCATION from
being embedded in another class that is heap allocated, it does at minimum
document the intent and gives the users of this class a chance to do the
right thing.

* WTF.xcodeproj/project.pbxproj:
* wtf/ForbidHeapAllocation.h: Added.

2017-02-20 Ryan Haddad <ryanhaddad@apple.com>

Unreviewed, rolling out r212685.

0 comments on commit 7d1b3b9

Please sign in to comment.
You can’t perform that action at this time.