Bug 225279 – [GTK] segmentation fault in WebKit::IconDatabase::loadIconForPageURL

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKitGTK

[Jim Mason]

gtk3: 3.24.11
gdk-pixbuf: 2.40.0
cairo: 1.16.0
glib2: 2.66.8

I am encountering a segmentation fault in IconDatabase::loadIconForPageURL due to dereference of a null pointer returned by NativeImage::nativeImageForCurrentFrame.  This happens for certain (broken? unsupported?) icons, but not all.  One such icon is the favicon for this URL:

    https://thomas-guettler.de/htmx-swap-err-webkit/page.html

For me, entering the above URL in epiphany 40.0 elicits the seg fault.

I've attached a patch which clears the issue.  The patch also applies a similar null pointer check in the method setIconForPageURL.

The fix assumes `nativeImageForCurrentFrame` can return null.  If that method is supposed to return non-null in all cases, then there is another problem.

[Jim Mason]

Created attachment 427525 [details]
Patch

[EWS Watchlist]

Thanks for the patch. If this patch contains new public API please make sure it follows the guidelines for new WebKit2 GTK+ API. See https://trac.webkit.org/wiki/WebKitGTK/AddingNewWebKit2API

[Jim Mason]

Created attachment 427526 [details]
Patch

[Jim Mason]

Created attachment 427529 [details]
Patch

[Carlos Garcia Campos]

Comment on attachment 427529 [details]
Patch

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

> Source/WebKit/UIProcess/API/glib/IconDatabase.cpp:586
> +                if (nativeImage != nullptr)

Do not compare to nullptr. We normally prefer the early return first and avoid the else, so this would be:

if (!nativeImage) {
    completionHandler(nullptr);
    return;
}

addResult.iterator->value.first = nativeImage->platformImage();

> Source/WebKit/UIProcess/API/glib/IconDatabase.cpp:626
> +                    auto nativeImage = image->nativeImageForCurrentFrame();
> +                    if (nativeImage != nullptr)

And here this could be

if (auto nativeImage = image->nativeImageForCurrentFrame())

[Carlos Garcia Campos]

*** Bug 225313 has been marked as a duplicate of this bug. ***

[Jim Mason]

Thanks for the feedback.

In setIconForPageURL, I am tempted to simplify this:

    auto image = BitmapImage::create();
    if (image->setData(SharedBuffer::create(iconData, iconDataSize), true) < EncodedDataStatus::SizeAvailable)
        result = false;
    else if (auto nativeImage = image->nativeImageForCurrentFrame())
        addResult.iterator->value.first = nativeImage->platformImage();
    else
        result = false;

to something like this:

    RefPtr<NativeImage> nativeImage;
    auto image = BitmapImage::create();
    if (image->setData(SharedBuffer::create(iconData, iconDataSize), true) >= EncodedDataStatus::SizeAvailable &&
            (nativeImage = image->nativeImageForCurrentFrame()))
        addResult.iterator->value.first = nativeImage->platformImage();
    else
        result = false;

However, this means using RefPtr<NativeImage> instead of auto; I have doubts whether this frowned upon.  Advice?

[Adrian Perez]

(In reply to Jim Mason from comment #7)
> Thanks for the feedback.
> 
> In setIconForPageURL, I am tempted to simplify this:
> 
>     auto image = BitmapImage::create();
>     if (image->setData(SharedBuffer::create(iconData, iconDataSize), true) <
> EncodedDataStatus::SizeAvailable)
>         result = false;
>     else if (auto nativeImage = image->nativeImageForCurrentFrame())
>         addResult.iterator->value.first = nativeImage->platformImage();
>     else
>         result = false;
> 
> to something like this:
> 
>     RefPtr<NativeImage> nativeImage;
>     auto image = BitmapImage::create();
>     if (image->setData(SharedBuffer::create(iconData, iconDataSize), true)
> >= EncodedDataStatus::SizeAvailable &&
>             (nativeImage = image->nativeImageForCurrentFrame()))
>         addResult.iterator->value.first = nativeImage->platformImage();
>     else
>         result = false;
> 
> However, this means using RefPtr<NativeImage> instead of auto; I have doubts
> whether this frowned upon.  Advice?

There are similar bits of code all around WebKit, this is fine 👌️

[Jim Mason]

Created attachment 427648 [details]
Patch

[EWS]

Committed r276954 (237287@main): <https://commits.webkit.org/237287@main>

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

[Jim Mason]

Fix verified in r276954.

[Adrian Perez]

*** Bug 225348 has been marked as a duplicate of this bug. ***