SIGILL @ WebCore::Shape::createRasterShape -- DOS ASAN

[Pinki Gyanchandani]

The crash is happening in Shape::createRasterShape function. This is a release assert RELEASE_ASSERT(imageData && imageData->data()); in code. 

The reason for crash is because ImageData::create returns NULL, because dataSize.hasOverFlowed() is TRUE. 

In the failure case intRect  size, width is huge value and when its multiplied with dataSize (=4) and height, that results in overflow.

[Pinki Gyanchandani]

Created attachment 398689 [details]
Patch

[Simon Fraser (smfr)]

Comment on attachment 398689 [details]
Patch

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

> Source/WebCore/rendering/shapes/Shape.cpp:196
> +        // RELEASE_ASSERT(imageData && imageData->data());

Please don't check in commented out code.

> Source/WebCore/rendering/shapes/Shape.cpp:197
> +        if (imageData && imageData->data()) {

Make this an early return.

[Geoffrey Garen]

Comment on attachment 398689 [details]
Patch

To enable early returns, you can put this code:

    auto rasterShape = makeUnique<RasterShape>(WTFMove(intervals), marginRect.size());
    rasterShape->m_writingMode = writingMode;
    rasterShape->m_margin = margin;
    return rasterShape;

in a local lambda like so:

auto createShape = [](WritingMode writingMode, float margin) {
    auto rasterShape = makeUnique<RasterShape>(WTFMove(intervals), marginRect.size());
    rasterShape->m_writingMode = writingMode;
    rasterShape->m_margin = margin;
    return rasterShape;
};

if (!condition)
    return createShape(writingMode, margin);

You should make the existing "if (imageBuffer)" check an early return too, for consistency.

[Pinki Gyanchandani]

Created attachment 398809 [details]
Patch

[Geoffrey Garen]

Comment on attachment 398809 [details]
Patch

r=me

[EWS]

Committed r261363: <https://trac.webkit.org/changeset/261363>

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

[Simon Fraser (smfr)]

Comment on attachment 398809 [details]
Patch

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

> Source/WebCore/rendering/shapes/Shape.cpp:204
> +    // Removing the Release Assert, as we could get to a value where imageData could be nullptr. A case where
> +    // ImageRect.size() is huge, imageData::create can return a nullptr because data size has overflowed.
> +    // Refer rdar://problem/61793884

This comment about removing the Release Assert should be removed.

[Pinki Gyanchandani]

re-opening to incorporate the final comment from Simon Fraser, on correcting the comment.

[Pinki Gyanchandani]

Created attachment 398875 [details]
Patch

[Pinki Gyanchandani]

Created attachment 398876 [details]
Patch

[EWS]

Committed r261400: <https://trac.webkit.org/changeset/261400>

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