Validate allocation size in PoolAllocator.

Validate allocation size in PoolAllocator.
Note that I am planning to get rid of PoolAllocator entirely.

BUG=crbug 179654

Committed: https://code.google.com/p/angleproject/source/detail?r=2186

[aedla, 2013-04-18 12:18:35]

I wrote a clarification in the issue tracker to better explain the problem.

https://codereview.appspot.com/8662046/diff/1/src/compiler/PoolAlloc.cpp
File src/compiler/PoolAlloc.cpp (right):

https://codereview.appspot.com/8662046/diff/1/src/compiler/PoolAlloc.cpp#newc...
src/compiler/PoolAlloc.cpp:231: if (numBytes <= 0)
numBytes is unsigned, so never < 0.

In case numBytes == 0, we should succeed instead of returning NULL.

https://codereview.appspot.com/8662046/diff/1/src/compiler/PoolAlloc.cpp#newc...
src/compiler/PoolAlloc.cpp:240: assert(allocationSize >= 0);
allocationSize is unsigned, so always >= 0

https://codereview.appspot.com/8662046/diff/1/src/compiler/PoolAlloc.cpp#newc...
src/compiler/PoolAlloc.cpp:241: if (allocationSize <= 0)
Same problem as for numBytes above.

https://codereview.appspot.com/8662046/diff/1/src/compiler/PoolAlloc.cpp#newc...
src/compiler/PoolAlloc.cpp:254: if (currentPageOffset + allocationSize <=
pageSize) {
This addition can overflow. See the issue tracker for an example.

https://codereview.appspot.com/8662046/diff/1/src/compiler/PoolAlloc.cpp#newc...
src/compiler/PoolAlloc.cpp:265: if (allocationSize + headerSkip > pageSize) {
This can also overflow.

https://codereview.appspot.com/8662046/diff/1/src/compiler/PoolAlloc.h
File src/compiler/PoolAlloc.h (right):

https://codereview.appspot.com/8662046/diff/1/src/compiler/PoolAlloc.h#newcode70
src/compiler/PoolAlloc.h:70: assert(size >= 0);
size is unsigned so this check is always true

https://codereview.appspot.com/8662046/diff/1/src/compiler/PoolAlloc.h#newcode73
src/compiler/PoolAlloc.h:73: return (size < 0) || (allocSize < size) ? 0 :
allocSize;
size < 0 is always false.

Returning 0 in case of an overflow is a bad idea. The allocation should probably
just fail instead by crashing the whole process - I think assert() would be ok,
but there is usually something like ((void (*)())0)();

[Alok Priyadarshi, 2013-04-18 20:34:13]

PTAL

https://codereview.appspot.com/8662046/diff/1/src/compiler/PoolAlloc.cpp
File src/compiler/PoolAlloc.cpp (right):

https://codereview.appspot.com/8662046/diff/1/src/compiler/PoolAlloc.cpp#newc...
src/compiler/PoolAlloc.cpp:231: if (numBytes <= 0)
On 2013/04/18 12:18:35, aedla wrote:
> numBytes is unsigned, so never < 0.
> 
> In case numBytes == 0, we should succeed instead of returning NULL.

Done.

https://codereview.appspot.com/8662046/diff/1/src/compiler/PoolAlloc.cpp#newc...
src/compiler/PoolAlloc.cpp:240: assert(allocationSize >= 0);
On 2013/04/18 12:18:35, aedla wrote:
> allocationSize is unsigned, so always >= 0

Done.

https://codereview.appspot.com/8662046/diff/1/src/compiler/PoolAlloc.cpp#newc...
src/compiler/PoolAlloc.cpp:241: if (allocationSize <= 0)
On 2013/04/18 12:18:35, aedla wrote:
> Same problem as for numBytes above.

Done.

https://codereview.appspot.com/8662046/diff/1/src/compiler/PoolAlloc.cpp#newc...
src/compiler/PoolAlloc.cpp:254: if (currentPageOffset + allocationSize <=
pageSize) {
On 2013/04/18 12:18:35, aedla wrote:
> This addition can overflow. See the issue tracker for an example.

Done.

https://codereview.appspot.com/8662046/diff/1/src/compiler/PoolAlloc.cpp#newc...
src/compiler/PoolAlloc.cpp:265: if (allocationSize + headerSkip > pageSize) {
On 2013/04/18 12:18:35, aedla wrote:
> This can also overflow.

Done.

https://codereview.appspot.com/8662046/diff/1/src/compiler/PoolAlloc.h
File src/compiler/PoolAlloc.h (right):

https://codereview.appspot.com/8662046/diff/1/src/compiler/PoolAlloc.h#newcode70
src/compiler/PoolAlloc.h:70: assert(size >= 0);
On 2013/04/18 12:18:35, aedla wrote:
> size is unsigned so this check is always true

Done.

https://codereview.appspot.com/8662046/diff/1/src/compiler/PoolAlloc.h#newcode73
src/compiler/PoolAlloc.h:73: return (size < 0) || (allocSize < size) ? 0 :
allocSize;
Instead of crashing I am now returning NULL from allocate making the behavior
same as malloc.

[kbr1, 2013-04-18 20:41:31]

The new checks LGTM. Please have aedla re-review.

[aedla, 2013-04-22 16:13:54]

LGTM

[Alok Priyadarshi, 2013-04-22 18:32:39]

Committed patchset #2 manually as r2186 (presubmit successful).