| Summary: | Missing exception check with new MediaStream(0) | ||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|
| Product: | WebKit | Reporter: | Ryosuke Niwa <rniwa> | ||||||||
| Component: | Media | Assignee: | Nobody <webkit-unassigned> | ||||||||
| Status: | RESOLVED FIXED | ||||||||||
| Severity: | Normal | CC: | bfulgham, cgarcia, ews-feeder, fred.wang, gpoo, mark.lam, product-security, rbuis, svillar, webkit-bug-importer, ysuzuki | ||||||||
| Priority: | P2 | Keywords: | InRadar | ||||||||
| Version: | WebKit Nightly Build | ||||||||||
| Hardware: | Unspecified | ||||||||||
| OS: | Unspecified | ||||||||||
| See Also: | https://bugs.webkit.org/show_bug.cgi?id=220790 | ||||||||||
| Attachments: |
|
||||||||||
|
Description
Ryosuke Niwa
2021-01-05 23:25:23 PST
Created attachment 417835 [details]
Patch
Comment on attachment 417835 [details]
Patch
Can you add the test in the patch as well?
(In reply to Mark Lam from comment #2) > Comment on attachment 417835 [details] > Patch > > Can you add the test in the patch as well? Does this missing exception check have a security implication? If so, we can't check in the test. (In reply to Ryosuke Niwa from comment #3) > (In reply to Mark Lam from comment #2) > > Comment on attachment 417835 [details] > > Patch > > > > Can you add the test in the patch as well? > > Does this missing exception check have a security implication? If so, we > can't check in the test. Let's attach the test to this bug. We will just land it into Internal repository. I thought about the test, but I was not sure how to do it. It requires ENABLE_EXCEPTION_SCOPE_VERIFICATION to be enabled, which is the default for debug builds, but it also needs JSC_validateExceptionChecks=1. I guess we can always set the env var from WTR, or at least when --debug is passed. This is already covered by fast/mediastream/MediaStreamConstructor.html but we need to run it with JSC_validateExceptionChecks=1 to make it crash in debug. I suspect there might be more existing tests crashing with JSC_validateExceptionChecks=1, so I'll file a new bug report to set the env var for debug config and see what EWS says. (In reply to Carlos Garcia Campos from comment #6) > This is already covered by fast/mediastream/MediaStreamConstructor.html but > we need to run it with JSC_validateExceptionChecks=1 to make it crash in > debug. To enable the option in just 1 test, you can use `[ jscOptions=--validateExceptionChecks=true ]`. See LayoutTests/js/dom/promise-should-have-exception-check-on-operation.html for an example. The reason we haven't turned it on for all debug build runs is because there are a lot more failures that need to be fixed first before we can do that (to avoid red test bots). You're welcome to try to fix them all though and turn on the flag for all debug builds. (In reply to Mark Lam from comment #8) > (In reply to Carlos Garcia Campos from comment #6) > > This is already covered by fast/mediastream/MediaStreamConstructor.html but > > we need to run it with JSC_validateExceptionChecks=1 to make it crash in > > debug. > > To enable the option in just 1 test, you can use `[ > jscOptions=--validateExceptionChecks=true ]`. See > LayoutTests/js/dom/promise-should-have-exception-check-on-operation.html for > an example. Great, that should work for this particular case then. I guess that should be done in a separate commit in any case, right? > The reason we haven't turned it on for all debug build runs is because there > are a lot more failures that need to be fixed first before we can do that > (to avoid red test bots). You're welcome to try to fix them all though and > turn on the flag for all debug builds. Indeed, see bug #220790. So, can we land this fix then? There's no new test for this, we just need to enable JSC_validateExceptionChecks for fast/mediastream/MediaStreamConstructor.html in a follow up. Comment on attachment 417835 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=417835&action=review Please rebase the bindings test after applying the fix below. > Source/WebCore/bindings/scripts/CodeGeneratorJS.pm:3669 > push(@implContent, "#if ${conditionalString}\n") if $conditionalString; > push(@implContent, " if ($condition)\n ") if $condition; > push(@implContent, " RELEASE_AND_RETURN(throwScope, (" . $overloadFunctionPrefix . $overload->{overloadIndex} . $overloadFunctionSuffix . "(${parametersToForward})));\n"); > + push(@implContent, " RETURN_IF_EXCEPTION(throwScope, { });\n") if $canThrow; This fix is not correct. The exception is potentially thrown by the evaluation of $condition. And then the if clause can call another function that may also throw. Hence, the exception check needs to appear before the second function call. Also, I recommend renaming $canThrow to $conditionCanThrow to make it clear that this bool only covers a throw from the condition. The correct fix is to do this instead: if ($condition && $conditionCanThrow) { push(@implContent, " {\n "); push(@implContent, " bool success = $condition;\n "); push(@implContent, " RETURN_IF_EXCEPTION(throwScope, { });\n"); push(@implContent, " if (success)\n "); push(@implContent, " RELEASE_AND_RETURN(throwScope, (" . $overloadFunctionPrefix . $overload->{overloadIndex} . $overloadFunctionSuffix . " push(@implContent, " }\n "); } elsif ($condition) { push(@implContent, " if ($condition)\n ") push(@implContent, " RELEASE_AND_RETURN(throwScope, (" . $overloadFunctionPrefix . $overload->{overloadIndex} . $overloadFunctionSuffix . " } else { push(@implContent, " RELEASE_AND_RETURN(throwScope, (" . $overloadFunctionPrefix . $overload->{overloadIndex} . $overloadFunctionSuffix . " } Comment on attachment 417835 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=417835&action=review >> Source/WebCore/bindings/scripts/CodeGeneratorJS.pm:3669 >> + push(@implContent, " RETURN_IF_EXCEPTION(throwScope, { });\n") if $canThrow; > > This fix is not correct. The exception is potentially thrown by the evaluation of $condition. And then the if clause can call another function that may also throw. Hence, the exception check needs to appear before the second function call. Also, I recommend renaming $canThrow to $conditionCanThrow to make it clear that this bool only covers a throw from the condition. > > The correct fix is to do this instead: > > if ($condition && $conditionCanThrow) { > push(@implContent, " {\n "); > push(@implContent, " bool success = $condition;\n "); > push(@implContent, " RETURN_IF_EXCEPTION(throwScope, { });\n"); > push(@implContent, " if (success)\n "); > push(@implContent, " RELEASE_AND_RETURN(throwScope, (" . $overloadFunctionPrefix . $overload->{overloadIndex} . $overloadFunctionSuffix . " > push(@implContent, " }\n "); > } elsif ($condition) { > push(@implContent, " if ($condition)\n ") > push(@implContent, " RELEASE_AND_RETURN(throwScope, (" . $overloadFunctionPrefix . $overload->{overloadIndex} . $overloadFunctionSuffix . " > } else { > push(@implContent, " RELEASE_AND_RETURN(throwScope, (" . $overloadFunctionPrefix . $overload->{overloadIndex} . $overloadFunctionSuffix . " > } Oops, I'm missing the `(${parametersToForward})));\n");` at the end of the RELEASE_AND_RETURN in each case of the above 3 cases. Please add. Created attachment 418849 [details]
Patch
Comment on attachment 418849 [details]
Patch
r=me
Committed r272199: <https://trac.webkit.org/changeset/272199> All reviewed patches have been landed. Closing bug and clearing flags on attachment 418849 [details]. Can we add a test since we don't believe this is a security issue? (In reply to Ryosuke Niwa from comment #16) > Can we add a test since we don't believe this is a security issue? Oh never mind. Forgot that we already had an existing test for this. |