Adding size parameter to read array functions

Adding size parameter to read array functions

In some cases, the allocated array into which the data will be read is using getArrayCount() to allocate itself, which should be safe, but some cases use fixed length arrays or compute the array size before reading, which could overflow if the stream is compromised.

To prevent that from happening, I added a check that will verify that the number of bytes to read will not exceed the capacity of the input buffer argument passed to all the read...Array() functions.

I chose to use the byte array for this initial version, so that "size" represents the same value across all read...Array() functions, but I could also use the element count, if it is preferred.

Note : readPointArray and writePointArray are unused, so I could also remove them

BUG=

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

[reed1, 2013-10-23 21:10:10]

https://codereview.chromium.org/37803002/diff/1/include/core/SkFlattenableBuf...
File include/core/SkFlattenableBuffers.h (right):

https://codereview.chromium.org/37803002/diff/1/include/core/SkFlattenableBuf...
include/core/SkFlattenableBuffers.h:102: virtual uint32_t readByteArray(void*
value, uint32_t size) = 0;
Lets add some dox here:

/**
 * The size parameter specifies the allocation size (in bytes? in elements?) of
the pointer parameter).
 */

[sugoi1, 2013-10-24 11:45:45]

https://codereview.chromium.org/37803002/diff/1/include/core/SkFlattenableBuf...
File include/core/SkFlattenableBuffers.h (right):

https://codereview.chromium.org/37803002/diff/1/include/core/SkFlattenableBuf...
include/core/SkFlattenableBuffers.h:102: virtual uint32_t readByteArray(void*
value, uint32_t size) = 0;
On 2013/10/23 21:10:10, reed1 wrote:
> Lets add some dox here:
> 
> /**
>  * The size parameter specifies the allocation size (in bytes? in elements?)
of
> the pointer parameter).
>  */

Done.

[reed1, 2013-10-24 16:10:32]

https://codereview.chromium.org/37803002/diff/50001/include/core/SkFlattenabl...
File include/core/SkFlattenableBuffers.h (right):

https://codereview.chromium.org/37803002/diff/50001/include/core/SkFlattenabl...
include/core/SkFlattenableBuffers.h:105: * the allocation size (in bytes) of the
pointer parameter.
// What does the return value indicate? What is the behavior if the buffer-size
is smaller than the underlying 'array'? [assert, crash, error w/ no read, error
w/ clamped read]? What is the state of the buffer object if there is a mismatch?

[sugoi1, 2013-10-24 19:40:04]

https://codereview.chromium.org/37803002/diff/50001/include/core/SkFlattenabl...
File include/core/SkFlattenableBuffers.h (right):

https://codereview.chromium.org/37803002/diff/50001/include/core/SkFlattenabl...
include/core/SkFlattenableBuffers.h:105: * the allocation size (in bytes) of the
pointer parameter.
On 2013/10/24 16:10:33, reed1 wrote:
> // What does the return value indicate? What is the behavior if the
buffer-size
> is smaller than the underlying 'array'? [assert, crash, error w/ no read,
error
> w/ clamped read]? What is the state of the buffer object if there is a
mismatch?

Done.

[reed1, 2013-10-29 19:39:42]

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

https://codereview.chromium.org/37803002/diff/180001/include/core/SkFlattenab...
include/core/SkFlattenableBuffers.h:109: * If isValidating() is false, then the
size parameter is ignored.
What gets return if there is *not* an error? (in both validating and
non-validating buffers)

Seems like non-validating could perform checks:
- only memcpy the min of the embedded size and the provided size

https://codereview.chromium.org/37803002/diff/180001/include/core/SkFlattenab...
include/core/SkFlattenableBuffers.h:111: virtual uint32_t readByteArray(void*
value, uint32_t size) = 0;
shouldn't these be size_t instead of uint32_t, just for clarity/consistency
sake? Same comment for the return value.

[reed1, 2013-10-29 19:44:09]

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

https://codereview.chromium.org/37803002/diff/180001/include/core/SkFlattenab...
include/core/SkFlattenableBuffers.h:111: virtual uint32_t readByteArray(void*
value, uint32_t size) = 0;
What is the state of the buffer if we get an error? How much has the cursor been
advanced?

[sugoi1, 2013-10-29 20:04:50]

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

