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

WebCore::PathQt::platformPath(): don't return a temporary object #38

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

whitslack
Copy link

@whitslack whitslack commented May 29, 2024

WebCore::Path::platformPath() expects to be able to return the return value of WebCore::PlatformPathImpl::platformPath() as a PlatformPathPtr without creating a dangling reference. However, WebCore::PathQt::platformPath() is defined as returning a temporary QPainterPath object, which does become a dangling reference when returned by reference, leading to segfaults at runtime.

This PR changes the return type of WebCore::PathQt::platformPath() to PlatformPathPtr (paralleling the definition of WebCore::PathCG::platformPath()), so as to avoid creating a temporary QPainterPath object, thereby avoiding the creation of a dangling reference when returning from WebCore::Path::platformPath().

Fixes: #37

`WebCore::Path::platformPath()` expects to be able to return the return
value of `WebCore::PlatformPathImpl::platformPath()` as a
`PlatformPathPtr` without creating a dangling reference. However,
`WebCore::PathQt::platformPath()` is defined as returning a temporary
`QPainterPath` object, which does become a dangling reference when
returned by reference, leading to segfaults at runtime.

This commit changes the return type of `WebCore::PathQt::platformPath()`
to `PlatformPathPtr` (paralleling the definition of
`WebCore::PathCG::platformPath()`), so as to avoid creating a temporary
`QPainterPath` object, thereby avoiding the creation of a dangling
reference when returning from `WebCore::Path::platformPath()`.

Fixes: movableink#37
@whitslack
Copy link
Author

Note: The other way this can be fixed is to typedef PlatformPathPtr as QPainterPath rather than as const QPainterPath&. That is the option taken by QtWebKit 5.212:

https://github.com/qtwebkit/qtwebkit/blob/756e1c8f23dc2720471298281c421c0076d02df8/Source/WebCore/platform/graphics/Path.h#L47

typedef QPainterPath PlatformPath;

https://github.com/qtwebkit/qtwebkit/blob/756e1c8f23dc2720471298281c421c0076d02df8/Source/WebCore/platform/graphics/Path.h#L70-L71

/* QPainterPath is valued based */
typedef PlatformPath PlatformPathPtr;

Curiously, it has the same typo'd comment but in a different file and with a different typedef, so I wonder whether the dangling reference bug was introduced in movableink/webkit after copying the correct code from qtwebkit/qtwebkit or was fixed in qtwebkit/qtwebkit after copying the incorrect code from movableink/webkit. I'd guess the former.

For reference, here is the related code in movableink/webkit (exclusive of this PR):

typedef QPainterPath PlatformPath;

/* QPainterPath is valued based */
typedef const PlatformPath& PlatformPathPtr;

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Wonderwall segfault
1 participant