Bug 227274 - Fix incorrect indexing / out of bounds access in getRestartIndices
Summary: Fix incorrect indexing / out of bounds access in getRestartIndices
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: ANGLE (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Kyle Piddington
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2021-06-22 17:13 PDT by Kyle Piddington
Modified: 2021-06-24 03:13 PDT (History)
6 users (show)

See Also:


Attachments
Patch (2.47 KB, patch)
2021-06-22 17:14 PDT, Kyle Piddington
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Kyle Piddington 2021-06-22 17:13:13 PDT
Fix incorrect indexing / out of bounds access in getRestartIndices
Comment 1 Kyle Piddington 2021-06-22 17:14:49 PDT
Created attachment 432008 [details]
Patch
Comment 2 Kyle Piddington 2021-06-22 17:14:51 PDT
<rdar://problem/79244789>
Comment 3 EWS Watchlist 2021-06-22 17:16:01 PDT
Note that there are important steps to take when updating ANGLE. See https://trac.webkit.org/wiki/UpdatingANGLE
Comment 4 Dean Jackson 2021-06-22 17:29:26 PDT
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?
Comment 5 Kyle Piddington 2021-06-23 12:14:13 PDT
(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.
Comment 6 EWS 2021-06-23 23:53:02 PDT
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].
Comment 7 Kimmo Kinnunen 2021-06-24 03:13:14 PDT
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.