Issue #8738: bufferSubData and texImage2D argument sanity checks.

components/canvas_traits/lib.rs

@@ -134,7 +134,7 @@ pub enum CanvasWebGLMsg {
     BlendFuncSeparate(u32, u32, u32, u32),
     AttachShader(u32, u32),
     BindAttribLocation(u32, u32, String),
-    BufferData(u32, Vec<f32>, u32),
+    BufferData(u32, Vec<u8>, u32),
     BufferSubData(u32, isize, Vec<u8>),
     Clear(u32),
     ClearColor(f32, f32, f32, f32),

components/script/dom/webglbuffer.rs

@@ -18,6 +18,7 @@ pub struct WebGLBuffer {
     id: u32,
     /// The target to which this buffer was bound the first time
     target: Cell<Option<u32>>,
+    capacity: Cell<usize>,
     is_deleted: Cell<bool>,
     #[ignore_heap_size_of = "Defined in ipc-channel"]
     renderer: IpcSender<CanvasMsg>,
@@ -29,6 +30,7 @@ impl WebGLBuffer {
             webgl_object: WebGLObject::new_inherited(),
             id: id,
             target: Cell::new(None),
+            capacity: Cell::new(0),
             is_deleted: Cell::new(false),
             renderer: renderer,
         }
@@ -68,6 +70,24 @@ impl WebGLBuffer {
         Ok(())
     }
 
+    pub fn buffer_data(&self, target: u32, data: &[u8], usage: u32) -> WebGLResult<()> {
+        if let Some(previous_target) = self.target.get() {
+            if target != previous_target {
+                return Err(WebGLError::InvalidOperation);
+            }
+        }
+        self.capacity.set(data.len());
+        self.renderer
+            .send(CanvasMsg::WebGL(CanvasWebGLMsg::BufferData(target, data.to_vec(), usage)))
+            .unwrap();
+
+        Ok(())
+    }
+
+    pub fn capacity(&self) -> usize {
+        self.capacity.get()
+    }
+
     pub fn delete(&self) {
         if !self.is_deleted.get() {
             self.is_deleted.set(true);

components/script/dom/webglrenderingcontext.rs

@@ -80,6 +80,8 @@ pub struct WebGLRenderingContext {
     texture_unpacking_settings: Cell<TextureUnpacking>,
     bound_texture_2d: MutNullableHeap<JS<WebGLTexture>>,
     bound_texture_cube_map: MutNullableHeap<JS<WebGLTexture>>,
+    bound_buffer_array: MutNullableHeap<JS<WebGLBuffer>>,
+    bound_buffer_element_array: MutNullableHeap<JS<WebGLBuffer>>,
 }
 
 impl WebGLRenderingContext {
@@ -106,6 +108,8 @@ impl WebGLRenderingContext {
                 texture_unpacking_settings: Cell::new(CONVERT_COLORSPACE),
                 bound_texture_2d: MutNullableHeap::new(None),
                 bound_texture_cube_map: MutNullableHeap::new(None),
+                bound_buffer_array: MutNullableHeap::new(None),
+                bound_buffer_element_array: MutNullableHeap::new(None),
             }
         })
     }
@@ -177,6 +181,20 @@ impl WebGLRenderingContext {
         }
     }
 
+    #[allow(unsafe_code)]
+    fn byte_array_to_slice(&self, data: *mut JSObject) -> Option<Vec<u8>> {
+        unsafe {
+            let mut length = 0;
+            let mut ptr = ptr::null_mut();
+            let buffer_data = JS_GetObjectAsArrayBufferView(data, &mut length, &mut ptr);
+            if buffer_data.is_null() {
+                self.webgl_error(InvalidValue); // https://github.com/servo/servo/issues/5014
+                return None;
+            }
+            Some(slice::from_raw_parts(ptr, length as usize).to_vec())
+        }
+    }
+
     fn vertex_attrib(&self, indx: u32, x: f32, y: f32, z: f32, w: f32) {
         self.ipc_renderer
             .send(CanvasMsg::WebGL(CanvasWebGLMsg::VertexAttrib(indx, x, y, z, w)))
@@ -358,15 +376,18 @@ impl WebGLRenderingContextMethods for WebGLRenderingContext {
 
     // https://www.khronos.org/registry/webgl/specs/latest/1.0/#5.14.5
     fn BindBuffer(&self, target: u32, buffer: Option<&WebGLBuffer>) {
-        match target {
+        let slot = match target {
-            constants::ARRAY_BUFFER |
+            constants::ARRAY_BUFFER => &self.bound_buffer_array,
-            constants::ELEMENT_ARRAY_BUFFER => (),
+            constants::ELEMENT_ARRAY_BUFFER => &self.bound_buffer_element_array,
 
             _ => return self.webgl_error(InvalidEnum),
-        }
+        };
 
         if let Some(buffer) = buffer {
-            handle_potential_webgl_error!(self, buffer.bind(target))
+            match buffer.bind(target) {
+                Ok(_) => slot.set(Some(buffer)),
+                Err(e) => return self.webgl_error(e),
+            }
         } else {
             // Unbind the current buffer
             self.ipc_renderer
@@ -431,11 +452,15 @@ impl WebGLRenderingContextMethods for WebGLRenderingContext {
     #[allow(unsafe_code)]
     // https://www.khronos.org/registry/webgl/specs/latest/1.0/#5.14.5
     fn BufferData(&self, _cx: *mut JSContext, target: u32, data: Option<*mut JSObject>, usage: u32) {
-        match target {
+        let bound_buffer = match target {
-            constants::ARRAY_BUFFER |
+            constants::ARRAY_BUFFER => self.bound_buffer_array.get(),
-            constants::ELEMENT_ARRAY_BUFFER => (),
+            constants::ELEMENT_ARRAY_BUFFER => self.bound_buffer_element_array.get(),
             _ => return self.webgl_error(InvalidEnum),
-        }
+        };
+        let bound_buffer = match bound_buffer {
+            Some(bound_buffer) => bound_buffer,
+            None => return self.webgl_error(InvalidValue),
+        };
         match usage {
             constants::STREAM_DRAW |
             constants::STATIC_DRAW |
@@ -446,21 +471,23 @@ impl WebGLRenderingContextMethods for WebGLRenderingContext {
             Some(data) => data,
             None => return self.webgl_error(InvalidValue),
         };
-        if let Some(data_vec) = self.float32_array_to_slice(data) {
+        if let Some(data_vec) = self.byte_array_to_slice(data) {
-            self.ipc_renderer
+            handle_potential_webgl_error!(self, bound_buffer.buffer_data(target, &data_vec, usage));
-                .send(CanvasMsg::WebGL(CanvasWebGLMsg::BufferData(target, data_vec, usage)))
-                .unwrap()
         }
     }
 
     #[allow(unsafe_code)]
     // https://www.khronos.org/registry/webgl/specs/latest/1.0/#5.14.5
     fn BufferSubData(&self, _cx: *mut JSContext, target: u32, offset: i64, data: Option<*mut JSObject>) {
-        match target {
+        let bound_buffer = match target {
-            constants::ARRAY_BUFFER |
+            constants::ARRAY_BUFFER => self.bound_buffer_array.get(),
-            constants::ELEMENT_ARRAY_BUFFER => (),
+            constants::ELEMENT_ARRAY_BUFFER => self.bound_buffer_element_array.get(),
             _ => return self.webgl_error(InvalidEnum),
-        }
+        };
+        let bound_buffer = match bound_buffer {
+            Some(bound_buffer) => bound_buffer,
+            None => return self.webgl_error(InvalidOperation),
+        };
         let data = match data {
             Some(data) => data,
             None => return self.webgl_error(InvalidValue),
@@ -468,20 +495,14 @@ impl WebGLRenderingContextMethods for WebGLRenderingContext {
         if offset < 0 {
             return self.webgl_error(InvalidValue);
         }
-        let data_vec = unsafe {
+        if let Some(data_vec) = self.byte_array_to_slice(data) {
-            let mut length = 0;
+            if (offset as usize) + data_vec.len() > bound_buffer.capacity() {
-            let mut ptr = ptr::null_mut();
+                return self.webgl_error(InvalidValue);
-            let buffer_data = JS_GetObjectAsArrayBufferView(data, &mut length, &mut ptr);
-            if buffer_data.is_null() {
-                return self.webgl_error(InvalidValue) // https://github.com/servo/servo/issues/5014
             }
-            slice::from_raw_parts(ptr, length as usize).to_vec()
+            self.ipc_renderer
-        };
+                .send(CanvasMsg::WebGL(CanvasWebGLMsg::BufferSubData(target, offset as isize, data_vec)))
-        // FIXME(simartin) Check that the defined region is inside the allocated one
+                .unwrap()
-        // https://github.com/servo/servo/issues/8738
+        }
-        self.ipc_renderer
-            .send(CanvasMsg::WebGL(CanvasWebGLMsg::BufferSubData(target, offset as isize, data_vec)))
-            .unwrap()
     }
 
     // https://www.khronos.org/registry/webgl/specs/latest/1.0/#5.14.8
@@ -1061,13 +1082,15 @@ impl WebGLRenderingContextMethods for WebGLRenderingContext {
                   format: u32,
                   data_type: u32,
                   source: Option<ImageDataOrHTMLImageElementOrHTMLCanvasElementOrHTMLVideoElement >) {
-        // TODO(ecoal95): Check for bound WebGLTexture, and validate more parameters
+        let texture = match target {
-        match target {
+            constants::TEXTURE_2D => self.bound_texture_2d.get(),
-            constants::TEXTURE_2D |
+            constants::TEXTURE_CUBE_MAP => self.bound_texture_cube_map.get(),
-            constants::TEXTURE_CUBE_MAP => (),
-
             _ => return self.webgl_error(InvalidEnum),
+        };
+        if texture.is_none() {
+            return self.webgl_error(InvalidOperation);
         }
+        // TODO(ecoal95): Validate more parameters
 
         let source = match source {
             Some(s) => s,

tests/wpt/mozilla/meta/MANIFEST.json

@@ -5853,6 +5853,12 @@
             "url": "/_mozilla/mozilla/webgl/bufferData.html"
           }
         ],
+        "mozilla/webgl/bufferSubData.html": [
+          {
+            "path": "mozilla/webgl/bufferSubData.html",
+            "url": "/_mozilla/mozilla/webgl/bufferSubData.html"
+          }
+        ],
         "mozilla/webgl/context_creation_error.html": [
           {
             "path": "mozilla/webgl/context_creation_error.html",

tests/wpt/mozilla/tests/mozilla/webgl/bufferData.html

@@ -1,6 +1,6 @@
 <!doctype html>
 <meta charset="utf-8">
-<title>bufferData and bufferSubData input array type check (issue #6791, #6601)</title>
+<title>bufferData input array type check (issue #6791, #6601)</title>
 <script src="/resources/testharness.js"></script>
 <script src="/resources/testharnessreport.js"></script>
 <script>
@@ -11,31 +11,24 @@ test(function() {
 
     var coordI8 = new Int8Array([ 1, 0 ]);
     gl.bufferData(gl.ARRAY_BUFFER, coordI8, gl.STATIC_DRAW);
-    gl.bufferSubData(gl.ARRAY_BUFFER, 1, coordI8);
 
     var coordU8 = new Uint8Array([ 1, 0 ]);
     gl.bufferData(gl.ARRAY_BUFFER, coordU8, gl.STATIC_DRAW);
-    gl.bufferSubData(gl.ARRAY_BUFFER, 1, coordU8);
 
     var coordI16 = new Int16Array([ 1, 0 ]);
     gl.bufferData(gl.ARRAY_BUFFER, coordI16, gl.STATIC_DRAW);
-    gl.bufferSubData(gl.ARRAY_BUFFER, 1, coordI16);
 
     var coordU16 = new Uint16Array([ 1, 0 ]);
     gl.bufferData(gl.ARRAY_BUFFER, coordU16, gl.STATIC_DRAW);
-    gl.bufferSubData(gl.ARRAY_BUFFER, 1, coordU16);
 
     var coordI32 = new Int32Array([ 1, 0 ]);
     gl.bufferData(gl.ARRAY_BUFFER, coordI32, gl.STATIC_DRAW);
-    gl.bufferSubData(gl.ARRAY_BUFFER, 1, coordI32);
 
     var coordU32 = new Uint32Array([ 1, 0 ]);
     gl.bufferData(gl.ARRAY_BUFFER, coordU32, gl.STATIC_DRAW);
-    gl.bufferSubData(gl.ARRAY_BUFFER, 1, coordU32);
 
     var coordF32 = new Float32Array([ 1.0, 0.0 ]);
     gl.bufferData(gl.ARRAY_BUFFER, coordF32, gl.STATIC_DRAW);
-    gl.bufferSubData(gl.ARRAY_BUFFER, 1, coordF32);
 
     var coordF64 = new Float64Array([ 1.0, 0.0 ]);
     gl.bufferData(gl.ARRAY_BUFFER, coordF64, gl.STATIC_DRAW);

tests/wpt/mozilla/tests/mozilla/webgl/bufferSubData.html

@@ -0,0 +1,79 @@
+<!doctype html>
+<meta charset="utf-8">
+<title>bufferSubData input array type check (issue #6791, #8738)</title>
+<script src="/resources/testharness.js"></script>
+<script src="/resources/testharnessreport.js"></script>
+<script>
+test(function() {
+    var gl = document.createElement("canvas").getContext("webgl");
+    var texture_buffer = gl.createBuffer();
+    gl.bindBuffer(gl.ARRAY_BUFFER, texture_buffer);
+
+
+    //
+    // Issue #6791
+    //
+
+    // Bind to a 20 Int8 buffer
+    var longBuffer = new Int8Array([ 1, 0, 1, 0, 1, 0, 1, 0, 1, 0,
+                                     1, 0, 1, 0, 1, 0, 1, 0, 1, 0 ]);
+    gl.bufferData(gl.ARRAY_BUFFER, longBuffer, gl.STATIC_DRAW);
+
+    var coordI8 = new Int8Array([ 1, 0 ]);
+    gl.bufferSubData(gl.ARRAY_BUFFER, 1, coordI8);
+
+    var coordU8 = new Uint8Array([ 1, 0 ]);
+    gl.bufferSubData(gl.ARRAY_BUFFER, 1, coordU8);
+
+    var coordI16 = new Int16Array([ 1, 0 ]);
+    gl.bufferSubData(gl.ARRAY_BUFFER, 1, coordI16);
+
+    var coordU16 = new Uint16Array([ 1, 0 ]);
+    gl.bufferSubData(gl.ARRAY_BUFFER, 1, coordU16);
+
+    var coordI32 = new Int32Array([ 1, 0 ]);
+    gl.bufferSubData(gl.ARRAY_BUFFER, 1, coordI32);
+
+    var coordU32 = new Uint32Array([ 1, 0 ]);
+    gl.bufferSubData(gl.ARRAY_BUFFER, 1, coordU32);
+
+    var coordF32 = new Float32Array([ 1.0, 0.0 ]);
+    gl.bufferSubData(gl.ARRAY_BUFFER, 1, coordF32);
+
+    var coordF64 = new Float64Array([ 1.0, 0.0 ]);
+    gl.bufferSubData(gl.ARRAY_BUFFER, 1, coordF64);
+
+
+    //
+    // Issue #8738
+    //
+
+    // Re-bind to a 10 Int8 buffer (so 10 bytes long)
+    var coordArrayI8_10 = new Int8Array([ 1, 0, 1, 0, 1, 0, 1, 0, 1, 0 ]);
+    gl.bufferData(gl.ARRAY_BUFFER, coordArrayI8_10, gl.STATIC_DRAW);
+
+    //   - Setting two Int8 elements starting at offset 8 is OK
+    gl.bufferSubData(gl.ARRAY_BUFFER, 8, coordI8);
+    assert_equals(gl.NO_ERROR, gl.getError());
+
+    //   - Setting two Int8 starting at offset 9 is an error
+    gl.bufferSubData(gl.ARRAY_BUFFER, 9, coordI8);
+    assert_equals(gl.INVALID_VALUE, gl.getError());
+
+    // Re-bind to a 2 Uint16 buffer (so 4 bytes long)
+    var coordArrayU16_10 = new Uint16Array([ 1, 0 ]);
+    gl.bufferData(gl.ARRAY_BUFFER, coordArrayU16_10, gl.STATIC_DRAW);
+
+    //   - Setting two Uint16 elements starting at offset 0 is OK
+    gl.bufferSubData(gl.ARRAY_BUFFER, 0, coordU16);
+    assert_equals(gl.NO_ERROR, gl.getError());
+
+    //  - Setting two Int8 starting at offset 1 is OK
+    gl.bufferSubData(gl.ARRAY_BUFFER, 1, coordI8);
+    assert_equals(gl.NO_ERROR, gl.getError());
+
+    //  - Setting two Uint16 starting at offset 1 is an error
+    gl.bufferSubData(gl.ARRAY_BUFFER, 1, coordU16);
+    assert_equals(gl.INVALID_VALUE, gl.getError());
+});
+</script>