Skip to content

Commit 98c0645

Browse files
author
Gavin Barraclough
committed
JSCallbackFunction::toStringCallback/valueOfCallback do not handle 0 return value from convertToType
https://bugs.webkit.org/show_bug.cgi?id=81468 <rdar://problem/11034745> Reviewed by Oliver Hunt. The API specifies that convertToType may opt not to handle a conversion: "@Result The objects's converted value, or NULL if the object was not converted." In which case, it would propagate first up the JSClass hierarchy, calling its superclass's conversion functions, and failing that call the JSObject::defaultValue function. Unfortunately this behaviour was removed in bug#69677/bug#69858, and instead we now rely on the toStringCallback/valueOfCallback function introduced in bug#69156. Even after a fix in bug#73368, these will return the result from the first convertToType they find, regardless of whether this result is null, and if no convertToType method is found in the api class hierarchy (possible if toStringCallback/valueOfCallback was accessed off the prototype chain), they will also return a null pointer. This is unsafe. It would be easy to make the approach based around toStringCallback/valueOfCallback continue to walk the api class hierarchy, but making the fallback to defaultValue would be problematic (since defaultValue calls toStringCallback/valueOfCallback, this would infinitely recurse). Making the fallback work with toString/valueOf methods attached to api objects is probably not the right thing to do – instead, we should just implement the defaultValue trap for api objects. In addition, this bug highlights that fact that JSCallbackFunction::call will allow a hard null to be returned from C to JavaScript - this is not okay. Handle with an exception. * API/JSCallbackFunction.cpp: (JSC::JSCallbackFunction::call): - Should be null checking the return value. (JSC): - Remove toStringCallback/valueOfCallback. * API/JSCallbackFunction.h: (JSCallbackFunction): - Remove toStringCallback/valueOfCallback. * API/JSCallbackObject.h: (JSCallbackObject): - Add defaultValue mthods to JSCallbackObject. * API/JSCallbackObjectFunctions.h: (JSC::::defaultValue): - Add defaultValue mthods to JSCallbackObject. * API/JSClassRef.cpp: (OpaqueJSClass::prototype): - Remove toStringCallback/valueOfCallback. * API/tests/testapi.js: - Revert this test, now we no longer artificially introduce a toString method onto the api object. Canonical link: https://commits.webkit.org/98706@main git-svn-id: https://svn.webkit.org/repository/webkit/trunk@111162 268f45cc-cd09-0410-ab3c-d52691b4dbfc
1 parent 7792ca5 commit 98c0645

10 files changed

Lines changed: 149 additions & 78 deletions

File tree

‎Source/JavaScriptCore/API/JSCallbackFunction.cpp‎

Lines changed: 4 additions & 32 deletions
Original file line numberDiff line numberDiff line change
@@ -30,7 +30,6 @@
3030
#include "APICast.h"
3131
#include "CodeBlock.h"
3232
#include "ExceptionHelpers.h"
33-
#include "JSCallbackObject.h"
3433
#include "JSFunction.h"
3534
#include "FunctionPrototype.h"
3635
#include <runtime/JSGlobalObject.h>
@@ -76,6 +75,10 @@ EncodedJSValue JSCallbackFunction::call(ExecState* exec)
7675
if (exception)
7776
throwError(exec, toJS(exec, exception));
7877

78+
// result must be a valid JSValue.
79+
if (!result)
80+
return throwVMTypeError(exec);
81+
7982
return JSValue::encode(toJS(exec, result));
8083
}
8184

@@ -85,35 +88,4 @@ CallType JSCallbackFunction::getCallData(JSCell*, CallData& callData)
8588
return CallTypeHost;
8689
}
8790

