[Libreoffice-commits] core.git: Branch 'libreoffice-7-0' - vcl/inc vcl/skia
Luboš Luňák (via logerrit)
logerrit at kemper.freedesktop.org
Wed Sep 23 13:39:57 UTC 2020
vcl/inc/skia/utils.hxx | 5 +++++
vcl/skia/SkiaHelper.cxx | 32 ++++++++++++++++++++++++++++----
vcl/skia/gdiimpl.cxx | 27 ++++++++++++++++-----------
vcl/skia/salbmp.cxx | 8 ++++----
vcl/skia/win/gdiimpl.cxx | 4 ++--
5 files changed, 55 insertions(+), 21 deletions(-)
New commits:
commit 7c0b0f6fa8ca294774401ddabb6773d707cb15b3
Author: Luboš Luňák <l.lunak at collabora.com>
AuthorDate: Tue Sep 22 13:42:02 2020 +0200
Commit: Caolán McNamara <caolanm at redhat.com>
CommitDate: Wed Sep 23 15:39:23 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>
(cherry picked from commit 797315f220f82682748efaf80a29844a93f04f48)
Reviewed-on: https://gerrit.libreoffice.org/c/core/+/103189
Reviewed-by: Caolán McNamara <caolanm at redhat.com>
diff --git a/vcl/inc/skia/utils.hxx b/vcl/inc/skia/utils.hxx
index e0fcf70c30e7..a43ff8f58bc5 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 3e02ef29cea2..343ebc284ceb 100644
--- a/vcl/skia/SkiaHelper.cxx
+++ b/vcl/skia/SkiaHelper.cxx
@@ -408,10 +408,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)
@@ -431,7 +437,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",
@@ -448,6 +454,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().
@@ -568,7 +592,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 a34bb4f0d4c1..743eea5d891a 100644
--- a/vcl/skia/gdiimpl.cxx
+++ b/vcl/skia/gdiimpl.cxx
@@ -299,7 +299,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;
@@ -420,7 +420,7 @@ void SkiaSalGraphicsImpl::checkSurface()
if (!isOffscreen())
{
flushDrawing();
- snapshot = mSurface->makeImageSnapshot();
+ snapshot = SkiaHelper::makeCheckedImageSnapshot(mSurface);
}
recreateSurface();
if (snapshot)
@@ -576,7 +576,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);
@@ -1094,7 +1094,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;
@@ -1162,7 +1162,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
@@ -1275,7 +1275,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);
}
@@ -1334,10 +1335,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
@@ -1382,10 +1385,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());
}
@@ -1527,7 +1532,7 @@ sk_sp<SkImage> SkiaSalGraphicsImpl::mergeCacheBitmaps(const SkiaSalBitmap& bitma
paint.setBlendMode(SkBlendMode::kDstOut); // VCL alpha is one-minus-alpha
canvas->drawImage(alphaBitmap->GetAlphaSkImage(), 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 9b4e0b3324b4..0f62f7dc5cfb 100644
--- a/vcl/skia/salbmp.cxx
+++ b/vcl/skia/salbmp.cxx
@@ -378,7 +378,7 @@ bool SkiaSalBitmap::ConvertToGreyscale()
surface->getCanvas()->drawImage(mImage, 0, 0, &paint);
mBitCount = 8;
mPalette = Bitmap::GetGreyPalette(256);
- ResetToSkImage(surface->makeImageSnapshot());
+ ResetToSkImage(SkiaHelper::makeCheckedImageSnapshot(surface));
SAL_INFO("vcl.skia.trace", "converttogreyscale(" << this << ")");
return true;
}
@@ -528,7 +528,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;
}
@@ -582,7 +582,7 @@ const sk_sp<SkImage>& SkiaSalBitmap::GetAlphaSkImage() const
else
SAL_INFO("vcl.skia.trace", "getalphaskimage(" << this << ") from image");
SkiaSalBitmap* thisPtr = const_cast<SkiaSalBitmap*>(this);
- thisPtr->mAlphaImage = surface->makeImageSnapshot();
+ thisPtr->mAlphaImage = SkiaHelper::makeCheckedImageSnapshot(surface);
return mAlphaImage;
}
SkiaZone zone;
@@ -621,7 +621,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);
}
SAL_INFO("vcl.skia.trace", "getalphaskimage(" << this << ")");
return mAlphaImage;
diff --git a/vcl/skia/win/gdiimpl.cxx b/vcl/skia/win/gdiimpl.cxx
index b79ed5f9c8f6..6e3e99ec8937 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