Initial error handling code

src/core/SkValidatingReadBuffer.cpp

Index: src/core/SkValidatingReadBuffer.cpp
diff --git a/src/core/SkValidatingReadBuffer.cpp b/src/core/SkValidatingReadBuffer.cpp
new file mode 100644
index 0000000000000000000000000000000000000000..bb33af55eac6d31054cc9661431acc5c5b8b49ad
--- /dev/null
+++ b/src/core/SkValidatingReadBuffer.cpp
@@ -0,0 +1,251 @@
+/*
+ * Copyright 2013 Google Inc.
+ *
+ * Use of this source code is governed by a BSD-style license that can be
+ * found in the LICENSE file.
+ */
+
+#include "SkBitmap.h"
+#include "SkErrorInternals.h"
+#include "SkValidatingReadBuffer.h"
+#include "SkStream.h"
+#include "SkTypeface.h"
+
+SkValidatingReadBuffer::SkValidatingReadBuffer() : INHERITED() {

[mtklein, 2013/10/07 19:29:56]

Are all these constructors in use?  Let's cut out any code we can get away with
not having in here.

[mtklein, 2013/10/07 19:29:56]

I think you can get away without calling INHERITED().  At least be consistent
across the constructors.

[sugoi1, 2013/10/08 20:23:10]

Done.

[sugoi1, 2013/10/08 20:23:10]

Done.

+    fMemoryPtr = NULL;
+
+    setFlags(SkFlattenableReadBuffer::kValidation_Flag);
+}
+
+SkValidatingReadBuffer::SkValidatingReadBuffer(const void* data, size_t size) : INHERITED()  {
+    this->setMemory(data, size);
+    fMemoryPtr = NULL;

[mtklein, 2013/10/07 19:29:56]

This is, um, confusing?  We're going to need names here that make it clearer
that setMemory and fMemoryPtr have nothing to do with each other here.  Maybe
fMemoryPtr-> fOwnedMemory?

[sugoi1, 2013/10/08 20:23:10]

I removed fMemoryPtr for now.

+
+    setFlags(SkFlattenableReadBuffer::kValidation_Flag);

[mtklein, 2013/10/07 19:29:56]

this->, and in a few other places

[sugoi1, 2013/10/08 20:23:10]

Done.

+}
+
+SkValidatingReadBuffer::SkValidatingReadBuffer(SkStream* stream) {

[mtklein, 2013/10/07 19:29:56]

We've got to be careful here.  Not every SkStream has a sensible getLength()
method.

We'll need to check hasLength() first, or use SkStreamAsset* which is a subclass
where hasLength() is always true and getLength() works how you'd think.

[sugoi1, 2013/10/08 20:23:10]

I removed the stream constructor for now, as it is unused. Only the data / size
constructor will be kept.

+    const size_t length = stream->getLength();
+    fMemoryPtr = sk_malloc_throw(length);
+    stream->read(fMemoryPtr, length);
+    this->setMemory(fMemoryPtr, length);
+
+    setFlags(SkFlattenableReadBuffer::kValidation_Flag);
+}
+
+SkValidatingReadBuffer::~SkValidatingReadBuffer() {
+    sk_free(fMemoryPtr);

[mtklein, 2013/10/07 19:29:56]

Would prefer it if you made fMemoryPtr an SkAutoFree or similar instead of an
explicit destructor.

[sugoi1, 2013/10/08 20:23:10]

fMemoryPtr is gone

+}
+
+void SkValidatingReadBuffer::setMemory(const void* data, size_t size) {
+    fError |= (!IsPtrAlign4(data) || (SkAlign4(size) != size));

[mtklein, 2013/10/07 19:29:56]

This is a nit, but it'd be nice to consistently parenthesize these types of
statements.

[mtklein, 2013/10/07 19:29:56]

Yeah, I think I would also prefer to work on booleans using the doubled
operators, |= -> ||=.  |= makes me think bits.

[sugoi1, 2013/10/08 20:23:10]

Done.

+    if (!fError) {
+        fReader.setMemory(data, size);
+    }
+}
+
+const void* SkValidatingReadBuffer::skip(size_t size) {
+    size_t inc = SkAlign4(size);

[mtklein, 2013/10/07 19:29:56]

const size_t inc, and below

[sugoi1, 2013/10/08 20:23:10]

Done.

+    const void* addr = fReader.peek();
+    fError |= !IsPtrAlign4(addr) || !fReader.isAvailable(inc);
+    if (!fError) {
+        fReader.skip(size);
+    }
+    return addr;
+}
+
+// All the methods in this file funnel down into either readInt() or skip() followed by a memcpy.
+// So we've got all our validation in readInt() and skip(); if they fail they'll return a zero
+// value or skip nothing, respectively.

