[Libreoffice-commits] core.git: vcl/inc vcl/skia
Luboš Luňák (via logerrit)
logerrit at kemper.freedesktop.org
Wed Sep 23 06:46:57 UTC 2020
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 <l.lunak at collabora.com>
AuthorDate: Tue Sep 22 13:42:02 2020 +0200
Commit: Luboš Luňák <l.lunak at collabora.com>
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 <l.lunak at collabora.com>
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()
More information about the Libreoffice-commits
mailing list