Used size_t for object size instead of signed int.

Used size_t for object size instead of signed int.
BUG=crbug 179653
R=aedla@chromium.org, kbr@chromium.org

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

[kbr1, 2013-04-19 18:00:48]

LGTM

[cevans, 2013-04-19 18:43:20]

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

https://codereview.appspot.com/8834048/diff/1/src/compiler/SymbolTable.cpp#ne...
src/compiler/SymbolTable.cpp:81: totalSize = size * size;
On 32-bit, size_t is just 32-bits so looks like there's still an overflow here?
And below? Seems undesirable.

[Alok Priyadarshi, 2013-04-19 19:10:28]

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

https://codereview.appspot.com/8834048/diff/1/src/compiler/SymbolTable.cpp#ne...
src/compiler/SymbolTable.cpp:81: totalSize = size * size;
On 2013/04/19 18:43:21, cevans wrote:
> On 32-bit, size_t is just 32-bits so looks like there's still an overflow
here?
> And below? Seems undesirable.

(size * size) will not overflow because it size <= 4.

https://codereview.appspot.com/8834048/diff/1/src/compiler/SymbolTable.cpp#ne...
src/compiler/SymbolTable.cpp:86: totalSize *= std::max(getArraySize(),
getMaxArraySize());
This may overflow but is there any harm if it overflows? What does sizeof
operator do?

https://codereview.appspot.com/8834048/diff/1/src/compiler/SymbolTable.cpp#ne...
src/compiler/SymbolTable.cpp:89: if (totalSize > SIZE_MAX)
oops.. did not notice this check, which is completely redundant. Will remove.

[aedla, 2013-04-22 14:44:01]