88-
JSValueRef JSCallbackFunction::toStringCallback(JSContextRef ctx, JSObjectRef, JSObjectRef thisObject, size_t, const JSValueRef[], JSValueRef* exception)
89-
{
90-
JSObject* object = toJS(thisObject);
91-
if (object->inherits(&JSCallbackObject<JSNonFinalObject>::s_info)) {
92-
for (JSClassRef jsClass = jsCast<JSCallbackObject<JSNonFinalObject>*>(object)->classRef(); jsClass; jsClass = jsClass->parentClass)
93-
if (jsClass->convertToType)
94-
return jsClass->convertToType(ctx, thisObject, kJSTypeString, exception);
95-
} else if (object->inherits(&JSCallbackObject<JSGlobalObject>::s_info)) {
96-
for (JSClassRef jsClass = jsCast<JSCallbackObject<JSGlobalObject>*>(object)->classRef(); jsClass; jsClass = jsClass->parentClass)
97-
if (jsClass->convertToType)
98-
return jsClass->convertToType(ctx, thisObject, kJSTypeString, exception);
99-
}
100-
return 0;
101-
}
102-
103-
JSValueRef JSCallbackFunction::valueOfCallback(JSContextRef ctx, JSObjectRef, JSObjectRef thisObject, size_t, const JSValueRef[], JSValueRef* exception)
104-
{
105-
JSObject* object = toJS(thisObject);
106-
if (object->inherits(&JSCallbackObject<JSNonFinalObject>::s_info)) {
107-
for (JSClassRef jsClass = jsCast<JSCallbackObject<JSNonFinalObject>*>(object)->classRef(); jsClass; jsClass = jsClass->parentClass)
108-
if (jsClass->convertToType)
109-
return jsClass->convertToType(ctx, thisObject, kJSTypeNumber, exception);
110-
} else if (object->inherits(&JSCallbackObject<JSGlobalObject>::s_info)) {
111-
for (JSClassRef jsClass = jsCast<JSCallbackObject<JSGlobalObject>*>(object)->classRef(); jsClass; jsClass = jsClass->parentClass)
112-
if (jsClass->convertToType)
113-
return jsClass->convertToType(ctx, thisObject, kJSTypeNumber, exception);
114-
}
115-
return 0;
116-
}
117-
118-
11991
} // namespace JSC

‎Source/JavaScriptCore/API/JSCallbackFunction.h‎

Lines changed: 0 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -55,9 +55,6 @@ class JSCallbackFunction : public InternalFunction {
5555
return Structure::create(globalData, globalObject, proto, TypeInfo(ObjectType, StructureFlags), &s_info);
5656
}
5757

58-
static JSValueRef toStringCallback(JSContextRef ctx, JSObjectRef function, JSObjectRef thisObject, size_t argumentCount, const JSValueRef arguments[], JSValueRef* exception);
59-
static JSValueRef valueOfCallback(JSContextRef ctx, JSObjectRef function, JSObjectRef thisObject, size_t argumentCount, const JSValueRef arguments[], JSValueRef* exception);
60-
6158
private:
6259
static CallType getCallData(JSCell*, CallData&);
6360

