vcl/inc/skia/utils.hxx | 5 +++++ vcl/skia/SkiaHelper.cxx | 32 ++++++++++++++++++++++++++++---- vcl/skia/gdiimpl.cxx | 27 ++++++++++++++++----------- vcl/skia/salbmp.cxx | 12 ++++++------ vcl/skia/win/gdiimpl.cxx | 4 ++-- 5 files changed, 57 insertions(+), 23 deletions(-)
New commits: commit 797315f220f82682748efaf80a29844a93f04f48 Author: Luboš Luňák <[email protected]> AuthorDate: Tue Sep 22 13:42:02 2020 +0200 Commit: Luboš Luňák <[email protected]> CommitDate: Wed Sep 23 08:46:10 2020 +0200 detect and fail immediately on failed Skia allocations (tdf#135952) The problem with tdf#135952 is that the PNG export dialog can lead to requesting an absurdly large image, which leads to allocation failures. Some VCL backends such as headless try to cope with this and bail out, but they often crash anyway, since at higher levels of LO code nothing checks for these corner cases anyway. And even if nothing would crash, this can lead to silently losing data. So I've decided to directly detect such cases and fail hard and early. Change-Id: I5277a65c794116206de8280faf22ff897daa2f56 Reviewed-on: https://gerrit.libreoffice.org/c/core/+/103171 Tested-by: Jenkins Reviewed-by: Luboš Luňák <[email protected]> diff --git a/vcl/inc/skia/utils.hxx b/vcl/inc/skia/utils.hxx index fc2567ac4e32..ed9ae0eaf100 100644 --- a/vcl/inc/skia/utils.hxx +++ b/vcl/inc/skia/utils.hxx @@ -47,6 +47,11 @@ inline sk_sp<SkSurface> createSkSurface(const Size& size, SkColorType type = kN3 // Create SkImage, GPU-backed if possible. VCL_DLLPUBLIC sk_sp<SkImage> createSkImage(const SkBitmap& bitmap); +// Call surface->makeImageSnapshot() and abort on failure. +VCL_DLLPUBLIC sk_sp<SkImage> makeCheckedImageSnapshot(sk_sp<SkSurface> surface); +VCL_DLLPUBLIC sk_sp<SkImage> makeCheckedImageSnapshot(sk_sp<SkSurface> surface, + const SkIRect& bounds); + // Must be called in any VCL backend before any Skia functionality is used. // If not set, Skia will be disabled. VCL_DLLPUBLIC void diff --git a/vcl/skia/SkiaHelper.cxx b/vcl/skia/SkiaHelper.cxx index e029a9137429..f9b231d87ce6 100644 --- a/vcl/skia/SkiaHelper.cxx +++ b/vcl/skia/SkiaHelper.cxx @@ -413,10 +413,16 @@ sk_sp<SkSurface> createSkSurface(int width, int height, SkColorType type) // Create raster surface as a fallback. surface = SkSurface::MakeRaster(SkImageInfo::Make(width, height, type, kPremul_SkAlphaType)); assert(surface); + if (surface) + { #ifdef DBG_UTIL - prefillSurface(surface); + prefillSurface(surface); #endif - return surface; + return surface; + } + // In non-debug builds we could return SkSurface::MakeNull() and try to cope with the situation, + // but that can lead to unnoticed data loss, so better fail clearly. + abort(); } sk_sp<SkImage> createSkImage(const SkBitmap& bitmap) @@ -437,7 +443,7 @@ sk_sp<SkImage> createSkImage(const SkBitmap& bitmap) SkPaint paint; paint.setBlendMode(SkBlendMode::kSrc); // set as is, including alpha surface->getCanvas()->drawBitmap(bitmap, 0, 0, &paint); - return surface->makeImageSnapshot(); + return makeCheckedImageSnapshot(surface); } // Try to fall back in non-debug builds. SAL_WARN("vcl.skia", @@ -454,6 +460,24 @@ sk_sp<SkImage> createSkImage(const SkBitmap& bitmap) return image; } +sk_sp<SkImage> makeCheckedImageSnapshot(sk_sp<SkSurface> surface) +{ + sk_sp<SkImage> ret = surface->makeImageSnapshot(); + assert(ret); + if (ret) + return ret; + abort(); +} + +sk_sp<SkImage> makeCheckedImageSnapshot(sk_sp<SkSurface> surface, const SkIRect& bounds) +{ + sk_sp<SkImage> ret = surface->makeImageSnapshot(bounds); + assert(ret); + if (ret) + return ret; + abort(); +} + namespace { // Image cache, for saving results of complex operations such as drawTransformedBitmap(). @@ -573,7 +597,7 @@ void dump(const SkBitmap& bitmap, const char* file) { dump(SkImage::MakeFromBitm void dump(const sk_sp<SkSurface>& surface, const char* file) { surface->getCanvas()->flush(); - dump(surface->makeImageSnapshot(), file); + dump(makeCheckedImageSnapshot(surface), file); } void dump(const sk_sp<SkImage>& image, const char* file) diff --git a/vcl/skia/gdiimpl.cxx b/vcl/skia/gdiimpl.cxx index 1821f48a7cce..698d20730751 100644 --- a/vcl/skia/gdiimpl.cxx +++ b/vcl/skia/gdiimpl.cxx @@ -308,7 +308,7 @@ void SkiaSalGraphicsImpl::createWindowSurface(bool forceRaster) destroySurface(); // destroys also WindowContext return createWindowSurface(true); // try again case SkiaHelper::RenderRaster: - abort(); // this should not really happen + abort(); // This should not really happen, do not even try to cope with it. } } mIsGPU = mSurface->getCanvas()->getGrContext() != nullptr; @@ -438,7 +438,7 @@ void SkiaSalGraphicsImpl::checkSurface() if (!isOffscreen()) { flushDrawing(); - snapshot = mSurface->makeImageSnapshot(); + snapshot = SkiaHelper::makeCheckedImageSnapshot(mSurface); } recreateSurface(); if (snapshot) @@ -595,7 +595,7 @@ void SkiaSalGraphicsImpl::applyXor() SkPaint paint; paint.setBlendMode(SkBlendMode::kSrc); // copy as is SkCanvas canvas(surfaceBitmap); - canvas.drawImageRect(mSurface->makeImageSnapshot(), mXorRegion.getBounds(), + canvas.drawImageRect(SkiaHelper::makeCheckedImageSnapshot(mSurface), mXorRegion.getBounds(), SkRect::Make(mXorRegion.getBounds()), &paint); // xor to surfaceBitmap assert(surfaceBitmap.info().alphaType() == kUnpremul_SkAlphaType); @@ -1110,7 +1110,7 @@ static void copyArea(SkCanvas* canvas, sk_sp<SkSurface> surface, long nDestX, lo { SkPaint paint; paint.setBlendMode(SkBlendMode::kSrc); // copy as is, including alpha - canvas->drawImageRect(surface->makeImageSnapshot(), + canvas->drawImageRect(SkiaHelper::makeCheckedImageSnapshot(surface), SkIRect::MakeXYWH(nSrcX, nSrcY, nSrcWidth, nSrcHeight), SkRect::MakeXYWH(nDestX, nDestY, nSrcWidth, nSrcHeight), &paint); return; @@ -1178,7 +1178,7 @@ void SkiaSalGraphicsImpl::copyBits(const SalTwoRect& rPosAry, SalGraphics* pSrcG { SAL_INFO("vcl.skia.trace", "copybits(" << this << "): (" << src << "): " << rPosAry); // Do not use makeImageSnapshot(rect), as that one may make a needless data copy. - sk_sp<SkImage> image = src->mSurface->makeImageSnapshot(); + sk_sp<SkImage> image = SkiaHelper::makeCheckedImageSnapshot(src->mSurface); SkPaint paint; paint.setBlendMode(SkBlendMode::kSrc); // copy as is, including alpha if (rPosAry.mnSrcWidth != rPosAry.mnDestWidth @@ -1305,7 +1305,8 @@ std::shared_ptr<SalBitmap> SkiaSalGraphicsImpl::getBitmap(long nX, long nY, long // TODO makeImageSnapshot(rect) may copy the data, which may be a waste if this is used // e.g. for VirtualDevice's lame alpha blending, in which case the image will eventually end up // in blendAlphaBitmap(), where we could simply use the proper rect of the image. - sk_sp<SkImage> image = mSurface->makeImageSnapshot(SkIRect::MakeXYWH(nX, nY, nWidth, nHeight)); + sk_sp<SkImage> image = SkiaHelper::makeCheckedImageSnapshot( + mSurface, SkIRect::MakeXYWH(nX, nY, nWidth, nHeight)); return std::make_shared<SkiaSalBitmap>(image); } @@ -1364,10 +1365,12 @@ void SkiaSalGraphicsImpl::invert(basegfx::B2DPolygon const& rPoly, SalInvert eFl SkPaint copy; copy.setBlendMode(SkBlendMode::kSrc); flushDrawing(); - surface->getCanvas()->drawImageRect(mSurface->makeImageSnapshot(), area, size, ©); + surface->getCanvas()->drawImageRect(SkiaHelper::makeCheckedImageSnapshot(mSurface), + area, size, ©); aPath.offset(-area.x(), -area.y()); surface->getCanvas()->drawPath(aPath, aPaint); - getDrawCanvas()->drawImageRect(surface->makeImageSnapshot(), size, area, ©); + getDrawCanvas()->drawImageRect(SkiaHelper::makeCheckedImageSnapshot(surface), size, + area, ©); } } else @@ -1412,10 +1415,12 @@ void SkiaSalGraphicsImpl::invert(basegfx::B2DPolygon const& rPoly, SalInvert eFl SkPaint copy; copy.setBlendMode(SkBlendMode::kSrc); flushDrawing(); - surface->getCanvas()->drawImageRect(mSurface->makeImageSnapshot(), area, size, ©); + surface->getCanvas()->drawImageRect(SkiaHelper::makeCheckedImageSnapshot(mSurface), + area, size, ©); aPath.offset(-area.x(), -area.y()); surface->getCanvas()->drawPath(aPath, aPaint); - getDrawCanvas()->drawImageRect(surface->makeImageSnapshot(), size, area, ©); + getDrawCanvas()->drawImageRect(SkiaHelper::makeCheckedImageSnapshot(surface), size, + area, ©); } addXorRegion(aPath.getBounds()); } @@ -1537,7 +1542,7 @@ sk_sp<SkImage> SkiaSalGraphicsImpl::mergeCacheBitmaps(const SkiaSalBitmap& bitma } else canvas->drawImage(bitmap.GetSkImage(), 0, 0, &paint); - image = tmpSurface->makeImageSnapshot(); + image = SkiaHelper::makeCheckedImageSnapshot(tmpSurface); SkiaHelper::addCachedImage(key, image); return image; } diff --git a/vcl/skia/salbmp.cxx b/vcl/skia/salbmp.cxx index d933b33e6ac2..3c5b21ef1461 100644 --- a/vcl/skia/salbmp.cxx +++ b/vcl/skia/salbmp.cxx @@ -407,7 +407,7 @@ bool SkiaSalBitmap::ConvertToGreyscale() mBitCount = 8; ComputeScanlineSize(); mPalette = Bitmap::GetGreyPalette(256); - ResetToSkImage(surface->makeImageSnapshot()); + ResetToSkImage(SkiaHelper::makeCheckedImageSnapshot(surface)); SAL_INFO("vcl.skia.trace", "converttogreyscale(" << this << ")"); return true; } @@ -613,7 +613,7 @@ const sk_sp<SkImage>& SkiaSalBitmap::GetSkImage() const assert(surface); surface->getCanvas()->clear(toSkColor(mEraseColor)); SkiaSalBitmap* thisPtr = const_cast<SkiaSalBitmap*>(this); - thisPtr->mImage = surface->makeImageSnapshot(); + thisPtr->mImage = SkiaHelper::makeCheckedImageSnapshot(surface); SAL_INFO("vcl.skia.trace", "getskimage(" << this << ") from erase color " << mEraseColor); return mImage; } @@ -653,7 +653,7 @@ const sk_sp<SkImage>& SkiaSalBitmap::GetSkImage() const << "->" << mSize << ":" << static_cast<int>(mScaleQuality)); SkiaSalBitmap* thisPtr = const_cast<SkiaSalBitmap*>(this); - thisPtr->mImage = surface->makeImageSnapshot(); + thisPtr->mImage = SkiaHelper::makeCheckedImageSnapshot(surface); } return mImage; } @@ -685,7 +685,7 @@ const sk_sp<SkImage>& SkiaSalBitmap::GetAlphaSkImage() const assert(surface); surface->getCanvas()->clear(fromEraseColorToAlphaImageColor(mEraseColor)); SkiaSalBitmap* thisPtr = const_cast<SkiaSalBitmap*>(this); - thisPtr->mAlphaImage = surface->makeImageSnapshot(); + thisPtr->mAlphaImage = SkiaHelper::makeCheckedImageSnapshot(surface); SAL_INFO("vcl.skia.trace", "getalphaskimage(" << this << ") from erase color " << mEraseColor); return mAlphaImage; @@ -730,7 +730,7 @@ const sk_sp<SkImage>& SkiaSalBitmap::GetAlphaSkImage() const // generally only happen with the separate-alpha-outdev hack, and those bitmaps should // be temporary. SkiaSalBitmap* thisPtr = const_cast<SkiaSalBitmap*>(this); - thisPtr->mAlphaImage = surface->makeImageSnapshot(); + thisPtr->mAlphaImage = SkiaHelper::makeCheckedImageSnapshot(surface); return mAlphaImage; } SkiaZone zone; @@ -769,7 +769,7 @@ const sk_sp<SkImage>& SkiaSalBitmap::GetAlphaSkImage() const paint.setColorFilter(SkColorFilters::Matrix(redToAlpha)); surface->getCanvas()->drawBitmap(GetAsSkBitmap(), 0, 0, &paint); SkiaSalBitmap* thisPtr = const_cast<SkiaSalBitmap*>(this); - thisPtr->mAlphaImage = surface->makeImageSnapshot(); + thisPtr->mAlphaImage = SkiaHelper::makeCheckedImageSnapshot(surface); } // The data is now stored both in the SkImage and in our mBuffer, so drop the buffer // if conserving memory and the conversion back would be simple (it'll be converted back diff --git a/vcl/skia/win/gdiimpl.cxx b/vcl/skia/win/gdiimpl.cxx index 5c5582b8ffe6..ce67db42914b 100644 --- a/vcl/skia/win/gdiimpl.cxx +++ b/vcl/skia/win/gdiimpl.cxx @@ -313,7 +313,7 @@ sk_sp<SkImage> SkiaCompatibleDC::getAsImage() const SkRect::MakeXYWH(0, 0, maRects.mnSrcWidth, maRects.mnSrcHeight), SkRect::MakeXYWH(0, 0, maRects.mnSrcWidth, maRects.mnSrcHeight), &paint); canvas->restore(); - return surface->makeImageSnapshot(); + return SkiaHelper::makeCheckedImageSnapshot(surface); } sk_sp<SkImage> SkiaCompatibleDC::getAsImageDiff(const SkiaCompatibleDC& white) const @@ -362,7 +362,7 @@ sk_sp<SkImage> SkiaCompatibleDC::getAsImageDiff(const SkiaCompatibleDC& white) c canvas->concat(matrix); canvas->drawBitmap(tmpBitmap, 0, 0, &paint); canvas->restore(); - return surface->makeImageSnapshot(); + return SkiaHelper::makeCheckedImageSnapshot(surface); } SkiaControlsCache::SkiaControlsCache() _______________________________________________ Libreoffice-commits mailing list [email protected] https://lists.freedesktop.org/mailman/listinfo/libreoffice-commits