https://codereview.chromium.org/37803002/diff/180001/include/core/SkFlattenab...
include/core/SkFlattenableBuffers.h:109: * If isValidating() is false, then the
size parameter is ignored.
On 2013/10/29 19:39:43, reed1 wrote:
> What gets return if there is *not* an error? (in both validating and
non-validating buffers)
It returns the size (or amount) of memory actually read. I'll update the
description.

> Seems like non-validating could perform checks:
> - only memcpy the min of the embedded size and the provided size
Will add. I just wanted to minimize changes to non-validating code, but adding
this should be trivial.

https://codereview.chromium.org/37803002/diff/180001/include/core/SkFlattenab...
include/core/SkFlattenableBuffers.h:111: virtual uint32_t readByteArray(void*
value, uint32_t size) = 0;
On 2013/10/29 19:44:09, reed1 wrote:
> What is the state of the buffer if we get an error? How much has the cursor
been advanced?

Will add in the description. Thanks.

https://codereview.chromium.org/37803002/diff/180001/include/core/SkFlattenab...
include/core/SkFlattenableBuffers.h:111: virtual uint32_t readByteArray(void*
value, uint32_t size) = 0;
On 2013/10/29 19:39:43, reed1 wrote:
> shouldn't these be size_t instead of uint32_t, just for clarity/consistency
sake? Same comment for the return value.

Sure, but I'll just mention that, currently, the underlying reader is
SkReader32, so using uint32_t prevents overflowing the reader's memory with a
potentially 64b size_t, on 64b platforms (For example, SkReader32::size(),
SkReader32::offset() and SkReader32::available() all return a uint32_t). Now, I
understand that this interface should be reader agnostic, so I'll change it.
Maybe the reader could support 64b on 64b platforms eventually ?

[sugoi1, 2013-10-30 14:26:12]

Fixed comments and added a test.

[reed1, 2013-10-30 14:50:30]

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

https://codereview.chromium.org/37803002/diff/280001/include/core/SkFlattenab...
include/core/SkFlattenableBuffers.h:110: * If isValidating() is false, then the
size parameter is used as the maximum amount of memory
It seems like we're saying the following:

if we're validating
    - size must be == actual size
    - we return 0 or size

if we're not validating
    - size can be anything
    - we return 0 or min(size, actual_size)

Can we unify these, so the behaviors (modulo setting the error field perhaps)
are the same?
    1. can we always require == sizes?
    2. if so, should this guy just return a bool?

[sugoi1, 2013-10-30 17:43:50]

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

https://codereview.chromium.org/37803002/diff/280001/include/core/SkFlattenab...
include/core/SkFlattenableBuffers.h:110: * If isValidating() is false, then the
size parameter is used as the maximum amount of memory
On 2013/10/30 14:50:30, reed1 wrote:
> It seems like we're saying the following:
> 
> if we're validating
>     - size must be == actual size
>     - we return 0 or size
> 
> if we're not validating
>     - size can be anything
>     - we return 0 or min(size, actual_size)
> 
> Can we unify these, so the behaviors (modulo setting the error field perhaps)
> are the same?
>     1. can we always require == sizes?
>     2. if so, should this guy just return a bool?
> 
> 
> 

Done.

[reed1, 2013-10-30 17:50:11]

The dox say that in the case of an error (size mismatch) the cursor will not be
affected.

- validating does affect the cursor (at least by the count)
- nonvalidating does not affect the cursor

[reed1, 2013-10-30 17:59:54]

I propose we change readArray to always move the cursor to the end of the stream
if there is an error (either truncated input or the size parameter does not
match the embedded size). It is heavy-weight, but makes it easy to document, and
allows both backends (validating and non-validating) to be very similar.

thoughts?

[sugoi1, 2013-10-30 18:12:51]

On 2013/10/30 17:50:11, reed1 wrote:
> The dox say that in the case of an error (size mismatch) the cursor will not
be
> affected.
> 
> - validating does affect the cursor (at least by the count)
> - nonvalidating does not affect the cursor

Validating should not affect the cursor in case of an error. Remember that if
fError is already true, skip() will no longer move the cursor.

[sugoi1, 2013-10-30 18:14:32]

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