‎Source/JavaScriptCore/API/JSCallbackObject.h‎

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -175,6 +175,8 @@ class JSCallbackObject : public Parent {
175175

176176
static void destroy(JSCell*);
177177

178+
static JSValue defaultValue(const JSObject*, ExecState*, PreferredPrimitiveType);
179+
178180
static bool getOwnPropertySlot(JSCell*, ExecState*, const Identifier&, PropertySlot&);
179181
static bool getOwnPropertyDescriptor(JSObject*, ExecState*, const Identifier&, PropertyDescriptor&);
180182

‎Source/JavaScriptCore/API/JSCallbackObjectFunctions.h‎

Lines changed: 24 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -183,6 +183,30 @@ bool JSCallbackObject<Parent>::getOwnPropertySlot(JSCell* cell, ExecState* exec,
183183
return Parent::getOwnPropertySlot(thisObject, exec, propertyName, slot);
184184
}
185185

186+
template <class Parent>
187+
JSValue JSCallbackObject<Parent>::defaultValue(const JSObject* object, ExecState* exec, PreferredPrimitiveType hint)
188+
{
189+
const JSCallbackObject* thisObject = jsCast<const JSCallbackObject*>(object);
190+
JSContextRef ctx = toRef(exec);
191+
JSObjectRef thisRef = toRef(thisObject);
192+
::JSType jsHint = hint == PreferString ? kJSTypeString : kJSTypeNumber;
193+
194+
for (JSClassRef jsClass = thisObject->classRef(); jsClass; jsClass = jsClass->parentClass) {
195+
if (JSObjectConvertToTypeCallback convertToType = jsClass->convertToType) {
196+
JSValueRef exception = 0;
197+
JSValueRef result = convertToType(ctx, thisRef, jsHint, &exception);
198+
if (exception) {
199+
throwError(exec, toJS(exec, exception));
200+
return jsUndefined();
201+
}
202+
if (result)
203+
return toJS(exec, result);
204+
}
205+
}
206+
207+
return Parent::defaultValue(object, exec, hint);
208+
}
209+
186210
template <class Parent>
187211
bool JSCallbackObject<Parent>::getOwnPropertyDescriptor(JSObject* object, ExecState* exec, const Identifier& propertyName, PropertyDescriptor& descriptor)
188212
{

‎Source/JavaScriptCore/API/JSClassRef.cpp‎

Lines changed: 0 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -204,17 +204,6 @@ JSObject* OpaqueJSClass::prototype(ExecState* exec)
204204
* DerivedClass | DerivedClassPrototype
205205
*/
206206

207-
if (convertToType) {
208-
if (!prototypeClass)
209-
prototypeClass = OpaqueJSClass::create(&kJSClassDefinitionEmpty).leakRef();
210-
if (!prototypeClass->m_staticFunctions)
211-
prototypeClass->m_staticFunctions = adoptPtr(new OpaqueJSClassStaticFunctionsTable);
212-
const Identifier& toString = exec->propertyNames().toString;
213-
const Identifier& valueOf = exec->propertyNames().valueOf;
214-
prototypeClass->m_staticFunctions->add(StringImpl::create(toString.characters(), toString.length()), adoptPtr(new StaticFunctionEntry(&JSCallbackFunction::toStringCallback, 0)));
215-
prototypeClass->m_staticFunctions->add(StringImpl::create(valueOf.characters(), valueOf.length()), adoptPtr(new StaticFunctionEntry(&JSCallbackFunction::valueOfCallback, 0)));
216-
}
217-
218207
if (!prototypeClass)
219208
return 0;
220209

‎Source/JavaScriptCore/API/tests/testapi.c‎

Lines changed: 64 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -311,6 +311,16 @@ static JSValueRef MyObject_convertToType(JSContextRef context, JSObjectRef objec
311311
return JSValueMakeNull(context);
312312
}
313313

314+
static JSValueRef MyObject_convertToTypeWrapper(JSContextRef context, JSObjectRef object, JSType type, JSValueRef* exception)
315+
{
316+
UNUSED_PARAM(context);
317+
UNUSED_PARAM(object);
318+
UNUSED_PARAM(type);
319+
UNUSED_PARAM(exception);
320+
// Forward to default object class
321+
return 0;
322+
}
323+
314324
static bool MyObject_set_nullGetForwardSet(JSContextRef ctx, JSObjectRef object, JSStringRef propertyName, JSValueRef value, JSValueRef* exception)
315325
{
316326
UNUSED_PARAM(ctx);
@@ -355,14 +365,65 @@ JSClassDefinition MyObject_definition = {
355365
MyObject_convertToType,
356366
};
357367

368+
JSClassDefinition MyObject_convertToTypeWrapperDefinition = {
369+
0,
370+
kJSClassAttributeNone,
371+
372+
"MyObject",
373+
NULL,
374+
375+
NULL,
376+
NULL,
377+
378+
NULL,
379+
NULL,
380+
NULL,
381+
NULL,
382+
NULL,
383+
NULL,
384+
NULL,
385+
NULL,
386+
NULL,
387+
NULL,
388+
MyObject_convertToTypeWrapper,
389+
};
390+
391+
JSClassDefinition MyObject_nullWrapperDefinition = {
392+
0,
393+
kJSClassAttributeNone,
394+
395+
"MyObject",
396+
NULL,
397+
398+
NULL,
399+
NULL,
400+
401+
NULL,
402+
NULL,
403+
NULL,
404+
NULL,
405+
NULL,
406+
NULL,
407+
NULL,
408+
NULL,
409+
NULL,
410+
NULL,
411+
NULL,
412+
};
413+
358414
static JSClassRef MyObject_class(JSContextRef context)
359415
{
360416
UNUSED_PARAM(context);
361417

362418
static JSClassRef jsClass;
363-
if (!jsClass)
364-
jsClass = JSClassCreate(&MyObject_definition);
365-
419+
if (!jsClass) {
420+
JSClassRef baseClass = JSClassCreate(&MyObject_definition);
421+
MyObject_convertToTypeWrapperDefinition.parentClass = baseClass;
422+
JSClassRef wrapperClass = JSClassCreate(&MyObject_convertToTypeWrapperDefinition);
423+
MyObject_nullWrapperDefinition.parentClass = wrapperClass;
424+
jsClass = JSClassCreate(&MyObject_nullWrapperDefinition);
425+
}
426+
366427
return jsClass;
367428
}
368429

‎Source/JavaScriptCore/API/tests/testapi.js‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -155,7 +155,7 @@ shouldBe("typeof myObject", "object");
155155
shouldBe("MyObject ? 1 : 0", true); // toBoolean
156156
shouldBe("+MyObject", 1); // toNumber
157157
shouldBe("(Object.prototype.toString.call(MyObject))", "[object MyObject]"); // Object.prototype.toString
158-
shouldBe("(MyObject.toString())", "MyObjectAsString"); // toString
158+
shouldBe("(MyObject.toString())", "[object MyObject]"); // toString
159159
shouldBe("String(MyObject)", "MyObjectAsString"); // toString
160160
shouldBe("MyObject - 0", 1); // toNumber
161161
shouldBe("MyObject.valueOf()", 1); // valueOf

‎Source/JavaScriptCore/ChangeLog‎

Lines changed: 49 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,3 +1,52 @@
1+
2012-03-19 Gavin Barraclough <barraclough@apple.com>
2+
3+
JSCallbackFunction::toStringCallback/valueOfCallback do not handle 0 return value from convertToType
4+
https://bugs.webkit.org/show_bug.cgi?id=81468 <rdar://problem/11034745>
5+
6+
Reviewed by Oliver Hunt.
7+
8+
The API specifies that convertToType may opt not to handle a conversion:
9+
"@result The objects's converted value, or NULL if the object was not converted."
10+
In which case, it would propagate first up the JSClass hierarchy, calling its superclass's
11+
conversion functions, and failing that call the JSObject::defaultValue function.
12+
13+
Unfortunately this behaviour was removed in bug#69677/bug#69858, and instead we now rely on
14+
the toStringCallback/valueOfCallback function introduced in bug#69156. Even after a fix in
15+
bug#73368, these will return the result from the first convertToType they find, regardless
16+
of whether this result is null, and if no convertToType method is found in the api class
17+
hierarchy (possible if toStringCallback/valueOfCallback was accessed off the prototype
18+
chain), they will also return a null pointer. This is unsafe.
19+
20+
It would be easy to make the approach based around toStringCallback/valueOfCallback continue
21+
to walk the api class hierarchy, but making the fallback to defaultValue would be problematic
22+
(since defaultValue calls toStringCallback/valueOfCallback, this would infinitely recurse).
23+
Making the fallback work with toString/valueOf methods attached to api objects is probably
24+
not the right thing to do – instead, we should just implement the defaultValue trap for api
25+
objects.
26+
27+
In addition, this bug highlights that fact that JSCallbackFunction::call will allow a hard
28+
null to be returned from C to JavaScript - this is not okay. Handle with an exception.
29+
30+
* API/JSCallbackFunction.cpp:
31+
(JSC::JSCallbackFunction::call):
32+
- Should be null checking the return value.
33+
(JSC):
34+
- Remove toStringCallback/valueOfCallback.
35+
* API/JSCallbackFunction.h:
36+
(JSCallbackFunction):
37+
- Remove toStringCallback/valueOfCallback.
38+
* API/JSCallbackObject.h:
39+
(JSCallbackObject):
40+
- Add defaultValue mthods to JSCallbackObject.
41+
* API/JSCallbackObjectFunctions.h:
42+
(JSC::::defaultValue):
43+
- Add defaultValue mthods to JSCallbackObject.
44+
* API/JSClassRef.cpp:
45+
(OpaqueJSClass::prototype):
46+
- Remove toStringCallback/valueOfCallback.
47+
* API/tests/testapi.js:
48+
- Revert this test, now we no longer artificially introduce a toString method onto the api object.
49+
150
2012-03-18 Raphael Kubo da Costa <rakuco@FreeBSD.org>
251

352
[EFL] Include ICU_INCLUDE_DIRS when building.

‎Source/JavaScriptCore/runtime/Arguments.cpp‎

Lines changed: 5 additions & 23 deletions
Original file line numberDiff line numberDiff line change
@@ -306,6 +306,7 @@ bool Arguments::defineOwnProperty(JSObject* object, ExecState* exec, const Ident
306306
bool isArrayIndex;
307307
unsigned i = propertyName.toArrayIndex(isArrayIndex);
308308
if (isArrayIndex && i < thisObject->d->numArguments) {
309+
object->putDirect(exec->globalData(), propertyName, thisObject->argument(i).get(), 0);
309310
if (!Base::defineOwnProperty(object, exec, propertyName, descriptor, shouldThrow))
310311
return false;
311312

@@ -331,35 +332,16 @@ bool Arguments::defineOwnProperty(JSObject* object, ExecState* exec, const Ident
331332
thisObject->d->deletedArguments[i] = true;
332333
}
333334
}
334-
335335
return true;
336336
}
337337

338338
if (propertyName == exec->propertyNames().length && !thisObject->d->overrodeLength) {
339+
thisObject->putDirect(exec->globalData(), propertyName, jsNumber(thisObject->d->numArguments), DontEnum);
339340
thisObject->d->overrodeLength = true;
340-
if (!descriptor.isAccessorDescriptor()) {
341-
if (!descriptor.value())
342-
descriptor.setValue(jsNumber(thisObject->d->numArguments));
343-
if (!descriptor.configurablePresent())
344-
descriptor.setConfigurable(true);
345-
}
346-
if (!descriptor.configurablePresent())
347-
descriptor.setConfigurable(true);
348-
}
349-
350-
if (propertyName == exec->propertyNames().callee && !thisObject->d->overrodeCallee) {
341+
} else if (propertyName == exec->propertyNames().callee && !thisObject->d->overrodeCallee) {
342+
thisObject->putDirect(exec->globalData(), propertyName, thisObject->d->callee.get(), DontEnum);
351343
thisObject->d->overrodeCallee = true;
352-
if (!descriptor.isAccessorDescriptor()) {
353-
if (!descriptor.value())
354-
descriptor.setValue(thisObject->d->callee.get());
355-
if (!descriptor.configurablePresent())
356-
descriptor.setConfigurable(true);
357-
}
358-
if (!descriptor.configurablePresent())
359-
descriptor.setConfigurable(true);
360-
}
361-
362-
if (propertyName == exec->propertyNames().caller && thisObject->d->isStrictMode)
344+
} else if (propertyName == exec->propertyNames().caller && thisObject->d->isStrictMode)
363345
thisObject->createStrictModeCallerIfNecessary(exec);
364346

365347
return Base::defineOwnProperty(object, exec, propertyName, descriptor, shouldThrow);

‎Source/JavaScriptCore/runtime/JSFunction.cpp‎

Lines changed: 0 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -371,11 +371,6 @@ bool JSFunction::defineOwnProperty(JSObject* object, ExecState* exec, const Iden
371371
PropertySlot slot;
372372
thisObject->methodTable()->getOwnPropertySlot(thisObject, exec, propertyName, slot);
373373
} else if (propertyName == exec->propertyNames().arguments || propertyName == exec->propertyNames().length || propertyName == exec->propertyNames().caller) {
374-
if (!object->isExtensible()) {
375-
if (throwException)
376-
throwError(exec, createTypeError(exec, "Attempting to define property on object that is not extensible."));
377-
return false;
378-
}
379374
if (descriptor.configurablePresent() && descriptor.configurable()) {
380375
if (throwException)
381376
throwError(exec, createTypeError(exec, "Attempting to configurable attribute of unconfigurable property."));

0 commit comments

Comments
 (0)