Callers of JSString::value() should check for exceptions thereafter.
authormark.lam@apple.com <mark.lam@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Thu, 18 Feb 2016 06:28:26 +0000 (06:28 +0000)
committermark.lam@apple.com <mark.lam@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Thu, 18 Feb 2016 06:28:26 +0000 (06:28 +0000)
https://bugs.webkit.org/show_bug.cgi?id=154346

Reviewed by Geoffrey Garen.

Source/JavaScriptCore:

JSString::value() can throw an exception if the JS string is a rope and value()
needs to resolve the rope but encounters an OutOfMemory error.  If value() is not
able to resolve the rope, it will return a null string (in addition to throwing
the exception).  If a caller does not check for exceptions after calling
JSString::value(), they may eventually use the returned null string and crash the
VM.

The fix is to add all the necessary exception checks, and do the appropriate
handling if needed.

* jsc.cpp:
(functionRun):
(functionLoad):
(functionReadFile):
(functionCheckSyntax):
(functionLoadWebAssembly):
(functionLoadModule):
(functionCheckModuleSyntax):
* runtime/DateConstructor.cpp:
(JSC::dateParse):
(JSC::dateNow):
* runtime/JSGlobalObjectFunctions.cpp:
(JSC::globalFuncEval):
* tools/JSDollarVMPrototype.cpp:
(JSC::functionPrint):

Source/WebCore:

No new tests.  The crash that results from this issue is dependent on a race
condition where an OutOfMemory error occurs precisely at the point where the
JSString::value() function is called on a rope JSString.

* bindings/js/JSHTMLAllCollectionCustom.cpp:
(WebCore::callHTMLAllCollection):
* bindings/js/JSStorageCustom.cpp:
(WebCore::JSStorage::putDelegate):
- Added a comment at the site of the exception check to clarify the meaning of
  the return value.

git-svn-id: https://svn.webkit.org/repository/webkit/trunk@196745 268f45cc-cd09-0410-ab3c-d52691b4dbfc

Source/JavaScriptCore/ChangeLog
Source/JavaScriptCore/jsc.cpp
Source/JavaScriptCore/runtime/DateConstructor.cpp
Source/JavaScriptCore/runtime/JSGlobalObjectFunctions.cpp
Source/JavaScriptCore/tools/JSDollarVMPrototype.cpp
Source/WebCore/ChangeLog
Source/WebCore/bindings/js/JSHTMLAllCollectionCustom.cpp
Source/WebCore/bindings/js/JSStorageCustom.cpp

index 079d751..c1a6cd1 100644 (file)
@@ -1,3 +1,36 @@
+2016-02-17  Mark Lam  <mark.lam@apple.com>
+
+        Callers of JSString::value() should check for exceptions thereafter.
+        https://bugs.webkit.org/show_bug.cgi?id=154346
+
+        Reviewed by Geoffrey Garen.
+
+        JSString::value() can throw an exception if the JS string is a rope and value() 
+        needs to resolve the rope but encounters an OutOfMemory error.  If value() is not
+        able to resolve the rope, it will return a null string (in addition to throwing
+        the exception).  If a caller does not check for exceptions after calling
+        JSString::value(), they may eventually use the returned null string and crash the
+        VM.
+
+        The fix is to add all the necessary exception checks, and do the appropriate
+        handling if needed.
+
+        * jsc.cpp:
+        (functionRun):
+        (functionLoad):
+        (functionReadFile):
+        (functionCheckSyntax):
+        (functionLoadWebAssembly):
+        (functionLoadModule):
+        (functionCheckModuleSyntax):
+        * runtime/DateConstructor.cpp:
+        (JSC::dateParse):
+        (JSC::dateNow):
+        * runtime/JSGlobalObjectFunctions.cpp:
+        (JSC::globalFuncEval):
+        * tools/JSDollarVMPrototype.cpp:
+        (JSC::functionPrint):
+
 2016-02-17  Benjamin Poulain  <bpoulain@apple.com>
 
         [JSC] ARM64: Support the immediate format used for bit operations in Air