[mtklein, 2013/10/07 19:29:56]

And I guess ReadScalar?  Add a note about how they'll set fError which should be
checked by callers?

[sugoi1, 2013/10/08 20:23:10]

Done.

+
+bool SkValidatingReadBuffer::readBool() {
+    return this->readInt() != 0;
+}
+
+SkColor SkValidatingReadBuffer::readColor() {
+    return this->readInt();
+}
+
+SkFixed SkValidatingReadBuffer::readFixed() {
+    return this->readInt();
+}
+
+int32_t SkValidatingReadBuffer::readInt() {
+    size_t inc = sizeof(int32_t);
+    fError |= !IsPtrAlign4(fReader.peek()) || !fReader.isAvailable(inc);
+    return fError ? 0 : fReader.readInt();
+}
+
+SkScalar SkValidatingReadBuffer::readScalar() {
+    size_t inc = sizeof(SkScalar);
+    fError |= !IsPtrAlign4(fReader.peek()) || !fReader.isAvailable(inc);
+    return fError ? 0 : fReader.readScalar();
+}
+
+uint32_t SkValidatingReadBuffer::readUInt() {
+    return this->readInt();
+}
+
+int32_t SkValidatingReadBuffer::read32() {
+    return this->readInt();
+}
+
+void SkValidatingReadBuffer::readString(SkString* string) {
+    size_t len = this->readInt();
+    const void* ptr = fReader.peek();
+
+    // skip over the string + '\0' and then pad to a multiple of 4
+    size_t alignedSize = SkAlign4(len + 1);
+    this->skip(alignedSize);
+    if (!fError) {
+        string->set((const char*)ptr, len);
+    }

[mtklein, 2013/10/07 19:29:56]

Shouldn't we be checking that there is indeed an \0 in here?  Or do we not care
because we're only copying len characters no matter what?

[sugoi1, 2013/10/08 20:23:10]

I added a check. Doesn't hurt to be extra careful.

+}
+
+void* SkValidatingReadBuffer::readEncodedString(size_t* length, SkPaint::TextEncoding encoding) {
+    int32_t encodingType = fReader.readInt();
+    if (encodingType == encoding) {
+        fError = true;

[mtklein, 2013/10/07 19:29:56]

for consistency, fError ||= (encodingType == encoding); ?

I don't quite get why this is an error.

Don't we want to bail out as early as possible if we fail?  Why continue to read
the length?

[sugoi1, 2013/10/08 20:23:10]

That was a mistake. Fixed it by writing :
fError = fError || (encodingType != encoding);

If we want to bail as early as we fail, I'd have to write something like :

    const int32_t encodingType = fReader.readInt();
    fError = fError || (encodingType != encoding);
    if (fError) {
        return NULL;
    }
    *length = this->readInt();
    if (fError) {
        return NULL;
    }
    const void* ptr = this->skip(SkAlign4(*length));
    if (fError) {
        return NULL;
    }

Seems overkill, since each of these functions check fError internally.

+    }
+    *length = this->readInt();
+    const void* ptr = this->skip(SkAlign4(*length));
+    void* data = NULL;
+    if (!fError) {
+        data = sk_malloc_throw(*length);
+        memcpy(data, ptr, *length);
+    }
+    return data;
+}
+
+void SkValidatingReadBuffer::readPoint(SkPoint* point) {
+    point->fX = fReader.readScalar();
+    point->fY = fReader.readScalar();
+}
+
+void SkValidatingReadBuffer::readMatrix(SkMatrix* matrix) {
+    size_t size = matrix->readFromMemory(fReader.peek());

[mtklein, 2013/10/07 19:29:56]

Don't we need to skip this memory first to make sure there's enough to read a
matrix?  Same for region, path, etc?

[sugoi1, 2013/10/08 20:23:10]

Chicken and egg problem. Right now, we don't know ahead of time how many bytes
we'll be reading (ok, in the matrix's case, it's always 9 * sizeof(SkScalar), so
I could fix that one), but we'd need to add functions to precompute the read
size for SkRegion and SkPath, so should I do that ?

