Adding error checks to SkRBuffer

Adding error checks to SkRBuffer

BUG=

Committed: http://code.google.com/p/skia/source/detail?r=12202

[robertphillips, 2013-11-06 13:07:25]

lgtm. How much of a perf hit are we going to take for this though?

[sugoi1, 2013-11-06 13:36:06]

On 2013/11/06 13:07:25, robertphillips wrote:
> lgtm. How much of a perf hit are we going to take for this though?

This class is only used for reading SkPath, SkRegion and
SkFontConfigInterface::FontIdentity. In each case, the number of initializations
to 0 will be :

SkPath : 6 (all 32 int)
SkRegion : 3 (all 32 int)
SkFontConfigInterface::FontIdentity : 6 (5 x 32 int + 1 x 8 bit unsigned)

I'm not sure how significant this can be as compared with the rest of the
reading operations. There are other solutions that may have less impact, but
that require a bit more logic, like changing SkRBufferWithSizeCheck::read() to :

void SkRBufferWithSizeCheck::read(void* buffer, size_t size) {
    fError = fError || (fPos + size > fStop);
    if (size > 0) {
        if (!fError) {
            readNoSizeCheck(buffer, size);
        } else if ((NULL != buffer) && (size <= 4)) { // The size check here
would allow us to avoid zeroing out large buffers in case of an error
            memset(buffer, 0, size);
        }
    }
}

