Bug 214820 – IndexedDB binding utilities miss exception checks

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Yusuke Suzuki]

IndexedDB binding utilities miss exception checks

[Yusuke Suzuki]

Created attachment 405265 [details]
Patch

[Yusuke Suzuki]

<rdar://problem/66152374>

[Mark Lam]

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?

[Mark Lam]

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()))

[Yusuke Suzuki]

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.

[Yusuke Suzuki]

Created attachment 405326 [details]
Patch

[Yusuke Suzuki]

Created attachment 405328 [details]
Patch

[Mark Lam]

Comment on attachment 405328 [details]
Patch

r=me

[Yusuke Suzuki]

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.

[Yusuke Suzuki]

Created attachment 405336 [details]
Patch

[Mark Lam]

(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!

[EWS]

Committed r264988: <https://trac.webkit.org/changeset/264988>

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