https://codereview.chromium.org/37803002/diff/350001/src/core/SkValidatingRea...
src/core/SkValidatingReadBuffer.cpp:155: (void)this->skip(sizeof(uint32_t)); //
Skip array count
Here, the reason why I changed readUInt() for :
1 ) getArrayCount()
2 ) error check
3 ) skip
is that if an error is set at step 2, then skip will not move the cursor

[sugoi1, 2013-10-30 18:19:47]

On 2013/10/30 17:59:54, reed1 wrote:
> I propose we change readArray to always move the cursor to the end of the
stream
> if there is an error (either truncated input or the size parameter does not
> match the embedded size). It is heavy-weight, but makes it easy to document,
and
> allows both backends (validating and non-validating) to be very similar.
> 
> thoughts?

It's a bit less intuitive to implement that in the validating case, because,
right now, the cursor no longer moves once an error is found.
I can change it to skipping to the end if you want to, but then shouldn't that
mean that it should always skip to the end whenever an error is found ?

[reed1, 2013-10-30 18:36:49]

On 2013/10/30 18:19:47, sugoi1 wrote:
> On 2013/10/30 17:59:54, reed1 wrote:
> > I propose we change readArray to always move the cursor to the end of the
> stream
> > if there is an error (either truncated input or the size parameter does not
> > match the embedded size). It is heavy-weight, but makes it easy to document,
> and
> > allows both backends (validating and non-validating) to be very similar.
> > 
> > thoughts?
> 
> It's a bit less intuitive to implement that in the validating case, because,
> right now, the cursor no longer moves once an error is found.
> I can change it to skipping to the end if you want to, but then shouldn't that
> mean that it should always skip to the end whenever an error is found ?

I think this is a key question. If we encounter an error... should we ever trust
the stream again?

If so, then we have to be terribly clear about where the cursor is at all times
after the error was encountered.

If not, moving the cursor to the end is an easy way to to
- ensure subsequent reads will fail
- document where the cursor is after an error occurs

[sugoi1, 2013-10-30 20:02:52]

- Cleaned up the code by adding a few handy template functions
- Hunted down the last calls to read...Array() that weren't using the return
value properly.

[reed1, 2013-10-30 20:22:31]

lgtm w/ (future) nit about using templates when a local static function would do
(which would give the compiler a little more permission to share code as
needed).

https://codereview.chromium.org/37803002/diff/500001/src/core/SkOrderedReadBu...
File src/core/SkOrderedReadBuffer.cpp (right):