I would like to point out even on 64-bit, getObjectSize() could return anything
from 0 to 2^64-1 with enough arrays and nesting. So using a larger type in
itself doesn't help much. But going from the signed int to the unsigned size_t
actually introduces a subtle change that breaks my PoC:

    if (...
            (op == EOpConstructStruct && size < type->getObjectSize())) {
            error(line, "not enough data provided for construction",
"constructor");

This check used to succeed with getObjectSize() == -1. Now with 0xffffffff it
fails.

But even so, getObjectSize() is used in a lot of places, and in most of them it
is assigned to int. So it is probably easier to limit object sizes to INT_MAX or
less than to make sure that none of the current or future callers break with
negative size.

[Alok Priyadarshi, 2013-04-29 21:56:47]

I will check the call-sites to make sure if they really need getObjectSize(). In
many cases they actually mean getNominalSize(), which returns signed int.

I think getObjectSize should behave like sizeof operator, which returns size_t
without any clamping.

[Alok Priyadarshi, 2013-05-01 22:24:16]

The new patch also removes all implicit conversions. Do you think we still need
clamping?

[aedla, 2013-05-02 17:59:14]

A lot of things can go wrong if getObjectSize returns above INT_MAX. In fact,
here is an almost complete list of things that can go wrong:


Truncation to negative, loop is skipped:

TOutputTraverser::visitConstantUnion:
    int size = node->getType().getObjectSize();
    for (int i = 0; i < size; i++) {

TOutputTraverser::visitConstantUnion:
    int size = node->getType().getObjectSize();
    for (int i = 0; i < size; i++) {

CompareStruct:
    int size = (*fields)[j].type->getObjectSize();
    for (int i = 0; i < size; i++) {

OutputHLSL::addConstructor:
    int remainingComponents = ctorType.getObjectSize();
    while (remainingComponents > 0)

OutputHLSL::writeConstantUnion:
    int size = type.getObjectSize();
    for (int i = 0; i < size; i++, constUnion++)

TOutputGLSLBase::writeConstantUnion:
    int size = type.getObjectSize();
    for (int i = 0; i < size; ++i, ++pConstUnion)



Overflows:

TParseContext::constructorErrorCheck:
    int size = 0;
    for (size_t i = 0; i < function.getParamCount(); ++i) {
        size += param.type->getObjectSize();

TParseContext::addConstStruct:
    int instanceSize = 0;
    for ( index = 0; index < fields->size(); ++index) {
        instanceSize += (*fields)[index].type->getObjectSize();

TType::getStructSize:
    structureSize += ((*tl).type)->getObjectSize();



Large allocations fail, either by crashing the process or returning NULL, which
is later dereferenced:

TParseContext::foldConstConstructor:
    ConstantUnion* unionArray = new ConstantUnion[type.getObjectSize()];

TIntermConstantUnion::fold:
    int objectSize = getType().getObjectSize();
    rightUnionArray = new ConstantUnion[objectSize];

TIntermConstantUnion::fold:
    unionArray = new ConstantUnion[constantNode->getType().getObjectSize()];

TIntermConstantUnion::fold:
    objectSize = constantNode->getType().getObjectSize();
    tempConstArray = new ConstantUnion[objectSize];

TIntermediate::promoteConstantUnion:
    int size = node->getType().getObjectSize();
    ConstantUnion *leftUnionArray = new ConstantUnion[size];

TVariable::getConstPointer:
    unionArray = new ConstantUnion[type.getObjectSize()];



Truncation to negative, used as array index:

TParseContext::addConstArrayNode:
    int arrayElementSize = arrayElementType.getObjectSize();
    typedNode = intermediate.addConstantUnion(&unionArray[arrayElementSize *
index], tempConstantNode->getType(), line);

CompareStructure:
    int offset = typeWithoutArrayness.getObjectSize() * i;
    if (!CompareStruct(typeWithoutArrayness, &rightUnionArray[offset],
&leftUnionArray[offset]))



loop counter never reaches its limit, goes to negative instead:

TIntermConstantUnion::fold:
    for (int i = 0; i < constantNode->getType().getObjectSize(); ++i)
        unionArray[i] = *getUnionArrayPointer();

OutputHLSL::initializer:
    for (int component = 0; component < type.getObjectSize(); component++)



Most of these cases are probably not reachable with a large object size, but it
is difficult to figure out which ones are. It would be easier to limit the
object size.

To be clear, I'm not suggesting that getObjectSize should clamp the size to be
different from the real object size. I'm suggesting that there should be a
maximum size for the objects and the code parser returns an error if this limit
is exceeded.

It also makes sense to return an error, rather than failing in weird ways.
Currently the code can't handle >2GB objects anyway.

[Alok Priyadarshi, 2013-05-06 17:31:50]

Sorry I do not understand. Did you look at the new patch that removes all
implicit conversion to signed int?

How is "overflow" or "truncation to a negative value" possible if there is no
conversion to signed int?

[aedla, 2013-05-06 18:32:34]

Ah sorry, not sure why I didn't notice patch set 3. Maybe I started writing my
reply earlier, or maybe I just missed it.

I think going to size_t everywhere is the right solution, although quite a bit
of work. Thanks for taking it on. I'll try to review it soon.

[Alok Priyadarshi, 2013-05-10 16:21:35]

Any issues with this specific patch?

[aedla, 2013-05-10 16:58:39]

Overflow checks for getObjectSize and getStructSize should probably be done in
this patch as well. Let's say an attacker creates an const object of 2^64+2
ints. Then the size of the object overflows and only 2 ConstantUnions are
allocated. The attacker can read any of the 2^64 ConstantUnions, which are out
of bounds. So that's an infoleak.

[aedla, 2013-05-10 19:23:19]

One thing that bothers me is memory allocations. There are no checks for
allocations returning NULL.

On chrome it doesn't matter, because the allocator crashes the process instead
of returning NULL. But as ANGLE is a library, it could be used with different
allocators.

Maybe leave this for another time though.

[aedla, 2013-05-11 14:43:43]

So yeah, I think we should have the overflow checks, but otherwise this patch
LGTM. I'd like to take a quick look once we have the checks though. Thanks.

[Alok Priyadarshi, 2013-05-14 17:56:45]

On 2013/05/11 14:43:43, aedla wrote:
> So yeah, I think we should have the overflow checks, but otherwise this patch
> LGTM. I'd like to take a quick look once we have the checks though. Thanks.

aedla: PTAL

[aedla, 2013-05-15 12:45:16]

LGTM

Clamping might cause problems though. An attacker could create an object with
more than INT_MAX ints. So only INT_MAX ints are allocated. Accessing ints with
index >= INT_MAX leaks memory. It might not be possible to construct such a
large object though. I tried and failed. Since there is no overflow of object
size, I had to provide a large amount of data for the constructor, which hit the
limits of both memory and processing time.

But there might be ways around these limits. I think maybe in the future we
should avoid creating large types during parsing, so getObjectSize and
getStructSize can just have asserts for overflows.

[aedla, 2013-05-15 13:39:18]

Did some more thinking/experimenting. Creating a const object of size INT_MAX is
pretty much possible. But as sizeof(ConstantUnion)=8, such an object takes 16GB
of memory. In chrome this allocation OOM kills the GPU process. So there's a
DoS, but otherwise we should be good.

[Alok Priyadarshi, 2013-05-15 16:46:46]

Committed patchset #6 manually as r2211 (presubmit successful).