Another solution would change line 1142 in SkRegion.cpp to :

    if (buffer.isValid() && (count >= 0)) {

Although this doesn't solve the generic issue, just this single instance of the
issue.

I have no problem changing the solution if another solution is preferred.

[sugoi1, 2013-11-06 15:32:01]

Adding Mike as a reviewer to get his opinion on the matter.

[reed1, 2013-11-06 15:39:26]

What gets better with this change? Are zeros some how secure, where random bytes
are not? Should instead the caller check the buffer afterwards to see if it ran
past eof?

[robertphillips, 2013-11-06 15:42:52]

We are getting the following valgrind complaint on the bots:

[02:44:23.773820] ==2823== Thread 8:
[02:44:23.773925] ==2823== Conditional jump or move depends on uninitialised
value(s)
[02:44:23.773963] ==2823==    at 0x56349D: SkRegion::readFromMemory(void const*,
unsigned long) (SkRegion.cpp:1142)
[02:44:23.774009] ==2823==    by 0x576ED2:
SkValidatingReadBuffer::readRegion(SkRegion*) (SkValidatingReadBuffer.cpp:148)
[02:44:23.774052] ==2823==    by 0x48BAB4: Tests(skiatest::Reporter*)
(SerializationTest.cpp:50)
[02:44:23.774086] ==2823==    by 0x48CBB5:
skiatest::SerializationClass::onRun(skiatest::Reporter*)
(SerializationTest.cpp:212)
[02:44:23.774122] ==2823==    by 0x495B42: skiatest::Test::run() (Test.cpp:109)
[02:44:23.774155] ==2823==    by 0x48F1E0: SkTestRunnable::run()
(skia_test.cpp:155)
[02:44:23.774194] ==2823==    by 0x64A095: SkThreadPool::Loop(void*)
(SkThreadPool.cpp:97)
[02:44:23.774224] ==2823==    by 0x654B0A: thread_start(void*)
(SkThreadUtils_pthread.cpp:66)
[02:44:23.774257] ==2823==    by 0x4E39E99: start_thread (pthread_create.c:308)
[02:44:23.774289] ==2823==    by 0x6C71CCC: clone (clone.S:112)
[02:44:23.774325] ==2823==  Uninitialised value was created by a stack
allocation
[02:44:23.774358] ==2823==    at 0x563440: SkRegion::readFromMemory(void const*,
unsigned long) (SkRegion.cpp:1136)
[02:44:23.774396] ==2823==

[sugoi1, 2013-11-06 15:46:45]

On 2013/11/06 15:39:26, reed1 wrote:
> What gets better with this change? Are zeros some how secure, where random
bytes
> are not? Should instead the caller check the buffer afterwards to see if it
ran
> past eof?

Yes, I can do that, of course. I was hoping to have a deterministic behavior
when we get an error, though, which is why I was returning 0. That way, I can
add isValid() checks only where 0 would otherwise cause an issue while reading
the object.

[sugoi1, 2013-11-06 15:56:42]

Here's the new fix. It should only solve the current issue and let valgrind
signal other similar errors if they occur.

[robertphillips, 2013-11-06 16:46:57]

lgtm. Do we need to run your fuzzer locally with valgrind? Will Chromium be
doing that?

[sugoi1, 2013-11-06 18:31:42]

On 2013/11/06 16:46:57, robertphillips wrote:
> lgtm. Do we need to run your fuzzer locally with valgrind? Will Chromium be
doing that?

Chromium will be running the fuzzer with ASAN, most likely, not Valgrind, so it
may not pick up exactly the same errors. Also, I'm just starting with the fuzzer
and I don't know when it'll be ready.
It wouldn't be a bad idea to run the fuzzer locally with Valgrind, if we can,
but I guess we can't rely on that.

[reed1, 2013-11-06 22:04:43]

wow, if we need to solve just the current valgrind complaint, then fine. But...

If we're going to make region:readFromMemory safe, it needs to be rewritten. It
may also suggest a different api on RBuffer... e.g. all read(ptr, len) methods
return a bool for success, plus perhaps a query for bytesRemaining().

[sugoi1, 2013-11-07 13:05:48]

On 2013/11/06 22:04:43, reed1 wrote:
> wow, if we need to solve just the current valgrind complaint, then fine.
But...
> 
> If we're going to make region:readFromMemory safe, it needs to be rewritten.
It
> may also suggest a different api on RBuffer... e.g. all read(ptr, len) methods
> return a bool for success, plus perhaps a query for bytesRemaining().

I'll give it a try. I'm not sure if the bytesRemaining() function will be
required, but I'll add it if it can help me write these functions more clearly.

[sugoi1, 2013-11-07 16:42:30]

On 2013/11/07 13:05:48, sugoi1 wrote:
> I'll give it a try. I'm not sure if the bytesRemaining() function will be
> required, but I'll add it if it can help me write these functions more
clearly.

Ok, here's a first version of it.

[sugoi1, 2013-11-08 16:20:06]

Found and fixed a few issues.

https://codereview.chromium.org/61913002/diff/280001/src/core/SkPath.cpp
File src/core/SkPath.cpp (right):

https://codereview.chromium.org/61913002/diff/280001/src/core/SkPath.cpp#newc...
src/core/SkPath.cpp:2127: // If the buffer is not valid, pathRef should be NULL
Although pathRef should always be NULL if the buffer isn't valid, I added a
safety check here to make sure that it is the case.

https://codereview.chromium.org/61913002/diff/280001/src/core/SkValidatingRea...
File src/core/SkValidatingReadBuffer.cpp (right):

https://codereview.chromium.org/61913002/diff/280001/src/core/SkValidatingRea...
src/core/SkValidatingReadBuffer.cpp:29: return !fError;
Added this to get a way to check the validity of a readbuffer in the tests.

https://codereview.chromium.org/61913002/diff/280001/src/core/SkValidatingRea...
src/core/SkValidatingReadBuffer.cpp:125: this->validate((SkAlign4(size) == size)
&& (0 != size));
These conditions were upside down !

https://codereview.chromium.org/61913002/diff/280001/tests/SerializationTest.cpp
File tests/SerializationTest.cpp (left):

https://codereview.chromium.org/61913002/diff/280001/tests/SerializationTest....
tests/SerializationTest.cpp:116: REPORTER_ASSERT(reporter, peekBefore ==
peekAfter);
This test wasn't working properly. In this scenario, peekBefore != peekAfter,
since the error causes the buffer to skip to the end.

[bsalomon, 2013-11-08 18:05:11]

public header lgtm

[reed1, 2013-11-08 18:15:39]

https://codereview.chromium.org/61913002/diff/280001/include/core/SkFlattenab...
File include/core/SkFlattenableBuffers.h (right):

https://codereview.chromium.org/61913002/diff/280001/include/core/SkFlattenab...
include/core/SkFlattenableBuffers.h:147: virtual bool validate(bool isValid) {
return true; }
Lets keep the impls for virtuals in the .cpp

https://codereview.chromium.org/61913002/diff/280001/include/core/SkFlattenab...
include/core/SkFlattenableBuffers.h:147: virtual bool validate(bool isValid) {
return true; }
I think we need some dox for this guy, now that there are 4 combinations to
interpret :

true <-- (true)
false <-- (true)
true <-- (false)
false <-- (false)

https://codereview.chromium.org/61913002/diff/280001/src/core/SkPath.cpp
File src/core/SkPath.cpp (right):

https://codereview.chromium.org/61913002/diff/280001/src/core/SkPath.cpp#newc...
src/core/SkPath.cpp:2127: // If the buffer is not valid, pathRef should be NULL
On 2013/11/08 16:20:06, sugoi1 wrote:
> Although pathRef should always be NULL if the buffer isn't valid, I added a
> safety check here to make sure that it is the case.

should we call sk_throw()? Why break in the debug build, but not in release? Is
this really an error, or just mysterious?

[sugoi1, 2013-11-08 18:40:55]

https://codereview.chromium.org/61913002/diff/280001/include/core/SkFlattenab...
File include/core/SkFlattenableBuffers.h (right):

https://codereview.chromium.org/61913002/diff/280001/include/core/SkFlattenab...
include/core/SkFlattenableBuffers.h:147: virtual bool validate(bool isValid) {
return true; }
On 2013/11/08 18:15:39, reed1 wrote:
> Lets keep the impls for virtuals in the .cpp

Done.

https://codereview.chromium.org/61913002/diff/280001/include/core/SkFlattenab...
include/core/SkFlattenableBuffers.h:147: virtual bool validate(bool isValid) {
return true; }
On 2013/11/08 18:15:39, reed1 wrote:
> I think we need some dox for this guy, now that there are 4 combinations to
> interpret :
> 
> true <-- (true)
> false <-- (true)
> true <-- (false)
> false <-- (false)

Done.

https://codereview.chromium.org/61913002/diff/280001/src/core/SkPath.cpp
File src/core/SkPath.cpp (right):

https://codereview.chromium.org/61913002/diff/280001/src/core/SkPath.cpp#newc...
src/core/SkPath.cpp:2127: // If the buffer is not valid, pathRef should be NULL
On 2013/11/08 18:15:39, reed1 wrote:
> On 2013/11/08 16:20:06, sugoi1 wrote:
> > Although pathRef should always be NULL if the buffer isn't valid, I added a
> > safety check here to make sure that it is the case.
> 
> should we call sk_throw()? Why break in the debug build, but not in release?
Is
> this really an error, or just mysterious?

Done. Changed to sk_throw(). This should really never happen.

[reed1, 2013-11-08 19:08:38]

lgtm

[commit-bot: I haz the power, 2013-11-08 19:15:02]

CQ is trying da patch. Follow status at
https://skia-tree-status.appspot.com/cq/sugoi@chromium.org/61913002/290005

[commit-bot: I haz the power, 2013-11-08 19:23:01]

Change committed as 12202