Bug 31239 – c3d test crash WebKit

• Status: RESOLVED
• Resolution: FIXED
• Component: WebGL

[Simon Fraser (smfr)]

When I load the page in the URL, webkit crashes. Stack trace coming...

[Simon Fraser (smfr)]

Crash is at:
Exception Type:  EXC_BAD_ACCESS (SIGSEGV)
Exception Codes: KERN_INVALID_ADDRESS at 0x000000000000000c
Crashed Thread:  0  Dispatch queue: com.apple.main-thread

Thread 0 Crashed:  Dispatch queue: com.apple.main-thread
0   com.apple.WebCore             	0x00000001009e460e WebCore::GraphicsContext3D::useProgram(WebCore::CanvasProgram*) + 46
1   com.apple.WebCore             	0x0000000100784e65 WebCore::CanvasRenderingContext3D::useProgram(WebCore::CanvasProgram*) + 21
2   com.apple.WebCore             	0x0000000100aec991 WebCore::jsCanvasRenderingContext3DPrototypeFunctionUseProgram(JSC::ExecState*, JSC::JSObject*, JSC::JSValue, JSC::ArgList const&) + 129
3   ???                           	0x00003978462001c4 0 + 63188735361476
4   com.apple.JavaScriptCore      	0x000000010056b119 JSC::Interpreter::execute(JSC::FunctionExecutable*, JSC::ExecState*, JSC::JSFunction*, JSC::JSObject*, JSC::ArgList const&, JSC::ScopeChainNode*, JSC::JSValue*) + 585
5   ???                           	0x0000000116a257c0 0 + 4674705344
6   ???                           	0x0000000120727f78 0 + 4839341944
7   com.apple.WebCore             	0x0000000100b818c0 WebCore::JSDOMWindowShell::~JSDOMWindowShell() + 0
8   ???                           	0x0000441f0f66ffff 0 + 74900193083391

[Chris Marrin]

Created attachment 43166 [details]
Patch

[Simon Fraser (smfr)]

Comment on attachment 43166 [details]
Patch


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.

[Chris Marrin]

Created attachment 43212 [details]
Patch

Updated patch addressing issues from Simon

[Chris Marrin]

Simon pointed out that there is an errant, unfinished comment in WebGLRenderingContext. I also forgot to change createShader() to include the new WebGLEnumType. These will be fixed before landing.

[Oliver Hunt]

Comment on attachment 43212 [details]
Patch

Minor formatting bug
> +bool WebGLRenderingContext::validateIndexArray(unsigned long count, unsigned long type, long offset, long& numElements)
> +{
...
> +    }
> +    else if

File a bug on createShader -- it will accept any non-FRAGMENT_SHADER value as GL_VERTEX_SHADER

r- until there are testcases for all of these as well :-(

--Oliver

[Chris Marrin]

Created attachment 43732 [details]
Replacement patch with test cases

I also added a bug as noted by Ollie (https://bugs.webkit.org/show_bug.cgi?id=31808)

[Oliver Hunt]

Comment on attachment 43732 [details]
Replacement patch with test cases

r- due to the large number of unnecessary changes (the indx->index renames)

[Chris Marrin]

Created attachment 43736 [details]
Desperate attempt at acceptance patch

One more try with spurious indx->index change reverted

[Oliver Hunt]

Comment on attachment 43736 [details]
Desperate attempt at acceptance patch

WebGLRenderingContext::validateIndexArray

has an else start on the line after }

You've removed the type checks from createShader, which is a bad thing.

r- mostly due to removing the type checks

[Oliver Hunt]

Comment on attachment 43736 [details]
Desperate attempt at acceptance patch

r=me, assuming you fix the style error, and fix bug 31808 quickly

[Chris Marrin]

Sending        LayoutTests/ChangeLog
Adding         LayoutTests/fast/canvas/webgl/drawArraysOutOfBounds-expected.txt
Adding         LayoutTests/fast/canvas/webgl/drawArraysOutOfBounds.html
Adding         LayoutTests/fast/canvas/webgl/drawElementssOutOfBounds-expected.txt
Adding         LayoutTests/fast/canvas/webgl/drawElementssOutOfBounds.html
Sending        WebCore/ChangeLog
Sending        WebCore/bindings/js/JSWebGLArrayCustom.cpp
Sending        WebCore/bindings/js/JSWebGLRenderingContextCustom.cpp
Sending        WebCore/html/canvas/WebGLArrayBuffer.cpp
Sending        WebCore/html/canvas/WebGLArrayBuffer.h
Sending        WebCore/html/canvas/WebGLBuffer.cpp
Sending        WebCore/html/canvas/WebGLBuffer.h
Sending        WebCore/html/canvas/WebGLRenderingContext.cpp
Sending        WebCore/html/canvas/WebGLRenderingContext.h
Sending        WebCore/html/canvas/WebGLRenderingContext.idl
Sending        WebCore/html/canvas/WebGLShader.cpp
Sending        WebCore/html/canvas/WebGLShader.h
Sending        WebCore/platform/graphics/GraphicsContext3D.h
Sending        WebCore/platform/graphics/mac/GraphicsContext3DMac.cpp
Transmitting file data ...................
Committed revision 51327.