Bug 181620 – Use a helper function for checked arithmetic in WebGL validation

• Component: New Bugs
• Status: RESOLVED FIXED

[Dean Jackson]

Use a helper function for checked arithmetic in WebGL validation

[Radar WebKit Bug Importer]

<rdar://problem/36485879>

[Dean Jackson]

Created attachment 331250 [details]
Patch

[Dean Jackson]

Committed r226927: <https://trac.webkit.org/changeset/226927>

[WebKit Commit Bot]

Re-opened since this is blocked by bug 181621

[Dean Jackson]

Committed r226953: <https://trac.webkit.org/changeset/226953>

[Darin Adler]

Comment on attachment 331250 [details]
Patch

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

> Source/WebCore/html/canvas/WebGL2RenderingContext.cpp:1830
> -    Checked<unsigned, RecordOverflow> checkedNumElementsRequired = Checked<unsigned>(maxIndex.value());
> -    checkedNumElementsRequired += 1;
> -    if (checkedNumElementsRequired.hasOverflowed())
> +    auto checkedNumElementsRequired = checkedAddAndMultiply<unsigned>(maxIndex.value(), 1, 1);
> +    if (!checkedNumElementsRequired)
>          return false;
> -    numElementsRequired = checkedNumElementsRequired.unsafeGet();
> +    numElementsRequired = checkedNumElementsRequired.value();
> +
>      return true;

I am not sure the new helper function is so great here since we don’t have any multiplying to do. Idiomatic use of Checked would be simple. Like this:

    auto checkedNumElementsRequired = Checked<unsigned, RecordOverflow>(maxIndex.value()) + 1;
    if (checkedNumElementsRequired.hasOverflowed())
        return false;
    numElementsRequired = checkedNumElementsRequired.unsafeGet();
    return true;

I don’t think the checkedAddAndMultiply version is better.

> Source/WebCore/html/canvas/WebGLRenderingContext.cpp:727
> -    // The number of required elements is one more than the maximum
> -    // index that will be accessed.
> -    Checked<unsigned, RecordOverflow> checkedNumElementsRequired = Checked<unsigned>(maxIndex.value());
> -    checkedNumElementsRequired += 1;
> -    if (checkedNumElementsRequired.hasOverflowed())
> +    // The number of required elements is one more than the maximum index that will be accessed.
> +    auto checkedNumElementsRequired = checkedAddAndMultiply<unsigned>(maxIndex.value(), 1, 1);
> +    if (!checkedNumElementsRequired)
>          return false;
> -    numElementsRequired = checkedNumElementsRequired.unsafeGet();
> +    numElementsRequired = checkedNumElementsRequired.value();
> +
>      return true;

Ditto.

> Source/WebCore/html/canvas/WebGLRenderingContextBase.cpp:1851
> +    Checked<T, RecordOverflow> checkedResult = Checked<T>(value);
> +    checkedResult += Checked<T>(add);
> +    checkedResult *= Checked<T>(multiply);

If we decide to keep this:

We don’t need so many typecasts here. Once something is Checked we can do arithmetic and everything stays checked. Also Checked<T> is not a good type to mix with Checked<T, RecordOverflow>. Here’s how I would write this:

    auto checkedResult = (Checked<GC3Dint, RecordOverflow>(value) + add) * multiply;

The typecasts are not needed. When a value is Checked<>, you don’t need to convert the other values to Checked<> before doing math with them. Everything stays checked as long as one of the two arguments to each operator is checked and the right type.

> Source/WebCore/html/canvas/WebGLRenderingContextBase.cpp:1911
> -    Checked<unsigned, RecordOverflow> checkedNumElementsRequired = Checked<unsigned>(lastIndex);
> -    checkedNumElementsRequired += 1;
> -    if (checkedNumElementsRequired.hasOverflowed())
> +    auto checkedNumElementsRequired = checkedAddAndMultiply<unsigned>(lastIndex, 1, 1);
> +    if (!checkedNumElementsRequired)
>          return false;
> -    numElementsRequired = checkedNumElementsRequired.unsafeGet();
> +    numElementsRequired = checkedNumElementsRequired.value();
>      return true;

Another case where the multiplying doesn’t really come in handy. So same comment as the first two call sites.

> Source/WebCore/html/canvas/WebGLRenderingContextBase.cpp:2024
> -    Checked<GC3Dint, RecordOverflow> checkedFirst(first);
> -    Checked<GC3Dint, RecordOverflow> checkedCount(count);
> -    Checked<GC3Dint, RecordOverflow> checkedSum = checkedFirst + checkedCount;
> +    Checked<GC3Dint, RecordOverflow> checkedSum = Checked<GC3Dint, RecordOverflow>(first) + Checked<GC3Dint, RecordOverflow>(count);

Don’t need so many typecasts here. This is a good way to write it:

    auto checkedSum = Checked<GC3Dint, RecordOverflow>(first) + count;

> Source/WebCore/html/canvas/WebGLRenderingContextBase.cpp:5732
> -    Checked<GC3Duint, RecordOverflow> bufferSize(numVertex);
> -    bufferSize += 1;
> -    bufferSize *= Checked<GC3Duint>(4);
> -    if (bufferSize.hasOverflowed())
> +    auto bufferSize = checkedAddAndMultiply<GC3Duint>(numVertex, 1, 4);
> +    if (!bufferSize)
>          return false;
> -    Checked<GC3Dsizeiptr, RecordOverflow> bufferDataSize(bufferSize);
> +
> +    Checked<GC3Dsizeiptr, RecordOverflow> bufferDataSize(bufferSize.value());
>      bufferDataSize *= Checked<GC3Dsizeiptr>(sizeof(GC3Dfloat));
>      return !bufferDataSize.hasOverflowed() && bufferDataSize.unsafeGet() > 0;

I think this can be written more simply without the helper function:

    auto bufferSize = (Checked<GC3Duint, RecordOverflow>(numVertex) + 1) * 4;
    auto bufferDataSize = Checked<GC3Dsizeiptr, RecordOverflow>(bufferSize) * sizeof(GC3Dfloat);
    return !bufferDataSize.hasOverflowed() && bufferDataSize.unsafeGet() > 0;

If bufferSize overflowed, then bufferDataSize will be set to overflowed also, so no extra code is needed to explicitly check that.

> Source/WebCore/html/canvas/WebGLRenderingContextBase.h:832
> +    template <typename T> static std::optional<T> checkedAddAndMultiply(T value, T add, T multiply);

Not sure that std::optional<T> is a better return type than Checked<T, RecordOverflow>. The only difference for callers is that we need to check overflowed() instead of checking has_value() and use unsafeGet() instead of value(). Doesn’t seem much different.

I don’t think this function made things better in any of the four places we used it. I think the real problem is that we are writing Checked<> code in an excessively complex way. All we need to do is cast one value to the appropriate Checked<> type and not specify explicit types for things after that, and the code ends up being pretty readable.