Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Null check IOSurfaceRef when creating WebCore::IOSurface from IOSurfaceRef #2767

Conversation

tuankiet65
Copy link
Member

@tuankiet65 tuankiet65 commented Jul 27, 2022

a8fb293

Null check IOSurfaceRef when creating WebCore::IOSurface from IOSurfaceRef
https://bugs.webkit.org/show_bug.cgi?id=243241

Reviewed by NOBODY (OOPS!).

When creating a WebCore::IOSurface from an IOSurfaceRef, WebCore::IOSurface expects
the IOSurfaceRef to be non-null, so it can pull out the dimension and allocation
size of the IOSurfaceRef.

* Source/WebCore/platform/graphics/cocoa/IOSurface.mm:
(WebCore::IOSurface::createFromSurface):

@tuankiet65 tuankiet65 self-assigned this Jul 27, 2022
@tuankiet65 tuankiet65 added WebCore Misc. For miscellaneous bugs in the WebCore framework (and not JavaScriptCore or WebKit). WebKit Local Build labels Jul 27, 2022
@@ -213,6 +216,8 @@
: m_colorSpace(colorSpace)
, m_surface(surface)
{
ASSERT(surface);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is it possible for this assertion to be hit because surface null checked before it can reach this code? If so, an assertion might make more sense in the ::createFromSurface method so it has the chance to be hit while in a debug build.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was going to say "defense mechanism hey!" but this is a private constructor already. I can remove this then. About asserting vs. returning nullptr, I like the latter more because createFromImage below does that.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yup, it looks like DisplayBufferDisplayDelegate expects createFromSurface to potentially return a nullptr so it makes sense.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It might make sense to assert at the end of DisplayBufferDisplayDelegate::setDisplayBuffer in order to catch some simple errors, but I am not familiar with this code enough.

@tuankiet65 tuankiet65 force-pushed the eng/Null-check-IOSurfaceRef-when-creating-WebCoreIOSurface-from-IOSurfaceRef branch from bb81ec1 to ed35a92 Compare July 27, 2022 03:55
@litherum
Copy link
Contributor

Before landing, please add a more substantial commit message.

What’s the reasoning for putting this check inside the function rather than at the call sites?

@tuankiet65 tuankiet65 force-pushed the eng/Null-check-IOSurfaceRef-when-creating-WebCoreIOSurface-from-IOSurfaceRef branch from ed35a92 to a2a2b18 Compare July 29, 2022 21:08
…ceRef

https://bugs.webkit.org/show_bug.cgi?id=243241

Reviewed by NOBODY (OOPS!).

When creating a WebCore::IOSurface from an IOSurfaceRef, WebCore::IOSurface expects
the IOSurfaceRef to be non-null, so it can pull out the dimension and allocation
size of the IOSurfaceRef.

* Source/WebCore/platform/graphics/cocoa/IOSurface.mm:
(WebCore::IOSurface::createFromSurface):
@tuankiet65 tuankiet65 force-pushed the eng/Null-check-IOSurfaceRef-when-creating-WebCoreIOSurface-from-IOSurfaceRef branch from a2a2b18 to a8fb293 Compare July 29, 2022 21:09
@tuankiet65
Copy link
Member Author

Superseded by #5785.

@tuankiet65 tuankiet65 closed this Jun 14, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
WebCore Misc. For miscellaneous bugs in the WebCore framework (and not JavaScriptCore or WebKit).
Projects
None yet
4 participants