index 8085611..23f474a 100644 (file)
@@ -1243,6 +1243,8 @@ EncodedJSValue JSC_HOST_CALL functionVersion(ExecState*)
 EncodedJSValue JSC_HOST_CALL functionRun(ExecState* exec)
 {
     String fileName = exec->argument(0).toString(exec)->value(exec);
+    if (exec->hadException())
+        return JSValue::encode(jsUndefined());
     Vector<char> script;
     if (!fetchScriptFromLocalFileSystem(fileName, script))
         return JSValue::encode(exec->vm().throwException(exec, createError(exec, ASCIILiteral("Could not open file."))));
@@ -1272,6 +1274,8 @@ EncodedJSValue JSC_HOST_CALL functionRun(ExecState* exec)
 EncodedJSValue JSC_HOST_CALL functionLoad(ExecState* exec)
 {
     String fileName = exec->argument(0).toString(exec)->value(exec);
+    if (exec->hadException())
+        return JSValue::encode(jsUndefined());
     Vector<char> script;
     if (!fetchScriptFromLocalFileSystem(fileName, script))
         return JSValue::encode(exec->vm().throwException(exec, createError(exec, ASCIILiteral("Could not open file."))));
@@ -1288,6 +1292,8 @@ EncodedJSValue JSC_HOST_CALL functionLoad(ExecState* exec)
 EncodedJSValue JSC_HOST_CALL functionReadFile(ExecState* exec)
 {
     String fileName = exec->argument(0).toString(exec)->value(exec);
+    if (exec->hadException())
+        return JSValue::encode(jsUndefined());
     Vector<char> script;
     if (!fillBufferWithContentsOfFile(fileName, script))
         return JSValue::encode(exec->vm().throwException(exec, createError(exec, ASCIILiteral("Could not open file."))));
@@ -1298,6 +1304,8 @@ EncodedJSValue JSC_HOST_CALL functionReadFile(ExecState* exec)
 EncodedJSValue JSC_HOST_CALL functionCheckSyntax(ExecState* exec)
 {
     String fileName = exec->argument(0).toString(exec)->value(exec);
+    if (exec->hadException())
+        return JSValue::encode(jsUndefined());
     Vector<char> script;
     if (!fetchScriptFromLocalFileSystem(fileName, script))
         return JSValue::encode(exec->vm().throwException(exec, createError(exec, ASCIILiteral("Could not open file."))));
@@ -1565,6 +1573,8 @@ EncodedJSValue JSC_HOST_CALL functionIs32BitPlatform(ExecState*)
 EncodedJSValue JSC_HOST_CALL functionLoadWebAssembly(ExecState* exec)
 {
     String fileName = exec->argument(0).toString(exec)->value(exec);
+    if (exec->hadException())
+        return JSValue::encode(jsUndefined());
     Vector<char> buffer;
     if (!fillBufferWithContentsOfFile(fileName, buffer))
         return JSValue::encode(exec->vm().throwException(exec, createError(exec, ASCIILiteral("Could not open file."))));
@@ -1584,6 +1594,8 @@ EncodedJSValue JSC_HOST_CALL functionLoadWebAssembly(ExecState* exec)
 EncodedJSValue JSC_HOST_CALL functionLoadModule(ExecState* exec)
 {
     String fileName = exec->argument(0).toString(exec)->value(exec);
+    if (exec->hadException())
+        return JSValue::encode(jsUndefined());
     Vector<char> script;
     if (!fetchScriptFromLocalFileSystem(fileName, script))
         return JSValue::encode(exec->vm().throwException(exec, createError(exec, ASCIILiteral("Could not open file."))));
@@ -1608,6 +1620,8 @@ EncodedJSValue JSC_HOST_CALL functionLoadModule(ExecState* exec)
 EncodedJSValue JSC_HOST_CALL functionCheckModuleSyntax(ExecState* exec)
 {
     String source = exec->argument(0).toString(exec)->value(exec);
+    if (exec->hadException())
+        return JSValue::encode(jsUndefined());
 
     StopWatch stopWatch;
     stopWatch.start();
index 6d88f50..4fcc8a3 100644 (file)
@@ -205,7 +205,10 @@ CallType DateConstructor::getCallData(JSCell*, CallData& callData)
 
 EncodedJSValue JSC_HOST_CALL dateParse(ExecState* exec)
 {
-    return JSValue::encode(jsNumber(parseDate(exec->vm(), exec->argument(0).toString(exec)->value(exec))));
+    String dateStr = exec->argument(0).toString(exec)->value(exec);
+    if (exec->hadException())
+        return JSValue::encode(jsUndefined());
+    return JSValue::encode(jsNumber(parseDate(exec->vm(), dateStr)));
 }
 
 EncodedJSValue JSC_HOST_CALL dateNow(ExecState* exec)
index 6acea5e..ca5e4b3 100644 (file)
@@ -574,6 +574,8 @@ EncodedJSValue JSC_HOST_CALL globalFuncEval(ExecState* exec)
     }
 
     String s = x.toString(exec)->value(exec);
+    if (exec->hadException())
+        return JSValue::encode(jsUndefined());
 
     if (s.is8Bit()) {
         LiteralParser<LChar> preparser(exec, s.characters8(), s.length(), NonStrictJSON);
index 1513466..2d87eaa 100644 (file)
@@ -303,7 +303,10 @@ static EncodedJSValue JSC_HOST_CALL functionPrint(ExecState* exec)
     for (unsigned i = 0; i < exec->argumentCount(); ++i) {
         if (i)
             dataLog(" ");
-        dataLog(exec->uncheckedArgument(i).toString(exec)->value(exec));
+        String argStr = exec->uncheckedArgument(i).toString(exec)->value(exec);
+        if (exec->hadException())
+            return JSValue::encode(jsUndefined());
+        dataLog(argStr);
     }
     return JSValue::encode(jsUndefined());
 }
index b4fb4a9..27353e7 100644 (file)
@@ -1,3 +1,21 @@
+2016-02-17  Mark Lam  <mark.lam@apple.com>
+
+        Callers of JSString::value() should check for exceptions thereafter.
+        https://bugs.webkit.org/show_bug.cgi?id=154346
+
+        Reviewed by Geoffrey Garen.
+
+        No new tests.  The crash that results from this issue is dependent on a race
+        condition where an OutOfMemory error occurs precisely at the point where the
+        JSString::value() function is called on a rope JSString.
+
+        * bindings/js/JSHTMLAllCollectionCustom.cpp:
+        (WebCore::callHTMLAllCollection):
+        * bindings/js/JSStorageCustom.cpp:
+        (WebCore::JSStorage::putDelegate):
+        - Added a comment at the site of the exception check to clarify the meaning of
+          the return value.
+
 2016-02-17  David Kilzer  <ddkilzer@apple.com>
 
         [Cocoa] Always check the return value of dlopen() and dlsym() in Release builds
index 94bc324..c988198 100644 (file)
@@ -65,6 +65,8 @@ static EncodedJSValue JSC_HOST_CALL callHTMLAllCollection(ExecState* exec)
     if (exec->argumentCount() == 1) {
         // Support for document.all(<index>) etc.
         String string = exec->argument(0).toString(exec)->value(exec);
+        if (exec->hadException())
+            return JSValue::encode(jsUndefined());
         if (Optional<uint32_t> index = parseIndex(*string.impl()))
             return JSValue::encode(toJS(exec, jsCollection->globalObject(), collection.item(index.value())));
 
@@ -74,6 +76,8 @@ static EncodedJSValue JSC_HOST_CALL callHTMLAllCollection(ExecState* exec)
 
     // The second arg, if set, is the index of the item we want
     String string = exec->argument(0).toString(exec)->value(exec);
+    if (exec->hadException())
+        return JSValue::encode(jsUndefined());
     if (Optional<uint32_t> index = parseIndex(*exec->argument(1).toWTFString(exec).impl())) {
         if (auto* item = collection.namedItemWithIndex(string, index.value()))
             return JSValue::encode(toJS(exec, jsCollection->globalObject(), item));
index 29bae2a..3aa7e94 100644 (file)
@@ -114,8 +114,13 @@ bool JSStorage::putDelegate(ExecState* exec, PropertyName propertyName, JSValue
         return false;
 
     String stringValue = value.toString(exec)->value(exec);
-    if (exec->hadException())
+    if (exec->hadException()) {
+        // The return value indicates whether putDelegate() should handle the put operation (which
+        // if true, tells the caller not to execute the generic put). It does not indicate whether
+        // putDelegate() did successfully complete the operation or not (which it didn't in this
+        // case due to the exception).
         return true;
+    }
 
     ExceptionCode ec = 0;
     wrapped().setItem(propertyNameToString(propertyName), stringValue, ec);