+    fError |= (SkAlign4(size) != size);
+    if (!fError) {
+        (void)this->skip(size);
+    }
+}
+
+void SkValidatingReadBuffer::readIRect(SkIRect* rect) {
+    memcpy(rect, this->skip(sizeof(SkIRect)), sizeof(SkIRect));

[mtklein, 2013/10/07 19:29:56]

I'm not sure I understand the failure strategy of skip.

If skip fails, we'll just memcpy an SkIRect from where we started.  Isn't that
the same as the normal unsecure path?  Don't we need to check the result of skip
before trying a memcpy?  Same for rect, byte array, color array, etc?

[sugoi1, 2013/10/08 20:23:10]

Right, fixed.

+}
+
+void SkValidatingReadBuffer::readRect(SkRect* rect) {
+    memcpy(rect, this->skip(sizeof(SkRect)), sizeof(SkRect));
+}
+
+void SkValidatingReadBuffer::readRegion(SkRegion* region) {
+    size_t size = region->readFromMemory(fReader.peek());
+    fError |= (SkAlign4(size) != size);
+    if (!fError) {
+        (void)this->skip(size);
+    }
+}
+
+void SkValidatingReadBuffer::readPath(SkPath* path) {
+    size_t size = path->readFromMemory(fReader.peek());
+    fError |= (SkAlign4(size) != size);
+    if (!fError) {
+        (void)this->skip(size);
+    }
+}
+
+uint32_t SkValidatingReadBuffer::readByteArray(void* value) {
+    const uint32_t length = this->readUInt();
+    memcpy(value, this->skip(SkAlign4(length)), length);
+    return fError ? 0 : length;
+}
+
+uint32_t SkValidatingReadBuffer::readColorArray(SkColor* colors) {
+    const uint32_t count = this->readUInt();
+    const uint32_t byteLength = count * sizeof(SkColor);
+    memcpy(colors, this->skip(SkAlign4(byteLength)), byteLength);
+    return fError ? 0 : count;
+}
+
+uint32_t SkValidatingReadBuffer::readIntArray(int32_t* values) {
+    const uint32_t count = this->readUInt();
+    const uint32_t byteLength = count * sizeof(int32_t);
+    memcpy(values, this->skip(SkAlign4(byteLength)), byteLength);
+    return fError ? 0 : count;
+}
+
+uint32_t SkValidatingReadBuffer::readPointArray(SkPoint* points) {
+    const uint32_t count = this->readUInt();
+    const uint32_t byteLength = count * sizeof(SkPoint);
+    memcpy(points, this->skip(SkAlign4(byteLength)), byteLength);
+    return fError ? 0 : count;
+}
+
+uint32_t SkValidatingReadBuffer::readScalarArray(SkScalar* values) {
+    const uint32_t count = this->readUInt();
+    const uint32_t byteLength = count * sizeof(SkScalar);
+    memcpy(values, this->skip(SkAlign4(byteLength)), byteLength);
+    return fError ? 0 : count;
+}
+
+uint32_t SkValidatingReadBuffer::getArrayCount() {
+    return *(uint32_t*)fReader.peek();
+}
+
+void SkValidatingReadBuffer::readBitmap(SkBitmap* bitmap) {
+    const int width = this->readInt();
+    const int height = this->readInt();
+    const size_t length = this->readUInt();
+    // A size of zero means the SkBitmap was simply flattened.
+    if (length != 0) {

[mtklein, 2013/10/07 19:29:56]

fError ||= (length != 0); ?

[sugoi1, 2013/10/08 20:23:10]

Done.

+        fError = true;
+    }
+    if (fError) {
+        return;
+    }
+    bitmap->unflatten(*this);
+    if ((bitmap->width() != width) || (bitmap->height() != height)) {
+        fError = true;
+    }
+}
+
+SkFlattenable* SkValidatingReadBuffer::readFlattenable(const char* baseClassName) {
+    SkString name;
+    this->readString(&name);
+    if (fError) {
+        return NULL;
+    }
+
+    // Is this the type we wanted ?
+    const char* cname = name.c_str();
+    if (fError || !SkFlattenable::TypeIsA(cname, baseClassName)) {
+        return NULL;
+    }
+
+    SkFlattenable::Factory factory = SkFlattenable::NameToFactory(cname);
+    if (NULL == factory) {
+        return NULL; // writer failed to give us the flattenable
+    }
+
+    // if we get here, factory may still be null, but if that is the case, the
+    // failure was ours, not the writer.
+    SkFlattenable* obj = NULL;
+    uint32_t sizeRecorded = this->readUInt();
+    if (factory) {
+        uint32_t offset = fReader.offset();
+        obj = (*factory)(*this);
+        // check that we read the amount we expected
+        uint32_t sizeRead = fReader.offset() - offset;
+        if (sizeRecorded != sizeRead) {
+            // we could try to fix up the offset...
+            fError = true;
+            delete obj;
+            obj = NULL;
+        }
+    } else {
+        // we must skip the remaining data
+        this->skip(sizeRecorded);
+        SkASSERT(false);
+    }
+    return obj;
+}