Bug 163918 - JSONParse should not crash with null Strings
Summary: JSONParse should not crash with null Strings
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Alex Christensen
Depends on:
Reported: 2016-10-24 15:46 PDT by Alex Christensen
Modified: 2016-10-27 02:06 PDT (History)
1 user (show)

See Also:

Patch (12.26 KB, patch)
2016-10-24 15:50 PDT, Alex Christensen
msaboff: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Alex Christensen 2016-10-24 15:46:46 PDT
JSONParse should not crash with null Strings
Comment 1 Alex Christensen 2016-10-24 15:50:36 PDT
Created attachment 292670 [details]
Comment 2 Michael Saboff 2016-10-24 15:54:05 PDT
Comment on attachment 292670 [details]

Comment 3 Alex Christensen 2016-10-24 15:55:00 PDT
Comment 4 Darin Adler 2016-10-24 22:42:16 PDT
Comment on attachment 292670 [details]

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.
Comment 5 Alex Christensen 2016-10-26 09:42:13 PDT
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
Comment 6 youenn fablet 2016-10-27 02:06:39 PDT
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.