Bug 247562 - Intl.Locale.prototype.hourCycles leaks empty JSValue to script
Summary: Intl.Locale.prototype.hourCycles leaks empty JSValue to script
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: WebKit Local Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: David Degazio
Keywords: InRadar
Depends on:
Reported: 2022-11-07 01:13 PST by Samuel Groß
Modified: 2022-11-08 19:55 PST (History)
6 users (show)

See Also:


Note You need to log in before you can comment on or make changes to this bug.
Description Samuel Groß 2022-11-07 01:13:58 PST
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

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
Comment 1 Radar WebKit Bug Importer 2022-11-07 01:14:12 PST
Comment 2 Darin Adler 2022-11-07 08:48:57 PST
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.
Comment 3 Darin Adler 2022-11-07 08:50:38 PST
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);
Comment 4 Darin Adler 2022-11-07 08:53:50 PST
This code with the mistake was introduced in bug 227830.
Comment 5 David Degazio 2022-11-08 14:48:59 PST
Pull request: https://github.com/WebKit/WebKit/pull/6266
Comment 6 EWS 2022-11-08 19:55:46 PST
Committed 256473@main (86fbeb6fcd63): <https://commits.webkit.org/256473@main>

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