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@collabora.com>
(cherry picked from commit 797315f220f82682748efaf80a29844a93f04f48)
Reviewed-on: https://gerrit.libreoffice.org/c/core/+/103189
Reviewed-by: Caolán McNamara <caolanm@redhat.com>

5 files changed, 55 insertions, 21 deletions

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, &copy);
+            surface->getCanvas()->drawImageRect(SkiaHelper::makeCheckedImageSnapshot(mSurface),
+                                                area, size, &copy);
             aPath.offset(-area.x(), -area.y());
             surface->getCanvas()->drawPath(aPath, aPaint);
-            getDrawCanvas()->drawImageRect(surface->makeImageSnapshot(), size, area, &copy);
+            getDrawCanvas()->drawImageRect(SkiaHelper::makeCheckedImageSnapshot(surface), size,
+                                           area, &copy);
         }
     }
     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, &copy);
+            surface->getCanvas()->drawImageRect(SkiaHelper::makeCheckedImageSnapshot(mSurface),
+                                                area, size, &copy);
             aPath.offset(-area.x(), -area.y());
             surface->getCanvas()->drawPath(aPath, aPaint);
-            getDrawCanvas()->drawImageRect(surface->makeImageSnapshot(), size, area, &copy);
+            getDrawCanvas()->drawImageRect(SkiaHelper::makeCheckedImageSnapshot(surface), size,
+                                           area, &copy);
         }
         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()