https://codereview.chromium.org/37803002/diff/500001/src/core/SkOrderedReadBu...
src/core/SkOrderedReadBuffer.cpp:140: template <typename T> bool
SkOrderedReadBuffer::readArray(T* value, size_t size) {
if we care about code bloat at all, this can trivially be turned into a
non-template function, and still be shared/efficient... just pass fReader and
sizeof(T) as parameters.

https://codereview.chromium.org/37803002/diff/500001/src/core/SkValidatingRea...
File src/core/SkValidatingReadBuffer.h (right):

https://codereview.chromium.org/37803002/diff/500001/src/core/SkValidatingRea...
src/core/SkValidatingReadBuffer.h:63: virtual void validate(bool isValid)
SK_OVERRIDE {
at some point, we should move all of these virtual methods into the .cpp

[Stephen White, 2013-10-30 20:33:20]

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

https://codereview.chromium.org/37803002/diff/500001/include/core/SkFlattenab...
include/core/SkFlattenableBuffers.h:114: virtual bool readByteArray(void* value,
size_t size) = 0;
Sorry to get back so late on this one, but would it be better to use a size in
elements, rather than a size in bytes? It seems like it would be a clearer API,
and puts the onus on the implementation to worry about bytes, and lets the
callers just deal with elements.

(That would require changing this particular function to readCharArray(char*,
...) or something.)

https://codereview.chromium.org/37803002/diff/500001/src/core/SkColorTable.cpp
File src/core/SkColorTable.cpp (right):

https://codereview.chromium.org/37803002/diff/500001/src/core/SkColorTable.cp...
src/core/SkColorTable.cpp:97: SkDEBUGCODE(const size_t countRead =)
buffer.readColorArray(fColors, size);
Doesn't this assign a bool to a size_t, since the return value type changed?
Probably should check if it compiles in debug.

In fact, the check below is wrong too, since fCount is in elements, and
countRead (was) in bytes.

https://codereview.chromium.org/37803002/diff/500001/src/effects/SkEmbossMask...
File src/effects/SkEmbossMaskFilter.cpp (right):

https://codereview.chromium.org/37803002/diff/500001/src/effects/SkEmbossMask...
src/effects/SkEmbossMaskFilter.cpp:135: buffer.readByteArray(&fLight,
sizeof(Light));
Shouldn't we be calling validate() on the result? Or is that now handled
internally?

https://codereview.chromium.org/37803002/diff/500001/tests/SerializationTest.cpp
File tests/SerializationTest.cpp (right):

https://codereview.chromium.org/37803002/diff/500001/tests/SerializationTest....
tests/SerializationTest.cpp:40: DEFINE_TESTCLASS("Serialization",
SerializationClass, Tests)
IWBN to have tests for all of the readFoo() functions, to see that validation
fails when truncated.

[reed1, 2013-10-30 21:05:28]

On 2013/10/30 20:33:20, Stephen White wrote:
>
https://codereview.chromium.org/37803002/diff/500001/include/core/SkFlattenab...
> File include/core/SkFlattenableBuffers.h (right):
> 
>
https://codereview.chromium.org/37803002/diff/500001/include/core/SkFlattenab...
> include/core/SkFlattenableBuffers.h:114: virtual bool readByteArray(void*
value,
> size_t size) = 0;
> Sorry to get back so late on this one, but would it be better to use a size in
> elements, rather than a size in bytes? It seems like it would be a clearer
API,
> and puts the onus on the implementation to worry about bytes, and lets the
> callers just deal with elements.
> 
> (That would require changing this particular function to readCharArray(char*,
> ...) or something.)
> 
> https://codereview.chromium.org/37803002/diff/500001/src/core/SkColorTable.cpp
> File src/core/SkColorTable.cpp (right):
> 
>
https://codereview.chromium.org/37803002/diff/500001/src/core/SkColorTable.cp...
> src/core/SkColorTable.cpp:97: SkDEBUGCODE(const size_t countRead =)
> buffer.readColorArray(fColors, size);
> Doesn't this assign a bool to a size_t, since the return value type changed?
> Probably should check if it compiles in debug.
> 
> In fact, the check below is wrong too, since fCount is in elements, and
> countRead (was) in bytes.
> 
>
https://codereview.chromium.org/37803002/diff/500001/src/effects/SkEmbossMask...
> File src/effects/SkEmbossMaskFilter.cpp (right):
> 
>
https://codereview.chromium.org/37803002/diff/500001/src/effects/SkEmbossMask...
> src/effects/SkEmbossMaskFilter.cpp:135: buffer.readByteArray(&fLight,
> sizeof(Light));
> Shouldn't we be calling validate() on the result? Or is that now handled
> internally?
> 
>
https://codereview.chromium.org/37803002/diff/500001/tests/SerializationTest.cpp
> File tests/SerializationTest.cpp (right):
> 
>
https://codereview.chromium.org/37803002/diff/500001/tests/SerializationTest....
> tests/SerializationTest.cpp:40: DEFINE_TESTCLASS("Serialization",
> SerializationClass, Tests)
> IWBN to have tests for all of the readFoo() functions, to see that validation
> fails when truncated.

+1 for tests

[sugoi1, 2013-10-31 14:16:23]

- Added tests for all read...Array() functions
- Changed size parameter from size (in bytes) to size (in elements)
- Fixed bad code in SkColorTable.cpp

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

https://codereview.chromium.org/37803002/diff/500001/include/core/SkFlattenab...
include/core/SkFlattenableBuffers.h:114: virtual bool readByteArray(void* value,
size_t size) = 0;
On 2013/10/30 20:33:20, Stephen White wrote:
> Sorry to get back so late on this one, but would it be better to use a size in
> elements, rather than a size in bytes? It seems like it would be a clearer
API,
> and puts the onus on the implementation to worry about bytes, and lets the
> callers just deal with elements.
> 
> (That would require changing this particular function to readCharArray(char*,
> ...) or something.)

Done. Although I kept void* and assumed that the size parameter is in bytes.

https://codereview.chromium.org/37803002/diff/500001/src/core/SkColorTable.cpp
File src/core/SkColorTable.cpp (right):

https://codereview.chromium.org/37803002/diff/500001/src/core/SkColorTable.cp...
src/core/SkColorTable.cpp:97: SkDEBUGCODE(const size_t countRead =)
buffer.readColorArray(fColors, size);
On 2013/10/30 20:33:20, Stephen White wrote:
> Doesn't this assign a bool to a size_t, since the return value type changed?
> Probably should check if it compiles in debug.
> 
> In fact, the check below is wrong too, since fCount is in elements, and
> countRead (was) in bytes.

Done.

https://codereview.chromium.org/37803002/diff/500001/src/core/SkOrderedReadBu...
File src/core/SkOrderedReadBuffer.cpp (right):

https://codereview.chromium.org/37803002/diff/500001/src/core/SkOrderedReadBu...
src/core/SkOrderedReadBuffer.cpp:140: template <typename T> bool
SkOrderedReadBuffer::readArray(T* value, size_t size) {
On 2013/10/30 20:22:32, reed1 wrote:
> if we care about code bloat at all, this can trivially be turned into a
> non-template function, and still be shared/efficient... just pass fReader and
> sizeof(T) as parameters.

Done.

https://codereview.chromium.org/37803002/diff/500001/src/core/SkValidatingRea...
File src/core/SkValidatingReadBuffer.h (right):

https://codereview.chromium.org/37803002/diff/500001/src/core/SkValidatingRea...
src/core/SkValidatingReadBuffer.h:63: virtual void validate(bool isValid)
SK_OVERRIDE {
On 2013/10/30 20:22:32, reed1 wrote:
> at some point, we should move all of these virtual methods into the .cpp

Done.

https://codereview.chromium.org/37803002/diff/500001/src/effects/SkEmbossMask...
File src/effects/SkEmbossMaskFilter.cpp (right):

https://codereview.chromium.org/37803002/diff/500001/src/effects/SkEmbossMask...
src/effects/SkEmbossMaskFilter.cpp:135: buffer.readByteArray(&fLight,
sizeof(Light));
On 2013/10/30 20:33:20, Stephen White wrote:
> Shouldn't we be calling validate() on the result? Or is that now handled
> internally?

Yeah, this is done internally. If false is returned, an error is already set
inside SkValidatingReadBuffer.

https://codereview.chromium.org/37803002/diff/500001/tests/SerializationTest.cpp
File tests/SerializationTest.cpp (right):

https://codereview.chromium.org/37803002/diff/500001/tests/SerializationTest....
tests/SerializationTest.cpp:40: DEFINE_TESTCLASS("Serialization",
SerializationClass, Tests)
On 2013/10/30 20:33:20, Stephen White wrote:
> IWBN to have tests for all of the readFoo() functions, to see that validation
> fails when truncated.

Done.

[commit-bot: I haz the power, 2013-10-31 14:33:21]

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

[Stephen White, 2013-10-31 15:19:28]

https://codereview.chromium.org/37803002/diff/700001/src/core/SkOrderedReadBu...
File src/core/SkOrderedReadBuffer.cpp (right):

https://codereview.chromium.org/37803002/diff/700001/src/core/SkOrderedReadBu...
src/core/SkOrderedReadBuffer.cpp:142: if (count == size) {
Nit: prefer the early-out to be the error case rather than the valid one.

https://codereview.chromium.org/37803002/diff/700001/src/core/SkOrderedReadBu...
src/core/SkOrderedReadBuffer.cpp:144: const size_t byteLength = count *
elementSize;
Should we be checking for overflow here? At worst, I suppose we'll end up
reading fewer bytes, so I don't think there's a danger of buffer overflow, but I
could be wrong (there would probably be overflow at the malloc in the callsite
too).

https://codereview.chromium.org/37803002/diff/700001/tests/SerializationTest.cpp
File tests/SerializationTest.cpp (right):

https://codereview.chromium.org/37803002/diff/700001/tests/SerializationTest....
tests/SerializationTest.cpp:150: DEFINE_TESTCLASS("Serialization",
SerializationClass, Tests)
Thanks for the extra tests.

I was thinking we should also have truncation tests for the serializable objects
themselves (e.g., SkLightingImageFilter, etc). Perhaps this could be integrated
into the fuzzer: maybe 1 out of 10 passes it could randomly truncate the stream.

[commit-bot: I haz the power, 2013-10-31 18:38:02]

Change committed as 12058

[Tom Sepez, 2013-11-08 20:59:00]

https://codereview.chromium.org/37803002/diff/700001/src/core/SkFlattenableBu...
File src/core/SkFlattenableBuffers.cpp (right):

https://codereview.chromium.org/37803002/diff/700001/src/core/SkFlattenableBu...
src/core/SkFlattenableBuffers.cpp:40: this->readByteArray(&proc, sizeof(void*));
drive-by: Code like this always scares the crap out of us, because although it
looks like this may not get called in the inter-process case based upon flags in
the current implementation, something might change introducing the easiest
possible escape ever. Please try to remove this and use another mechanism even
in the intra-process case.
Besides, the code is not correct in that casting a function to void* is wrong
per C specs (see e.g.
http://stackoverflow.com/questions/1096341/function-pointers-casting-in-c).

[sugoi, 2013-11-08 21:31:15]

https://codereview.chromium.org/37803002/diff/700001/src/core/SkFlattenableBu...
File src/core/SkFlattenableBuffers.cpp (right):

https://codereview.chromium.org/37803002/diff/700001/src/core/SkFlattenableBu...
src/core/SkFlattenableBuffers.cpp:40: this->readByteArray(&proc, sizeof(void*));
On 2013/11/08 20:59:01, Tom Sepez wrote:
> drive-by: Code like this always scares the crap out of us, because although it
> looks like this may not get called in the inter-process case based upon flags
in
> the current implementation, something might change introducing the easiest
> possible escape ever. Please try to remove this and use another mechanism even
> in the intra-process case.
> Besides, the code is not correct in that casting a function to void* is wrong
> per C specs (see e.g.
> http://stackoverflow.com/questions/1096341/function-pointers-casting-in-c).

Thanks for your comment ! Yes, code like this was one of the main motivations
for the rewrite of a new more secure serialization class in skia (see
SkValidatingReadBuffer). The original serialization code in Skia was written for
speed, not for security, which is why it had to be redesigned and rewritten. If
performance allows it, maybe we could eventually modify Skia to only use the
secure serialization, but there's still code using the old one for now (in Skia,
not in Chromium, AFAIK). Maybe we could find a way to make these functions only
accessible in Skia (as in #ifdefed out and not even compiled in Chrome, since
Chrome will not use this). What do you think ?

[Tom Sepez, 2013-11-08 21:33:48]

On 2013/11/08 21:31:15, sugoi wrote:
>
https://codereview.chromium.org/37803002/diff/700001/src/core/SkFlattenableBu...
> File src/core/SkFlattenableBuffers.cpp (right):
> 
>
https://codereview.chromium.org/37803002/diff/700001/src/core/SkFlattenableBu...
> src/core/SkFlattenableBuffers.cpp:40: this->readByteArray(&proc,
sizeof(void*));
> On 2013/11/08 20:59:01, Tom Sepez wrote:
> > drive-by: Code like this always scares the crap out of us, because although
it
> > looks like this may not get called in the inter-process case based upon
flags
> in
> > the current implementation, something might change introducing the easiest
> > possible escape ever. Please try to remove this and use another mechanism
even
> > in the intra-process case.
> > Besides, the code is not correct in that casting a function to void* is
wrong
> > per C specs (see e.g.
> > http://stackoverflow.com/questions/1096341/function-pointers-casting-in-c).
> 
> Thanks for your comment ! Yes, code like this was one of the main motivations
> for the rewrite of a new more secure serialization class in skia (see
> SkValidatingReadBuffer). The original serialization code in Skia was written
for
> speed, not for security, which is why it had to be redesigned and rewritten.
If
> performance allows it, maybe we could eventually modify Skia to only use the
> secure serialization, but there's still code using the old one for now (in
Skia,
> not in Chromium, AFAIK). Maybe we could find a way to make these functions
only
> accessible in Skia (as in #ifdefed out and not even compiled in Chrome, since
> Chrome will not use this). What do you think ?

Yes, #ifdefs are a good approach for now.  Thanks.