Summary: | [JSC] JSON.stringify should handle Proxy which is non JSArray but isArray is true | ||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Kamil Frankowicz <kamil.frankowicz> | ||||||||
Component: | JavaScriptCore | Assignee: | Yusuke Suzuki <ysuzuki> | ||||||||
Status: | RESOLVED FIXED | ||||||||||
Severity: | Normal | CC: | darin, fpizlo, ggaren, jfbastien, keith_miller, mark.lam, msaboff, saam, webkit-bug-importer, ysuzuki | ||||||||
Priority: | P2 | Keywords: | InRadar | ||||||||
Version: | WebKit Nightly Build | ||||||||||
Hardware: | Unspecified | ||||||||||
OS: | Unspecified | ||||||||||
Attachments: |
|
More easy way is `JSON.parse(new Proxy([undefined], {}));`. investigating. s/parse/stringify/ Created attachment 293328 [details]
Patch
Comment on attachment 293328 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=293328&action=review > Source/JavaScriptCore/ChangeLog:11 > + When JSON.stringify encounter the value like undefined, which results depend > + on the context. If it is produced under the object property context, we ignore > + that property. On the other hand, if it is produced under the array element > + context, we produce "null". Can you quote the relevant urls to the spec here? > Source/JavaScriptCore/runtime/JSONObject.cpp:265 > + Holder root(Holder::RootHolder, vm, object); Why can't you just use: Holder root(vm, m_exec, object); > Source/JavaScriptCore/runtime/JSONObject.cpp:426 > + , m_isArray(JSC::isArray(exec, object)) m_isJSArray is uninitialized here. Is that intentional? > Source/JavaScriptCore/runtime/JSONObject.cpp:440 > +inline Stringifier::Holder::Holder(RootHolderTag, VM& vm, JSObject* object) > + : m_object(vm, object) > + , m_isArray(false) > + , m_isJSArray(false) > , m_index(0) > #ifndef NDEBUG > , m_size(0) Why do we need this version? Won't the Holder(VM& vm, ExecState* exec, JSObject* object) work just as well? Comment on attachment 293328 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=293328&action=review Thanks! >> Source/JavaScriptCore/ChangeLog:11 >> + context, we produce "null". > > Can you quote the relevant urls to the spec here? Yup! I'll include it. https://tc39.github.io/ecma262/#sec-serializejsonarray we write "null". But https://tc39.github.io/ecma262/#sec-serializejsonobject, we skip the property in 8-b. >> Source/JavaScriptCore/runtime/JSONObject.cpp:265 >> + Holder root(Holder::RootHolder, vm, object); > > Why can't you just use: > Holder root(vm, m_exec, object); I don't think using it directly is good. It calls `isArray` for the given object and isArray() could raise exceptions for some objects. Of course, here, the exception never happens. But I would like to explicitly say that this Holder does not cause any observable behaviors. That's why I did not include ExecState* in this constructor. >> Source/JavaScriptCore/runtime/JSONObject.cpp:426 >> + , m_isArray(JSC::isArray(exec, object)) > > m_isJSArray is uninitialized here. Is that intentional? Yes, that is intentional. And this function is original one and it is not changed. This m_isJSArray will be initialized when we call appendNextProperty for this holder. >> Source/JavaScriptCore/runtime/JSONObject.cpp:440 >> , m_size(0) > > Why do we need this version? Won't the Holder(VM& vm, ExecState* exec, JSObject* object) work just as well? Yeah, it works. But I don't think it is good manner. And since we do not call appendNextProperty for this root Holder, some of the fields (m_isJSArray) remains uninitialized. I don't think it is good thing. Notable example is, JSON.stringify({ value: undefined }) => "{}" JSON.stringify([undefined]) => "[null]" Comment on attachment 293328 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=293328&action=review >>> Source/JavaScriptCore/runtime/JSONObject.cpp:426 >>> + , m_isArray(JSC::isArray(exec, object)) >> >> m_isJSArray is uninitialized here. Is that intentional? > > Yes, that is intentional. And this function is original one and it is not changed. > This m_isJSArray will be initialized when we call appendNextProperty for this holder. But, I think initializing it is better. Since isJSArray() is not observable one, we can just call it here. I'll change this part. Created attachment 293335 [details]
Patch
Comment on attachment 293335 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=293335&action=review r=me > Source/JavaScriptCore/ChangeLog:8 > + When JSON.stringify encounter the value like undefined, which results depend /the value like undefined/the undefined value/ /which results depend/the result depends/ > Source/JavaScriptCore/ChangeLog:14 > + // https://tc39.github.io/ecma262/#sec-serializejsonobject I suggest adding "section 8.b" after the spec url above. > Source/JavaScriptCore/ChangeLog:18 > + // https://tc39.github.io/ecma262/#sec-serializejsonarray I suggest adding "section 8.b" after the spec url above. > JSTests/stress/json-stringify-with-non-jsarray-array.js:24 > +shouldBe(JSON.stringify(new Proxy([function() { }], {})), `[null]`); Can you also add the test case for JSON.stringify(new Proxy({ value: undefined }, {}))? Comment on attachment 293335 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=293335&action=review >> Source/JavaScriptCore/ChangeLog:8 >> + When JSON.stringify encounter the value like undefined, which results depend > > /the value like undefined/the undefined value/ > /which results depend/the result depends/ Oops, fixed. >> Source/JavaScriptCore/ChangeLog:14 >> + // https://tc39.github.io/ecma262/#sec-serializejsonobject > > I suggest adding "section 8.b" after the spec url above. Yeah, nice. Fixed. >> Source/JavaScriptCore/ChangeLog:18 >> + // https://tc39.github.io/ecma262/#sec-serializejsonarray > > I suggest adding "section 8.b" after the spec url above. Fixed. >> JSTests/stress/json-stringify-with-non-jsarray-array.js:24 >> +shouldBe(JSON.stringify(new Proxy([function() { }], {})), `[null]`); > > Can you also add the test case for JSON.stringify(new Proxy({ value: undefined }, {}))? Yup, added. Committed r208123: <http://trac.webkit.org/changeset/208123> Comment on attachment 293335 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=293335&action=review > Source/JavaScriptCore/runtime/JSONObject.cpp:431 > + , m_index(0) > +#ifndef NDEBUG > + , m_size(0) > +#endif It would be better to initialize these in the class definition. > Source/JavaScriptCore/runtime/JSONObject.cpp:441 > + , m_isArray(false) > + , m_isJSArray(false) > , m_index(0) > #ifndef NDEBUG > , m_size(0) It would be better to initialize these in the class definition. Comment on attachment 293335 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=293335&action=review Thanks, I'll create a follow-up patch. >> Source/JavaScriptCore/runtime/JSONObject.cpp:431 >> +#endif > > It would be better to initialize these in the class definition. Sounds nice. I'll move this function to the class definition. And use default initialization for some members (m_index for example). >> Source/JavaScriptCore/runtime/JSONObject.cpp:441 >> , m_size(0) > > It would be better to initialize these in the class definition. Ditto. This is CVE-2016-10222. |
Created attachment 293147 [details] POC to trigger SEGFAULT (jsc) Affected SVN revision: 208042 To reproduce the problem: ./jsc wekbit_string_builder.js ASAN Output: ASAN:DEADLYSIGNAL ================================================================= ==16295==ERROR: AddressSanitizer: SEGV on unknown address 0x6041000021a3 (pc 0x7fa5adf38ee4 bp 0x0000ffffffff sp 0x7ffd4d2f6790 T0) ==16295==The signal is caused by a READ memory access. #0 0x7fa5adf38ee3 in WTF::StringBuilder::operator[](unsigned int) const XYZ/webkit/Source/WTF/wtf/text/StringBuilder.h:247:20 #1 0x7fa5adf38ee3 in JSC::Stringifier::Holder::appendNextProperty(JSC::Stringifier&, WTF::StringBuilder&) XYZ/webkit/Source/JavaScriptCore/runtime/JSONObject.cpp:465 #2 0x7fa5adf35501 in JSC::Stringifier::appendStringifiedValue(WTF::StringBuilder&, JSC::JSValue, JSC::JSObject*, JSC::PropertyNameForFunctionCall const&) XYZ/webkit/Source/JavaScriptCore/runtime/JSONObject.cpp:384:37 #3 0x7fa5adf31deb in JSC::Stringifier::stringify(JSC::Handle<JSC::Unknown>) XYZ/webkit/Source/JavaScriptCore/runtime/JSONObject.cpp:262:9 #4 0x7fa5adf406c4 in JSC::JSONProtoFuncStringify(JSC::ExecState*) XYZ/webkit/Source/JavaScriptCore/runtime/JSONObject.cpp:786:57 #5 0x7fa565afe027 (<unknown module>) AddressSanitizer can not provide additional info. SUMMARY: AddressSanitizer: SEGV XYZ/webkit/Source/WTF/wtf/text/StringBuilder.h:247:20 in WTF::StringBuilder::operator[](unsigned int) const ==16295==ABORTING Regards, Kamil Frankowicz