Bug 227274 – Fix incorrect indexing / out of bounds access in getRestartIndices

• Status: RESOLVED
• Resolution: FIXED
• Component: ANGLE

[Kyle Piddington]

Fix incorrect indexing / out of bounds access in getRestartIndices

[Kyle Piddington]

Created attachment 432008 [details]
Patch

[Kyle Piddington]

<rdar://problem/79244789>

[EWS Watchlist]

Note that there are important steps to take when updating ANGLE. See https://trac.webkit.org/wiki/UpdatingANGLE

[Dean Jackson]

Comment on attachment 432008 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=432008&action=review

> Source/ThirdParty/ANGLE/ChangeLog:8
> +        In the case where a restart index is present in the last element of an index array, we can accidently
> +        access out of bounds. Fix this by checking boundaries, and fixing other indexing errors

Do we need a new test, or did the existing tests pick this up?

[Kyle Piddington]

(In reply to Dean Jackson from comment #4)
> Comment on attachment 432008 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=432008&action=review
> 
> > Source/ThirdParty/ANGLE/ChangeLog:8
> > +        In the case where a restart index is present in the last element of an index array, we can accidently
> > +        access out of bounds. Fix this by checking boundaries, and fixing other indexing errors
> 
> Do we need a new test, or did the existing tests pick this up?

The existing tests should hit this path, but would probably only trigger on ASAN builds.

[EWS]

Committed r279213 (239099@main): <https://commits.webkit.org/239099@main>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 432008 [details].

[Kimmo Kinnunen]

Comment on attachment 432008 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=432008&action=review

> Source/ThirdParty/ANGLE/src/libANGLE/renderer/metal/BufferMtl.mm:418
>      T *bufferData = (T*)(idxBuffer->mapReadOnly(ctx));

do you need unmap?

> Source/ThirdParty/ANGLE/src/libANGLE/renderer/metal/BufferMtl.mm:420
> +    for(int i = 0; i < numIndices; i++)

I think the algorithm creates many 1 length ranges for contents like F,F,F,F 
IndexRange is inclusive and as such cannot represent zero length ranges.

> Source/ThirdParty/ANGLE/src/libANGLE/renderer/metal/BufferMtl.mm:436
>          }

template<typename T>
static void calculateRestartRanges(ContextMtl* ctx, mtl::BufferRef idxBuffer, std::vector<IndexRange> * ranges)
{
    ranges->clear();
    T *bufferData = reinterpret_cast<T*>(idxBuffer->mapReadOnly(ctx));
    constexpr size_t indexCount = idxBuffer->size() / sizeof(T)
    constexpr size_t restartMarker = std::numeric_limits<T>::max();
    for(size_t i = 0; i < indexCount;)
    {
        if (bufferData[i] == restartMarker) {
            ++i;
            continue;
        }
        size_t restartBegin = i;
        ++i;
        while (i < indexCount && bufferData[i] != restartMarker)
        {
            ++i;
        }
        size_t restartEnd = i - 1;
        ranges->emplace_back(restartBegin, restartEnd);
    }
    idxBuffer->unmap();
}

I'd write something like that, but I'm the last person to correct anybody's off-by-ones.