Intl.Locale.prototype.hourCycles leaks empty JSValue to script

• Component: JavaScriptCore
• Status: RESOLVED FIXED

[Samuel Groß]

The following program crashes JSC, both local builds from HEAD and in Safari 16.1

    function main() {
      const v24 = new Intl.Locale("trimEnd", { 'numberingSystem': "foobar" });
      let empty = v24.hourCycles;
      print(empty);    // use console.log in Safari
    }
    main();

The issue seems to be that the intlLocalePrototypeGetterHourCycles function [1] doesn't check that the return value of `locale->hourCycles(globalObject))` isn't nullptr and so may end up leaking the empty JSValue to script. I'm not sure if this can be abused to cause memory corruption. The empty JSValue is used for a few internal things, for example to indicate holes in Arrays (so if the leaked empty value is assigned to an array element, that will now appear as a hole) or to signal failure cases in C++ code [2]. As such, it may be possible to confuse some other code with this empty value, but I couldn't find an obvious case where this would then cause memory corruption.

[1] https://github.com/WebKit/WebKit/blob/443de8e8e9716b164f0a25da35e9235efd72d4dd/Source/JavaScriptCore/runtime/IntlLocalePrototype.cpp#L265
[2] https://github.com/WebKit/WebKit/blob/443de8e8e9716b164f0a25da35e9235efd72d4dd/Source/JavaScriptCore/runtime/JSCJSValue.h#L449

[Radar WebKit Bug Importer]

<rdar://problem/102031379>

[Darin Adler]

Looking at IntlLocale::hourCycles and other functions in the file I see that not only do they return nullptr, but they do so without using the RELEASE_AND_RETURN macro. I think that is also incorrect.

There are three options here for behavior for these cases returning nullptr:

1) Raise a JavaScript exception. In that case the return value doesn't matter and an empty value would be OK.
2) Return a JavaScript undefined.
3) Return a JavaScript null.

Not sure whether we should fix this in the bindings or in the IntlLocale class itself.

[Darin Adler]

OK, I studied this more closely. The other places that return nullptr are doing fine because they all raise exceptions. It’s only this one place that has it wrong, when udatpg_open fails.

I believe the correct fix is to add this line of code before the "return nullptr":

        throwTypeError(globalObject, scope, "invalid locale"_s);

[Darin Adler]

This code with the mistake was introduced in bug 227830.

[David Degazio]

Pull request: https://github.com/WebKit/WebKit/pull/6266

[EWS]

Committed 256473@main (86fbeb6fcd63): <https://commits.webkit.org/256473@main>

Reviewed commits have been landed. Closing PR #6266 and removing active labels.