Fixed invalid memory access issue in SkColorSpaceXform::apply()

Passing in a large buffer along with a source colour space that
used a CLUT would cause apply() to read freed heap memory, or
for smaller buffers read possibly re-used stack memory.
The code previously likely lucked out due to optimizations
removing most or all of the subsequent stack allocations.

BUG=skia:

GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2759

Change-Id: I39f357bce080c4d737a83dd019f0d1ccbc56f995
Reviewed-on: https://skia-review.googlesource.com/2759
Commit-Queue: Robert Aftias <raftias@google.com>
Reviewed-by: Matt Sarett <msarett@google.com>

diff --git a/src/core/SkColorSpaceXform.cpp b/src/core/SkColorSpaceXform.cpp
index 6f2f75b..1c0dd88 100644 (file)
--- a/src/core/SkColorSpaceXform.cpp
+++ b/src/core/SkColorSpaceXform.cpp
@@ -1353,15 +1353,15 @@ const
         }
     }
 
-    if (fColorLUT) {
-        size_t storageBytes = len * sizeof(uint32_t);
 #if defined(GOOGLE3)
-        // Stack frame size is limited in GOOGLE3.
-        SkAutoSMalloc<256 * sizeof(uint32_t)> storage(storageBytes);
+    // Stack frame size is limited in GOOGLE3.
+    SkAutoSMalloc<256 * sizeof(uint32_t)> storage;
 #else
-        SkAutoSMalloc<1024 * sizeof(uint32_t)> storage(storageBytes);
+    SkAutoSMalloc<1024 * sizeof(uint32_t)> storage;
 #endif
-
+    if (fColorLUT) {
+        size_t storageBytes = len * sizeof(uint32_t);
+        storage.reset(storageBytes);
         handle_color_lut((uint32_t*) storage.get(), src, len, fColorLUT.get());
         src = (const uint32_t*) storage.get();
     }

diff --git a/tests/ColorSpaceXformTest.cpp b/tests/ColorSpaceXformTest.cpp
index 0f74713..ed990fc 100644 (file)
--- a/tests/ColorSpaceXformTest.cpp
+++ b/tests/ColorSpaceXformTest.cpp
@@ -164,3 +164,24 @@ DEF_TEST(ColorSpaceXform_NonMatchingGamma, r) {
 
     test_identity_xform(r, gammas);
 }
+
+DEF_TEST(ColorSpaceXform_applyCLUTMemoryAccess, r) {
+    // buffers larger than 1024 (or 256 in GOOGLE3) will force ColorSpaceXform_Base::apply()
+    // to heap-allocate a buffer that is used for CLUT application, and this test is here to
+    // ensure that it no longer causes potential invalid memory accesses when this happens
+    const size_t len = 2048;
+    SkAutoTMalloc<uint32_t> src(len);
+    SkAutoTMalloc<uint32_t> dst(len);
+    for (uint32_t i = 0; i < len; ++i) {
+        src[i] = i;
+    }
+    // this ICC profile has a CLUT in it
+    const SkString filename(GetResourcePath("icc_profiles/upperRight.icc"));
+    sk_sp<SkData> iccData = SkData::MakeFromFileName(filename.c_str());
+    REPORTER_ASSERT_MESSAGE(r, iccData, "upperRight.icc profile required for test");
+    sk_sp<SkColorSpace> srcSpace = SkColorSpace::NewICC(iccData->bytes(), iccData->size());
+    sk_sp<SkColorSpace> dstSpace = SkColorSpace::NewNamed(SkColorSpace::kSRGB_Named);
+    auto xform = SkColorSpaceXform::New(srcSpace.get(), dstSpace.get());
+    xform->apply(dst.get(), src.get(), len, SkColorSpaceXform::kRGBA_8888_ColorFormat,
+                 SkColorSpaceXform::kRGBA_8888_ColorFormat, kUnpremul_SkAlphaType);
+}