Summary: | JSONParse should not crash with null Strings | ||||||
---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Alex Christensen <achristensen> | ||||
Component: | New Bugs | Assignee: | Alex Christensen <achristensen> | ||||
Status: | RESOLVED FIXED | ||||||
Severity: | Normal | CC: | youennf | ||||
Priority: | P2 | ||||||
Version: | WebKit Nightly Build | ||||||
Hardware: | Unspecified | ||||||
OS: | Unspecified | ||||||
Attachments: |
|
Description
Alex Christensen
2016-10-24 15:46:46 PDT
Created attachment 292670 [details]
Patch
Comment on attachment 292670 [details]
Patch
r=me
Comment on attachment 292670 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=292670&action=review > Source/JavaScriptCore/ChangeLog:11 > + When JSONParse is called with a null String, it calls String::is8bit, which dereferences a null pointer. > + This is happening with new work in the Fetch API, but callers of JSONParse should not have to check > + if the String is null. I am not sure I agree. It’s not surprising that the Fetch API doesn’t get exactly what it wants when calling an internal JavaScript runtime function. We can export a different function with appropriate helpful semantics for use in Fetch. > Source/JavaScriptCore/API/tests/JSONParseTest.cpp:53 > + JSValue v0 = JSONParse(exec, ""); > + JSValue v1 = JSONParse(exec, "#$%^"); > + JSValue v2 = JSONParse(exec, String()); > + UChar emptyUCharArray[1] = { '\0' }; > + JSValue v3 = JSONParse(exec, String(emptyUCharArray, 0)); > + JSValue v4; > + JSValue v5 = JSONParse(exec, "123"); This test is looking at return values and not at whether an exception is thrown. That’s the reason the test seems to indicate that empty string and null string have the same result here but I am pretty sure they don’t, because the empty string case throws an exception and the null string case does not. > Source/JavaScriptCore/runtime/JSONObject.cpp:795 > + if (json.isNull()) > + return JSValue(); We don’t use null strings in the JavaScript engine, so normally we would not need to check for them. It’s also not normal for a JavaScript function to return an empty JSValue unless it has thrown an exception. I don’t think this change is a good idea. Yep, JSON.parse("") throws an exception. Something needs to change. I'll have to look into exactly what was going wrong with the fetch API in this case and whether it should throw an exception. We were using fulfillPromiseWithJSON If this is only related to fetch, we should probably assert that string is not null in fulfillPromiseWithJSON and fix fetch code wherever needed. A fetch body can be null, but we should not end up with a null string body. Empty string fetch bodies are possible on the other hand. |