Bug 163918 – JSONParse should not crash with null Strings

• Component: New Bugs
• Status: RESOLVED FIXED

[Alex Christensen]

JSONParse should not crash with null Strings

[Alex Christensen]

Created attachment 292670 [details]
Patch

[Michael Saboff]

Comment on attachment 292670 [details]
Patch

r=me

[Alex Christensen]

http://trac.webkit.org/changeset/207785

[Darin Adler]

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.

[Alex Christensen]

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

[youenn fablet]

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.