[webkit-reviews] review denied: [Bug 31239] c3d test crash WebKit : [Attachment 43166] Patch

[bugzilla-daemon at webkit.org]

Simon Fraser (smfr) <simon.fraser at apple.com> has denied Chris Marrin
<cmarrin at apple.com>'s request for review:
Bug 31239: c3d test crash WebKit
https://bugs.webkit.org/show_bug.cgi?id=31239

Attachment 43166: Patch
https://bugs.webkit.org/attachment.cgi?id=43166&action=review

------- Additional Comments from Simon Fraser (smfr) <simon.fraser at apple.com>

>From a quick scan:

> Index: WebCore/html/canvas/WebGLArrayBuffer.cpp
> ===================================================================

> +    PassRefPtr<WebGLArrayBuffer> WebGLArrayBuffer::create(WebGLArrayBuffer*
other)
> +    {
> +	   RefPtr<WebGLArrayBuffer> buffer = adoptRef(new
WebGLArrayBuffer(other->byteLength()));
> +	   memcpy(buffer->data(), other->data(), other->byteLength());
> +	   return buffer;

The normal idiom is 
  return buffer.release();

> Index: WebCore/html/canvas/WebGLBuffer.cpp
> ===================================================================

>  WebGLBuffer::WebGLBuffer(WebGLRenderingContext* ctx)
> -    : CanvasObject(ctx)
> + : CanvasObject(ctx)
> + , m_elementArrayBufferByteLength(0)
> + , m_arrayBufferByteLength(0)

The old indentation was correct.

> +bool WebGLBuffer::associateBufferData(unsigned long target, int size)
> +{
> +    if (target == GraphicsContext3D::ELEMENT_ARRAY_BUFFER)
> +	   m_elementArrayBufferByteLength = size;
> +    else if (target == GraphicsContext3D::ARRAY_BUFFER)
> +	   m_arrayBufferByteLength = size;
> +    else
> +	   return false;
> +    return true;
> +}

This would be clearer with early returns.

> +
> +bool WebGLBuffer::associateBufferData(unsigned long target, WebGLArray*
array)
> +{
> +    if (!array)
> +	   return false;
> +	   
> +    if (target == GraphicsContext3D::ELEMENT_ARRAY_BUFFER) {
> +	   m_elementArrayBufferByteLength = array->sizeInBytes();
> +	   m_elementArrayBuffer = array->buffer();
> +    }
> +    else if (target == GraphicsContext3D::ARRAY_BUFFER)
> +	   m_arrayBufferByteLength = array->sizeInBytes();
> +    else
> +	   return false;
> +    return true;
> +}

This would be clearer with early returns.

> +bool WebGLBuffer::associateBufferSubData(unsigned long target, long offset,
WebGLArray* array)
> +{
> +    if (!array)
> +	   return false;
> +	   
> +    if (target == GraphicsContext3D::ELEMENT_ARRAY_BUFFER) {
> +	   if (array->sizeInBytes() + offset > m_elementArrayBufferByteLength)
> +	       return false;

What if offset is a large negative number, or MAX_LONG?

> +	   // If we already have a buffer, we need to clone it and add the new
data
> +	   if (m_elementArrayBuffer)
> +	       m_elementArrayBuffer =
WebGLArrayBuffer::create(m_elementArrayBuffer.get());
> +	       
> +	   memcpy(static_cast<unsigned char*>(m_elementArrayBuffer->data()) +
offset, array->baseAddress(), array->sizeInBytes());
> +    }
> +    else if (target == GraphicsContext3D::ARRAY_BUFFER) {
> +	   if (array->sizeInBytes() + offset > m_arrayBufferByteLength)
> +	       return false;
> +    }
> +    else
> +	   return false;
> +    return true;
> +}

This would be clearer with early returns.

> Index: WebCore/html/canvas/WebGLRenderingContext.cpp
> ===================================================================

> +bool WebGLRenderingContext::validateIndexArray(unsigned long count, unsigned
long type, long offset, long& numElements)
> +{
> +    long lastIndex = -1;
> +    if (!m_boundElementArrayBuffer)
> +	   return false;
> +    
> +    if (type == GraphicsContext3D::UNSIGNED_SHORT) {
> +	   int n =
m_boundElementArrayBuffer->byteLength(GraphicsContext3D::ELEMENT_ARRAY_BUFFER);

> +	   if ((long) count * 2 + offset > n)
> +	       return false;

You risk integer overflow here.

> +	   n /= 2;
> +	   const unsigned short* p = static_cast<const unsigned
short*>(m_boundElementArrayBuffer->elementArrayBuffer()->data());
> +	   while (n-- > 0) {
> +	       if (*p > lastIndex)
> +		   lastIndex = *p;
> +	       ++p;
> +	   }
> +    }
> +    else if (type == GraphicsContext3D::UNSIGNED_BYTE) {
> +	   int n =
m_boundElementArrayBuffer->byteLength(GraphicsContext3D::ELEMENT_ARRAY_BUFFER);

> +	   const unsigned char* p = static_cast<const unsigned
char*>(m_boundElementArrayBuffer->elementArrayBuffer()->data());
> +	   if ((long) count + offset > n)
> +	       return false;

You risk integer overflow here too.

> +    if (smallestNumElements == 1e9)
> +	   smallestNumElements = 0;

1e9? Should that be a named constant?


> -void WebGLRenderingContext::vertexAttribPointer(unsigned long indx, long
size, unsigned long type, bool normalized, unsigned long stride, unsigned long
offset)
> +void WebGLRenderingContext::vertexAttribPointer(unsigned long indx, long
size, unsigned long type, bool normalized, unsigned long stride, unsigned long
offset, ExceptionCode& ec)

indx is a pretty ugly name.

> +    if (indx >= m_vertexAttribState.size())
> +	   m_vertexAttribState.resize(indx + 1);

What if indx is MAX_UNSIGNED_LONG? (Here and elsewhere).

> Index: WebCore/html/canvas/WebGLShader.h
> ===================================================================
> --- WebCore/html/canvas/WebGLShader.h (revision 50913)
> +++ WebCore/html/canvas/WebGLShader.h (working copy)
> @@ -37,10 +37,10 @@ namespace WebCore {
>      public:
>	   virtual ~WebGLShader() { deleteObject(); }
>	   
> -	   static PassRefPtr<WebGLShader> create(WebGLRenderingContext*,
GraphicsContext3D::ShaderType);
> +	   static PassRefPtr<WebGLShader> create(WebGLRenderingContext*,
unsigned long);
>	   
>      private:
> -	   WebGLShader(WebGLRenderingContext*, GraphicsContext3D::ShaderType);
> +	   WebGLShader(WebGLRenderingContext*, unsigned long);

These changes seem like a regression in readability, and I don't see any notes
in the Changelog about why they were changed.