diff options
author | Greg Daniel <egdaniel@google.com> | 2017-09-26 20:07:58 +0000 |
---|---|---|
committer | Skia Commit-Bot <skia-commit-bot@chromium.org> | 2017-09-26 20:08:07 +0000 |
commit | f46633f8af5296341e33ec4cdb69c71dfa997396 (patch) | |
tree | d6d59760f023f6855d921253d684716ae93bdc28 /src | |
parent | 58e23039bd4a8aee2c67fa6c9e5105b925d2e561 (diff) |
Revert "guard old apis for querying byte-size of a bitmap/imageinfo/pixmap"
This reverts commit 98a6216b18b57c2f7a0d58f542c60503686aed69.
Reason for revert: breaking the chrome roll. Looks like they may be writing data to create an image across all the row bytes and thus writing to unalloced data on the last row. Link to example failing bot:
https://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_ng/builds/539960
Original change's description:
> guard old apis for querying byte-size of a bitmap/imageinfo/pixmap
>
> Previously we had size_t and uint64_t variations.
>
> The new (simpler) API always..
> - returns size_t, or 0 if the calculation overflowed
> - returns the trimmed size (does not include rowBytes padding for the last row)
>
> Bug: skia:
> Change-Id: I05173e877918327c7b207d2f7f1ab0db36892e2e
> Reviewed-on: https://skia-review.googlesource.com/50980
> Commit-Queue: Mike Reed <reed@google.com>
> Reviewed-by: Florin Malita <fmalita@chromium.org>
> Reviewed-by: Leon Scroggins <scroggo@google.com>
TBR=mtklein@google.com,herb@google.com,scroggo@google.com,fmalita@chromium.org,reed@google.com
Change-Id: I726f6ab1b36b14979ba6f37105e0a469b3f0dbc0
No-Presubmit: true
No-Tree-Checks: true
No-Try: true
Bug: skia:
Reviewed-on: https://skia-review.googlesource.com/51262
Reviewed-by: Greg Daniel <egdaniel@google.com>
Commit-Queue: Greg Daniel <egdaniel@google.com>
Diffstat (limited to 'src')
-rw-r--r-- | src/android/SkBitmapRegionCodec.cpp | 2 | ||||
-rw-r--r-- | src/codec/SkIcoCodec.cpp | 2 | ||||
-rw-r--r-- | src/codec/SkSampler.cpp | 4 | ||||
-rw-r--r-- | src/codec/SkWebpCodec.cpp | 2 | ||||
-rw-r--r-- | src/core/SkAutoPixmapStorage.cpp | 4 | ||||
-rw-r--r-- | src/core/SkBitmap.cpp | 2 | ||||
-rw-r--r-- | src/core/SkBitmapCache.cpp | 4 | ||||
-rw-r--r-- | src/core/SkImageInfo.cpp | 12 | ||||
-rw-r--r-- | src/core/SkMallocPixelRef.cpp | 46 | ||||
-rw-r--r-- | src/core/SkScalerContext.cpp | 2 | ||||
-rw-r--r-- | src/core/SkSpecialImage.cpp | 2 | ||||
-rw-r--r-- | src/gpu/ops/GrSmallPathRenderer.cpp | 4 | ||||
-rw-r--r-- | src/image/SkImage.cpp | 5 | ||||
-rw-r--r-- | src/image/SkImage_Gpu.cpp | 17 | ||||
-rw-r--r-- | src/image/SkImage_Raster.cpp | 4 | ||||
-rw-r--r-- | src/image/SkSurface_Raster.cpp | 2 | ||||
-rw-r--r-- | src/utils/mac/SkCreateCGImageRef.cpp | 2 |
17 files changed, 56 insertions, 60 deletions
diff --git a/src/android/SkBitmapRegionCodec.cpp b/src/android/SkBitmapRegionCodec.cpp index 493e4b5672..f77c70264f 100644 --- a/src/android/SkBitmapRegionCodec.cpp +++ b/src/android/SkBitmapRegionCodec.cpp @@ -96,7 +96,7 @@ bool SkBitmapRegionCodec::decodeRegion(SkBitmap* bitmap, SkBRDAllocator* allocat if (SubsetType::kPartiallyInside_SubsetType == type && SkCodec::kNo_ZeroInitialized == zeroInit) { void* pixels = bitmap->getPixels(); - size_t bytes = outInfo.computeByteSize(bitmap->rowBytes()); + size_t bytes = outInfo.getSafeSize(bitmap->rowBytes()); memset(pixels, 0, bytes); } diff --git a/src/codec/SkIcoCodec.cpp b/src/codec/SkIcoCodec.cpp index da6d0c360c..74affe6709 100644 --- a/src/codec/SkIcoCodec.cpp +++ b/src/codec/SkIcoCodec.cpp @@ -173,7 +173,7 @@ std::unique_ptr<SkCodec> SkIcoCodec::MakeFromStream(std::unique_ptr<SkStream> st int maxIndex = 0; for (int i = 0; i < codecs->count(); i++) { SkImageInfo info = codecs->operator[](i)->getInfo(); - size_t size = info.computeMinByteSize(); + size_t size = info.getSafeSize(info.minRowBytes()); if (size > maxSize) { maxSize = size; diff --git a/src/codec/SkSampler.cpp b/src/codec/SkSampler.cpp index d18410be3b..c7d9a3ac23 100644 --- a/src/codec/SkSampler.cpp +++ b/src/codec/SkSampler.cpp @@ -14,8 +14,8 @@ void SkSampler::Fill(const SkImageInfo& info, void* dst, size_t rowBytes, uint64_t colorOrIndex, SkCodec::ZeroInitialized zeroInit) { SkASSERT(dst != nullptr); - // Calculate bytes to fill. - const size_t bytesToFill = info.computeByteSize(rowBytes); + // Calculate bytes to fill. We use getSafeSize since the last row may not be padded. + const size_t bytesToFill = info.getSafeSize(rowBytes); const int width = info.width(); const int numRows = info.height(); diff --git a/src/codec/SkWebpCodec.cpp b/src/codec/SkWebpCodec.cpp index 864fa96a1d..f7082de42a 100644 --- a/src/codec/SkWebpCodec.cpp +++ b/src/codec/SkWebpCodec.cpp @@ -542,7 +542,7 @@ SkCodec::Result SkWebpCodec::onGetPixels(const SkImageInfo& dstInfo, void* dst, config.output.u.RGBA.rgba = reinterpret_cast<uint8_t*>(webpDst.getAddr(dstX, dstY)); config.output.u.RGBA.stride = static_cast<int>(webpDst.rowBytes()); - config.output.u.RGBA.size = webpDst.computeByteSize(); + config.output.u.RGBA.size = webpDst.getSafeSize(); SkAutoTCallVProc<WebPIDecoder, WebPIDelete> idec(WebPIDecode(nullptr, 0, &config)); if (!idec) { diff --git a/src/core/SkAutoPixmapStorage.cpp b/src/core/SkAutoPixmapStorage.cpp index df0c0fa878..be13e7197f 100644 --- a/src/core/SkAutoPixmapStorage.cpp +++ b/src/core/SkAutoPixmapStorage.cpp @@ -29,7 +29,7 @@ size_t SkAutoPixmapStorage::AllocSize(const SkImageInfo& info, size_t* rowBytes) if (rowBytes) { *rowBytes = rb; } - return info.computeByteSize(rb); + return info.getSafeSize(rb); } bool SkAutoPixmapStorage::tryAlloc(const SkImageInfo& info) { @@ -58,7 +58,7 @@ const SkData* SkAutoPixmapStorage::detachPixelsAsData() { return nullptr; } - auto data = SkData::MakeFromMalloc(fStorage, this->computeByteSize()); + auto data = SkData::MakeFromMalloc(fStorage, this->getSafeSize()); fStorage = nullptr; this->INHERITED::reset(); diff --git a/src/core/SkBitmap.cpp b/src/core/SkBitmap.cpp index 4a82d54990..c3bc4a9d63 100644 --- a/src/core/SkBitmap.cpp +++ b/src/core/SkBitmap.cpp @@ -675,7 +675,7 @@ bool SkBitmap::ReadRawPixels(SkReadBuffer* buffer, SkBitmap* bitmap) { } // write_raw_pixels() always writes snug buffers with rowBytes == minRowBytes(). - size_t bytes = info.computeMinByteSize(); + size_t bytes = info.getSafeSize(info.minRowBytes()); if (!buffer->validate(bytes != 0)) { return false; } diff --git a/src/core/SkBitmapCache.cpp b/src/core/SkBitmapCache.cpp index 8d3991a9d5..b8767644a2 100644 --- a/src/core/SkBitmapCache.cpp +++ b/src/core/SkBitmapCache.cpp @@ -160,7 +160,7 @@ public: const Key& getKey() const override { return fKey; } size_t bytesUsed() const override { - return sizeof(fKey) + fInfo.computeByteSize(fRowBytes); + return sizeof(fKey) + fInfo.getSafeSize(fRowBytes); } bool canBePurged() override { SkAutoMutexAcquire ama(fMutex); @@ -289,7 +289,7 @@ SkBitmapCache::RecPtr SkBitmapCache::Alloc(const SkBitmapCacheDesc& desc, const } const size_t rb = info.minRowBytes(); - size_t size = info.computeByteSize(rb); + size_t size = info.getSafeSize(rb); if (0 == size) { return nullptr; } diff --git a/src/core/SkImageInfo.cpp b/src/core/SkImageInfo.cpp index 353397e99d..1baf0b77de 100644 --- a/src/core/SkImageInfo.cpp +++ b/src/core/SkImageInfo.cpp @@ -70,18 +70,6 @@ static SkColorType stored_to_live(unsigned stored) { /////////////////////////////////////////////////////////////////////////////////////////////////// -#include "SkSafeMath.h" - -size_t SkImageInfo::computeByteSize(size_t rowBytes) const { - if (0 == fHeight) { - return 0; - } - SkSafeMath safe; - size_t bytes = safe.add(safe.mul(fHeight - 1, rowBytes), - safe.mul(fWidth, this->bytesPerPixel())); - return safe ? bytes : 0; -} - static bool alpha_type_is_valid(SkAlphaType alphaType) { return (alphaType >= kUnknown_SkAlphaType) && (alphaType <= kLastEnum_SkAlphaType); } diff --git a/src/core/SkMallocPixelRef.cpp b/src/core/SkMallocPixelRef.cpp index 565a757ec2..6928f38ee2 100644 --- a/src/core/SkMallocPixelRef.cpp +++ b/src/core/SkMallocPixelRef.cpp @@ -35,28 +35,37 @@ sk_sp<SkPixelRef> SkMallocPixelRef::MakeDirect(const SkImageInfo& info, } -sk_sp<SkPixelRef> SkMallocPixelRef::MakeUsing(void*(*allocProc)(size_t), - const SkImageInfo& info, - size_t rowBytes) { - if (rowBytes == 0) { - rowBytes = info.minRowBytes(); + sk_sp<SkPixelRef> SkMallocPixelRef::MakeUsing(void*(*alloc)(size_t), + const SkImageInfo& info, + size_t requestedRowBytes) { + if (!is_valid(info)) { + return nullptr; } - if (!is_valid(info) || !info.validRowBytes(rowBytes)) { - return nullptr; + // only want to permit 31bits of rowBytes + int64_t minRB = (int64_t)info.minRowBytes64(); + if (minRB < 0 || !sk_64_isS32(minRB)) { + return nullptr; // allocation will be too large + } + if (requestedRowBytes > 0 && (int32_t)requestedRowBytes < minRB) { + return nullptr; // cannot meet requested rowbytes } - size_t size = 0; - // if the info is empty, or rowBytes is 0 (which can be valid), then we don't need to compute - // a size. - if (!info.isEmpty() && rowBytes > 0) { - size = info.computeByteSize(rowBytes); - if (size == 0) { - return nullptr; // overflow - } + int32_t rowBytes; + if (requestedRowBytes) { + rowBytes = SkToS32(requestedRowBytes); + } else { + rowBytes = minRB; } - void* addr = allocProc(size); + int64_t bigSize = (int64_t)info.height() * rowBytes; + if (!sk_64_isS32(bigSize)) { + return nullptr; + } + + size_t size = sk_64_asS32(bigSize); + SkASSERT(size >= info.getSafeSize(rowBytes)); + void* addr = alloc(size); if (nullptr == addr) { return nullptr; } @@ -98,11 +107,10 @@ sk_sp<SkPixelRef> SkMallocPixelRef::MakeWithData(const SkImageInfo& info, size_t rowBytes, sk_sp<SkData> data) { SkASSERT(data != nullptr); - if (!is_valid(info) || !info.validRowBytes(rowBytes)) { + if (!is_valid(info)) { return nullptr; } - size_t sizeNeeded = info.computeByteSize(rowBytes); - if (sizeNeeded == 0 || sizeNeeded > data->size()) { + if ((rowBytes < info.minRowBytes()) || (data->size() < info.getSafeSize(rowBytes))) { return nullptr; } // must get this address before we call release diff --git a/src/core/SkScalerContext.cpp b/src/core/SkScalerContext.cpp index 460907f164..98036cf8a0 100644 --- a/src/core/SkScalerContext.cpp +++ b/src/core/SkScalerContext.cpp @@ -400,7 +400,7 @@ static void generateMask(const SkMask& mask, const SkPath& path, } else { dst.reset(info, mask.fImage, dstRB); } - sk_bzero(dst.writable_addr(), dst.computeByteSize()); + sk_bzero(dst.writable_addr(), dst.getSafeSize()); SkDraw draw; draw.fDst = dst; diff --git a/src/core/SkSpecialImage.cpp b/src/core/SkSpecialImage.cpp index db4fadcce2..db40329204 100644 --- a/src/core/SkSpecialImage.cpp +++ b/src/core/SkSpecialImage.cpp @@ -219,7 +219,7 @@ public: SkAlphaType alphaType() const override { return fBitmap.alphaType(); } - size_t getSize() const override { return fBitmap.computeByteSize(); } + size_t getSize() const override { return fBitmap.getSize(); } void onDraw(SkCanvas* canvas, SkScalar x, SkScalar y, const SkPaint* paint) const override { SkRect dst = SkRect::MakeXYWH(x, y, diff --git a/src/gpu/ops/GrSmallPathRenderer.cpp b/src/gpu/ops/GrSmallPathRenderer.cpp index 222f6626a7..fd868a1dfe 100644 --- a/src/gpu/ops/GrSmallPathRenderer.cpp +++ b/src/gpu/ops/GrSmallPathRenderer.cpp @@ -455,7 +455,7 @@ private: devPathBounds.height()))) { return false; } - sk_bzero(dst.writable_addr(), dst.computeByteSize()); + sk_bzero(dst.writable_addr(), dst.getSafeSize()); // rasterize path SkPaint paint; @@ -562,7 +562,7 @@ private: devPathBounds.height()))) { return false; } - sk_bzero(dst.writable_addr(), dst.computeByteSize()); + sk_bzero(dst.writable_addr(), dst.getSafeSize()); // rasterize path SkPaint paint; diff --git a/src/image/SkImage.cpp b/src/image/SkImage.cpp index 705efaec30..3ac8ba80ca 100644 --- a/src/image/SkImage.cpp +++ b/src/image/SkImage.cpp @@ -422,10 +422,7 @@ sk_sp<SkImage> SkImageMakeRasterCopyAndAssignColorSpace(const SkImage* src, } size_t rowBytes = info.minRowBytes(); - size_t size = info.computeByteSize(rowBytes); - if (size == 0) { - return nullptr; - } + size_t size = info.getSafeSize(rowBytes); auto data = SkData::MakeUninitialized(size); if (!data) { return nullptr; diff --git a/src/image/SkImage_Gpu.cpp b/src/image/SkImage_Gpu.cpp index 1a79d59356..d3a1865039 100644 --- a/src/image/SkImage_Gpu.cpp +++ b/src/image/SkImage_Gpu.cpp @@ -518,7 +518,7 @@ sk_sp<SkImage> SkImage::makeNonTextureImage() const { } SkImageInfo info = as_IB(this)->onImageInfo(); size_t rowBytes = info.minRowBytes(); - size_t size = info.computeByteSize(rowBytes); + size_t size = info.getSafeSize(rowBytes); auto data = SkData::MakeUninitialized(size); if (!data) { return nullptr; @@ -667,7 +667,7 @@ size_t SkImage::getDeferredTextureImageData(const GrContextThreadSafeProxy& prox size_t pixelSize = 0; if (!isScaled && this->peekPixels(&pixmap) && pixmap.info().colorType() == dstColorType) { info = pixmap.info(); - pixelSize = SkAlign8(pixmap.computeByteSize()); + pixelSize = SkAlign8(pixmap.getSafeSize()); if (!dstColorSpace) { pixmap.setColorSpace(nullptr); info = info.makeColorSpace(nullptr); @@ -773,7 +773,7 @@ size_t SkImage::getDeferredTextureImageData(const GrContextThreadSafeProxy& prox void* pixels = pixelsAsCharPtr; memcpy(reinterpret_cast<void*>(SkAlign8(reinterpret_cast<uintptr_t>(pixelsAsCharPtr))), - pixmap.addr(), pixmap.computeByteSize()); + pixmap.addr(), pixmap.getSafeSize()); // If the context has sRGB support, and we're intending to render to a surface with an attached // color space, and the image has an sRGB-like color space attached, then use our gamma (sRGB) @@ -824,7 +824,7 @@ size_t SkImage::getDeferredTextureImageData(const GrContextThreadSafeProxy& prox } // Fill in the mipmap levels if they exist - char* mipLevelPtr = pixelsAsCharPtr + SkAlign8(pixmap.computeByteSize()); + char* mipLevelPtr = pixelsAsCharPtr + SkAlign8(pixmap.getSafeSize()); if (useMipMaps) { static_assert(std::is_standard_layout<MipMapLevelData>::value, @@ -845,10 +845,13 @@ size_t SkImage::getDeferredTextureImageData(const GrContextThreadSafeProxy& prox // Make sure the mipmap data starts before the end of the buffer SkASSERT(mipLevelPtr < bufferAsCharPtr + pixelOffset + pixelSize); // Make sure the mipmap data ends before the end of the buffer - SkASSERT(mipLevelPtr + mipLevel.fPixmap.computeByteSize() <= + SkASSERT(mipLevelPtr + mipLevel.fPixmap.getSafeSize() <= bufferAsCharPtr + pixelOffset + pixelSize); - memcpy(mipLevelPtr, mipLevel.fPixmap.addr(), mipLevel.fPixmap.computeByteSize()); + // getSafeSize includes rowbyte padding except for the last row, + // right? + + memcpy(mipLevelPtr, mipLevel.fPixmap.addr(), mipLevel.fPixmap.getSafeSize()); memcpy(bufferAsCharPtr + offsetof(DeferredTextureImage, fMipMapLevelData) + sizeof(MipMapLevelData) * (generatedMipLevelIndex + 1) + @@ -858,7 +861,7 @@ size_t SkImage::getDeferredTextureImageData(const GrContextThreadSafeProxy& prox sizeof(MipMapLevelData) * (generatedMipLevelIndex + 1) + offsetof(MipMapLevelData, fRowBytes), &rowBytes, sizeof(rowBytes)); - mipLevelPtr += SkAlign8(mipLevel.fPixmap.computeByteSize()); + mipLevelPtr += SkAlign8(mipLevel.fPixmap.getSafeSize()); } } return size; diff --git a/src/image/SkImage_Raster.cpp b/src/image/SkImage_Raster.cpp index 5258f64b3e..bade1b6235 100644 --- a/src/image/SkImage_Raster.cpp +++ b/src/image/SkImage_Raster.cpp @@ -57,11 +57,11 @@ public: if (kUnknown_SkColorType == info.colorType()) { return false; } - if (!info.validRowBytes(rowBytes)) { + if (rowBytes < info.minRowBytes()) { return false; } - size_t size = info.computeByteSize(rowBytes); + size_t size = info.getSafeSize(rowBytes); if (0 == size) { return false; } diff --git a/src/image/SkSurface_Raster.cpp b/src/image/SkSurface_Raster.cpp index e9d14fea7d..38694876c3 100644 --- a/src/image/SkSurface_Raster.cpp +++ b/src/image/SkSurface_Raster.cpp @@ -162,7 +162,7 @@ void SkSurface_Raster::onCopyOnWrite(ContentChangeMode mode) { fBitmap.allocPixels(); SkASSERT(prev.info() == fBitmap.info()); SkASSERT(prev.rowBytes() == fBitmap.rowBytes()); - memcpy(fBitmap.getPixels(), prev.getPixels(), fBitmap.computeByteSize()); + memcpy(fBitmap.getPixels(), prev.getPixels(), fBitmap.getSafeSize()); } SkASSERT(fBitmap.rowBytes() == fRowBytes); // be sure we always use the same value diff --git a/src/utils/mac/SkCreateCGImageRef.cpp b/src/utils/mac/SkCreateCGImageRef.cpp index 1514ba6e8a..fd55d91707 100644 --- a/src/utils/mac/SkCreateCGImageRef.cpp +++ b/src/utils/mac/SkCreateCGImageRef.cpp @@ -132,7 +132,7 @@ CGImageRef SkCreateCGImageRefWithColorspace(const SkBitmap& bm, const int w = bitmap->width(); const int h = bitmap->height(); - const size_t s = bitmap->computeByteSize(); + const size_t s = bitmap->getSize(); // our provider "owns" the bitmap*, and will take care of deleting it CGDataProviderRef dataRef = CGDataProviderCreateWithData(bitmap, bitmap->getPixels(), s, |