Use a helper function for checked arithmetic in WebGL validation
<rdar://problem/36485879>
Created attachment 331250 [details] Patch
Committed r226927: <https://trac.webkit.org/changeset/226927>
Re-opened since this is blocked by bug 181621
Committed r226953: <https://trac.webkit.org/changeset/226953>
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.