Bug 214820 - IndexedDB binding utilities miss exception checks
Summary: IndexedDB binding utilities miss exception checks
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Yusuke Suzuki
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2020-07-27 03:02 PDT by Yusuke Suzuki
Modified: 2020-07-28 09:15 PDT (History)
14 users (show)

See Also:


Attachments
Patch (30.70 KB, patch)
2020-07-27 03:03 PDT, Yusuke Suzuki
no flags Details | Formatted Diff | Diff
Patch (32.99 KB, patch)
2020-07-27 16:44 PDT, Yusuke Suzuki
no flags Details | Formatted Diff | Diff
Patch (33.95 KB, patch)
2020-07-27 16:48 PDT, Yusuke Suzuki
mark.lam: review+
Details | Formatted Diff | Diff
Patch (33.98 KB, patch)
2020-07-27 19:01 PDT, Yusuke Suzuki
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Yusuke Suzuki 2020-07-27 03:02:18 PDT
IndexedDB binding utilities miss exception checks
Comment 1 Yusuke Suzuki 2020-07-27 03:03:51 PDT
Created attachment 405265 [details]
Patch
Comment 2 Yusuke Suzuki 2020-07-27 03:03:53 PDT
<rdar://problem/66152374>
Comment 3 Mark Lam 2020-07-27 07:09:42 PDT
Comment on attachment 405265 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=405265&action=review

Can you check if the api-gtk bot failure is real?o

> Source/WebCore/bindings/js/IDBBindingUtilities.cpp:64
> +    auto scope = DECLARE_CATCH_SCOPE(vm);

Why is this a CatchScope?  Is it because this is the outermost scope that should handle exceptions?

> Source/WebCore/bindings/js/IDBBindingUtilities.cpp:195
> +    auto scope = DECLARE_CATCH_SCOPE(vm);

Ditto: what’s the rationale here?
Comment 4 Mark Lam 2020-07-27 09:22:53 PDT
Comment on attachment 405265 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=405265&action=review

>> Source/WebCore/bindings/js/IDBBindingUtilities.cpp:64
>> +    auto scope = DECLARE_CATCH_SCOPE(vm);
> 
> Why is this a CatchScope?  Is it because this is the outermost scope that should handle exceptions?

After trying out your patch and investigating the issue, it looks like the expected behavior is to propagate the exception to the caller.  That would mean that we should be using a ThrowScope here instead of a CatchScope.  So, I tried that and found that IDBKeyRange::bound() needs some changes in order to placate the exception check validator.  I understand that we may not want to add explicit exception checks there because it is redundant to the isValid() checks.  We can address that using an exception.  After I applied the following patch (in addition to yours), the 2 DECLARE_CATCH_SCOPEs can now be DECLARE_THROW_SCOPEs.  Can you take another look?  Thanks.

