webgl: Fix out-of-bounds readpixels handling.

This fixes the crash in read-pixels-pack-alignment (which was trying
to read out of bounds).

Fixes #13901

5 files changed, 122 insertions, 21 deletions

diff --git a/components/script/dom/webglframebuffer.rs b/components/script/dom/webglframebuffer.rs
index 8586b09d5bf..ef40fecac19 100644
--- a/components/script/dom/webglframebuffer.rs
+++ b/components/script/dom/webglframebuffer.rs
@@ -33,6 +33,7 @@ pub struct WebGLFramebuffer {
     /// target can only be gl::FRAMEBUFFER at the moment
     target: Cell<Option<u32>>,
     is_deleted: Cell<bool>,
+    size: Cell<Option<(i32, i32)>>,
     status: Cell<u32>,
     #[ignore_heap_size_of = "Defined in ipc-channel"]
     renderer: IpcSender<CanvasMsg>,
@@ -55,6 +56,7 @@ impl WebGLFramebuffer {
             target: Cell::new(None),
             is_deleted: Cell::new(false),
             renderer: renderer,
+            size: Cell::new(None),
             status: Cell::new(constants::FRAMEBUFFER_UNSUPPORTED),
             color: DOMRefCell::new(None),
             depth: DOMRefCell::new(None),
@@ -110,6 +112,10 @@ impl WebGLFramebuffer {
         self.is_deleted.get()
     }
 
+    pub fn size(&self) -> Option<(i32, i32)> {
+        self.size.get()
+    }
+
     fn update_status(&self) {
         let c = self.color.borrow();
         let z = self.depth.borrow();
@@ -165,6 +171,7 @@ impl WebGLFramebuffer {
                 }
             }
         }
+        self.size.set(fb_size);
 
         if has_c || has_z || has_zs || has_s {
             self.status.set(constants::FRAMEBUFFER_COMPLETE);
diff --git a/components/script/dom/webglrenderingcontext.rs b/components/script/dom/webglrenderingcontext.rs
index 781b95c60bf..c5af5f4c39d 100644
--- a/components/script/dom/webglrenderingcontext.rs
+++ b/components/script/dom/webglrenderingcontext.rs
@@ -283,6 +283,16 @@ impl WebGLRenderingContext {
             .unwrap();
     }
 
+    fn get_current_framebuffer_size(&self) -> Option<(i32, i32)> {
+        match self.bound_framebuffer.get() {
+            Some(fb) => return fb.size(),
+
+            // The window system framebuffer is bound
+            None => return Some((self.DrawingBufferWidth(),
+                                 self.DrawingBufferHeight())),
+        }
+    }
+
     fn validate_stencil_actions(&self, action: u32) -> bool {
         match action {
             0 | constants::KEEP | constants::REPLACE | constants::INCR | constants::DECR |
@@ -649,6 +659,26 @@ impl WebGLRenderingContextMethods for WebGLRenderingContext {
                 return object_binding_to_js_or_null!(cx, &self.bound_texture_2d),
             constants::TEXTURE_BINDING_CUBE_MAP =>
                 return object_binding_to_js_or_null!(cx, &self.bound_texture_cube_map),
+
+            // In readPixels we currently support RGBA/UBYTE only.  If
+            // we wanted to support other formats, we could ask the
+            // driver, but we would need to check for
+            // GL_OES_read_format support (assuming an underlying GLES
+            // driver. Desktop is happy to format convert for us).
+            constants::IMPLEMENTATION_COLOR_READ_FORMAT => {
+                if !self.validate_framebuffer_complete() {
+                    return NullValue();
+                } else {
+                    return Int32Value(constants::RGBA as i32);
+                }
+            }
+            constants::IMPLEMENTATION_COLOR_READ_TYPE => {
+                if !self.validate_framebuffer_complete() {
+                    return NullValue();
+                } else {
+                    return Int32Value(constants::UNSIGNED_BYTE as i32);
+                }
+            }
             _ => {}
         }
 
@@ -1777,10 +1807,80 @@ impl WebGLRenderingContextMethods for WebGLRenderingContext {
             _ => return Ok(self.webgl_error(InvalidOperation)),
         }
 
+        // From the WebGL specification, 5.14.12 Reading back pixels
+        //
+        //     "Only two combinations of format and type are
+        //      accepted. The first is format RGBA and type
+        //      UNSIGNED_BYTE. The second is an implementation-chosen
+        //      format. The values of format and type for this format
+        //      may be determined by calling getParameter with the
+        //      symbolic constants IMPLEMENTATION_COLOR_READ_FORMAT
+        //      and IMPLEMENTATION_COLOR_READ_TYPE, respectively. The
+        //      implementation-chosen format may vary depending on the
+        //      format of the currently bound rendering
+        //      surface. Unsupported combinations of format and type
+        //      will generate an INVALID_OPERATION error."
+        //
+        // To avoid having to support general format packing math, we
+        // always report RGBA/UNSIGNED_BYTE as our only supported
+        // format.
+        if format != constants::RGBA || pixel_type != constants::UNSIGNED_BYTE {
+            return Ok(self.webgl_error(InvalidOperation));
+        }
+        let cpp = 4;
+
+        //     "If pixels is non-null, but is not large enough to
+        //      retrieve all of the pixels in the specified rectangle
+        //      taking into account pixel store modes, an
+        //      INVALID_OPERATION error is generated."
+        let stride = match width.checked_mul(cpp) {
+            Some(stride) => stride,
+            _ => return Ok(self.webgl_error(InvalidOperation)),
+        };
+
+        match height.checked_mul(stride) {
+            Some(size) if size <= data.len() as i32 => {}
+            _ => return Ok(self.webgl_error(InvalidOperation)),
+        }
+
+        //     "For any pixel lying outside the frame buffer, the
+        //      corresponding destination buffer range remains
+        //      untouched; see Reading Pixels Outside the
+        //      Framebuffer."
+        let mut x = x;
+        let mut y = y;
+        let mut width = width;
+        let mut height = height;
+        let mut dst_offset = 0;
+
+        if x < 0 {
+            dst_offset += cpp * -x;
+            width += x;
+            x = 0;
+        }
+
+        if y < 0 {
+            dst_offset += stride * -y;
+            height += y;
+            y = 0;
+        }
+
         if width < 0 || height < 0 {
             return Ok(self.webgl_error(InvalidValue));
         }
 
+        match self.get_current_framebuffer_size() {
+            Some((fb_width, fb_height)) => {
+                if x + width > fb_width {
+                    width = fb_width - x;
+                }
+                if y + height > fb_height {
+                    height = fb_height - y;
+                }
+            }
+            _ => return Ok(self.webgl_error(InvalidOperation)),
+        };
+
         let (sender, receiver) = ipc::channel().unwrap();
         self.ipc_renderer
             .send(CanvasMsg::WebGL(WebGLCommand::ReadPixels(x, y, width, height, format, pixel_type, sender)))
@@ -1788,12 +1888,11 @@ impl WebGLRenderingContextMethods for WebGLRenderingContext {
 
         let result = receiver.recv().unwrap();
 
-        if result.len() > data.len() {
-            return Ok(self.webgl_error(InvalidOperation));
-        }
-
-        for i in 0..result.len() {
-            data[i] = result[i]
+        for i in 0..height {
+            for j in 0..(width * cpp) {
+                data[(dst_offset + i * stride + j) as usize] =
+                    result[(i * width * cpp + j) as usize];
+            }
         }
 
         Ok(())
diff --git a/tests/wpt/metadata/webgl/conformance-1.0.3/conformance/misc/object-deletion-behaviour.html.ini b/tests/wpt/metadata/webgl/conformance-1.0.3/conformance/misc/object-deletion-behaviour.html.ini
index 8e2be3e1863..8cb6c72e03d 100644
--- a/tests/wpt/metadata/webgl/conformance-1.0.3/conformance/misc/object-deletion-behaviour.html.ini
+++ b/tests/wpt/metadata/webgl/conformance-1.0.3/conformance/misc/object-deletion-behaviour.html.ini
@@ -174,9 +174,6 @@
   [WebGL test #171: gl.getFramebufferAttachmentParameter(gl.FRAMEBUFFER, gl.COLOR_ATTACHMENT0, gl.FRAMEBUFFER_ATTACHMENT_OBJECT_TYPE) threw exception TypeError: gl.getFramebufferAttachmentParameter is not a function]
     expected: FAIL
 
-  [WebGL test #187: gl.checkFramebufferStatus(gl.FRAMEBUFFER) should not be 36053.]
-    expected: FAIL
-
   [WebGL test #196: gl.getFramebufferAttachmentParameter(gl.FRAMEBUFFER, gl.COLOR_ATTACHMENT0, gl.FRAMEBUFFER_ATTACHMENT_OBJECT_NAME) should be [object WebGLTexture\]. Threw exception TypeError: gl.getFramebufferAttachmentParameter is not a function]
     expected: FAIL
 
diff --git a/tests/wpt/metadata/webgl/conformance-1.0.3/conformance/more/functions/readPixelsBadArgs.html.ini b/tests/wpt/metadata/webgl/conformance-1.0.3/conformance/more/functions/readPixelsBadArgs.html.ini
deleted file mode 100644
index e85893ee2a5..00000000000
--- a/tests/wpt/metadata/webgl/conformance-1.0.3/conformance/more/functions/readPixelsBadArgs.html.ini
+++ /dev/null
@@ -1,8 +0,0 @@
-[readPixelsBadArgs.html]
-  type: testharness
-  expected:
-    if os == "linux": CRASH
-    if os == "mac": TIMEOUT
-  [WebGL test #0: testReadPixels]
-    expected: FAIL
-
diff --git a/tests/wpt/metadata/webgl/conformance-1.0.3/conformance/reading/read-pixels-pack-alignment.html.ini b/tests/wpt/metadata/webgl/conformance-1.0.3/conformance/reading/read-pixels-pack-alignment.html.ini
index 3704c6c4f1b..d5b3c59a27d 100644
--- a/tests/wpt/metadata/webgl/conformance-1.0.3/conformance/reading/read-pixels-pack-alignment.html.ini
+++ b/tests/wpt/metadata/webgl/conformance-1.0.3/conformance/reading/read-pixels-pack-alignment.html.ini
@@ -1,8 +1,14 @@
 [read-pixels-pack-alignment.html]
   type: testharness
-  expected:
-    if os == "linux": CRASH
-    if os == "mac": TIMEOUT
-  [WebGL test #3: successfullyParsed should be true (of type boolean). Was undefined (of type undefined).]
+  [WebGL test #17: pixel should be 255,102,0,255. Was 0,0,0,0.]
+    expected: FAIL
+
+  [WebGL test #33: pixel should be 255,102,0,255. Was 0,0,0,0.]
+    expected: FAIL
+
+  [WebGL test #53: pixel should be 255,102,0,255. Was 0,0,0,0.]
+    expected: FAIL
+
+  [WebGL test #61: pixel should be 255,102,0,255. Was 0,0,0,0.]
     expected: FAIL