Index: Source/WebCore/Modules/indexeddb/IDBKeyRange.cpp
===================================================================
--- Source/WebCore/Modules/indexeddb/IDBKeyRange.cpp	(revision 264901)
+++ Source/WebCore/Modules/indexeddb/IDBKeyRange.cpp	(working copy)
@@ -1,5 +1,6 @@
 /*
  * Copyright (C) 2010 Google Inc. All rights reserved.
+ * Copyright (C) 2020 Apple Inc. All rights reserved.
  *
  * Redistribution and use in source and binary forms, with or without
  * modification, are permitted provided that the following conditions
@@ -32,7 +33,9 @@
 #include "IDBKey.h"
 #include "IDBKeyData.h"
 #include "ScriptExecutionContext.h"
+#include <JavaScriptCore/JSGlobalObject.h>
 #include <JavaScriptCore/JSCJSValue.h>
+#include <JavaScriptCore/ThrowScope.h>
 #include <wtf/IsoMallocInlines.h>
 
 namespace WebCore {
@@ -94,10 +97,14 @@
 
 ExceptionOr<Ref<IDBKeyRange>> IDBKeyRange::bound(JSGlobalObject& state, JSValue lowerValue, JSValue upperValue, bool lowerOpen, bool upperOpen)
 {
+    VM& vm = state.vm();
+    auto scope = DECLARE_THROW_SCOPE(vm);
     auto lower = scriptValueToIDBKey(state, lowerValue);
+    EXCEPTION_ASSERT_UNUSED(scope, !!scope.exception() == !lower->isValid());
     if (!lower->isValid())
         return Exception { DataError };
     auto upper = scriptValueToIDBKey(state, upperValue);
+    EXCEPTION_ASSERT(!!scope.exception() == !lower->isValid());
     if (!upper->isValid())
         return Exception { DataError };
     if (upper->isLessThan(lower.get()))
Comment 5 Yusuke Suzuki 2020-07-27 16:43:43 PDT
Comment on attachment 405265 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=405265&action=review

>>> Source/WebCore/bindings/js/IDBBindingUtilities.cpp:64
>>> +    auto scope = DECLARE_CATCH_SCOPE(vm);
>> 
>> Why is this a CatchScope?  Is it because this is the outermost scope that should handle exceptions?
> 
> After trying out your patch and investigating the issue, it looks like the expected behavior is to propagate the exception to the caller.  That would mean that we should be using a ThrowScope here instead of a CatchScope.  So, I tried that and found that IDBKeyRange::bound() needs some changes in order to placate the exception check validator.  I understand that we may not want to add explicit exception checks there because it is redundant to the isValid() checks.  We can address that using an exception.  After I applied the following patch (in addition to yours), the 2 DECLARE_CATCH_SCOPEs can now be DECLARE_THROW_SCOPEs.  Can you take another look?  Thanks.
> 
> Index: Source/WebCore/Modules/indexeddb/IDBKeyRange.cpp
> ===================================================================
> --- Source/WebCore/Modules/indexeddb/IDBKeyRange.cpp	(revision 264901)
> +++ Source/WebCore/Modules/indexeddb/IDBKeyRange.cpp	(working copy)
> @@ -1,5 +1,6 @@
>  /*
>   * Copyright (C) 2010 Google Inc. All rights reserved.
> + * Copyright (C) 2020 Apple Inc. All rights reserved.
>   *
>   * Redistribution and use in source and binary forms, with or without
>   * modification, are permitted provided that the following conditions
> @@ -32,7 +33,9 @@
>  #include "IDBKey.h"
>  #include "IDBKeyData.h"
>  #include "ScriptExecutionContext.h"
> +#include <JavaScriptCore/JSGlobalObject.h>
>  #include <JavaScriptCore/JSCJSValue.h>
> +#include <JavaScriptCore/ThrowScope.h>
>  #include <wtf/IsoMallocInlines.h>
>  
>  namespace WebCore {
> @@ -94,10 +97,14 @@
>  
>  ExceptionOr<Ref<IDBKeyRange>> IDBKeyRange::bound(JSGlobalObject& state, JSValue lowerValue, JSValue upperValue, bool lowerOpen, bool upperOpen)
>  {
> +    VM& vm = state.vm();
> +    auto scope = DECLARE_THROW_SCOPE(vm);
>      auto lower = scriptValueToIDBKey(state, lowerValue);
> +    EXCEPTION_ASSERT_UNUSED(scope, !!scope.exception() == !lower->isValid());
>      if (!lower->isValid())
>          return Exception { DataError };
>      auto upper = scriptValueToIDBKey(state, upperValue);
> +    EXCEPTION_ASSERT(!!scope.exception() == !lower->isValid());
>      if (!upper->isValid())
>          return Exception { DataError };
>      if (upper->isLessThan(lower.get()))

OK, I've revised and updated.
Comment 6 Yusuke Suzuki 2020-07-27 16:44:42 PDT
Created attachment 405326 [details]
Patch
Comment 7 Yusuke Suzuki 2020-07-27 16:48:48 PDT
Created attachment 405328 [details]
Patch
Comment 8 Mark Lam 2020-07-27 16:58:03 PDT
Comment on attachment 405328 [details]
Patch

r=me
Comment 9 Yusuke Suzuki 2020-07-27 18:57:18 PDT
Comment on attachment 405328 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=405328&action=review

> Source/WebCore/Modules/indexeddb/IDBKeyRange.cpp:78
> +    EXCEPTION_ASSERT_UNUSED(scope, !!scope.exception() == !key->isValid());

These assertions should be `!scope.exception() || !key->isValid()` since key can be invalid even if JS exception is not thrown.
Comment 10 Yusuke Suzuki 2020-07-27 19:01:25 PDT
Created attachment 405336 [details]
Patch
Comment 11 Mark Lam 2020-07-27 20:13:51 PDT
(In reply to Yusuke Suzuki from comment #9)
> Comment on attachment 405328 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=405328&action=review
> 
> > Source/WebCore/Modules/indexeddb/IDBKeyRange.cpp:78
> > +    EXCEPTION_ASSERT_UNUSED(scope, !!scope.exception() == !key->isValid());
> 
> These assertions should be `!scope.exception() || !key->isValid()` since key
> can be invalid even if JS exception is not thrown.

Good point!
Comment 12 EWS 2020-07-28 09:15:57 PDT
Committed r264988: <https://trac.webkit.org/changeset/264988>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 405336 [details].