Auto merge of #14081 - anholt:webgl-readpix-negative-width, r=emilio

webgl: out-of-bounds readPixels() fixes

<!-- Please describe your changes on the following line: -->
Fix crashes in two WebGL readPixels() tests by adding framebuffer size validation.

---
<!-- Thank you for contributing to Servo! Please replace each `[ ]` by `[X]` when the step is complete, and replace `__` with appropriate data: -->
- [X] `./mach build -d` does not report any errors
- [X] `./mach test-tidy` does not report any errors
- [X] These changes fix #13901

<!-- Either: -->
- [X] There are tests for these changes OR
- [ ] These changes do not require tests because _____

<!-- Pull requests that do not address these steps are welcome, but they will require additional verification as part of the review process. -->

<!-- Reviewable:start -->
---
This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/14081)
<!-- Reviewable:end -->

components/script/dom/webglframebuffer.rs

@@ -21,7 +21,7 @@ use webrender_traits::{WebGLCommand, WebGLFramebufferBindingRequest, WebGLFrameb
 #[derive(JSTraceable, Clone, HeapSizeOf)]
 enum WebGLFramebufferAttachment {
     Renderbuffer(JS<WebGLRenderbuffer>),
-    Texture(JS<WebGLTexture>),
+    Texture { texture: JS<WebGLTexture>, level: i32 },
 }
 
 impl HeapGCValue for WebGLFramebufferAttachment {}
@@ -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,11 +112,20 @@ impl WebGLFramebuffer {
         self.is_deleted.get()
     }
 
+    pub fn size(&self) -> Option<(i32, i32)> {
+        self.size.get()
+    }
+
     fn update_status(&self) {
-        let has_c = self.color.borrow().is_some();
+        let c = self.color.borrow();
-        let has_z = self.depth.borrow().is_some();
+        let z = self.depth.borrow();
-        let has_s = self.stencil.borrow().is_some();
+        let s = self.stencil.borrow();
-        let has_zs = self.depthstencil.borrow().is_some();
+        let zs = self.depthstencil.borrow();
+        let has_c = c.is_some();
+        let has_z = z.is_some();
+        let has_s = s.is_some();
+        let has_zs = zs.is_some();
+        let attachments = [&*c, &*z, &*s, &*zs];
 
         // From the WebGL spec, 6.6 ("Framebuffer Object Attachments"):
         //
@@ -135,6 +146,33 @@ impl WebGLFramebuffer {
             return;
         }
 
+        let mut fb_size = None;
+        for attachment in &attachments {
+            // Get the size of this attachment.
+            let size = match **attachment {
+                Some(WebGLFramebufferAttachment::Renderbuffer(ref att_rb)) => {
+                    att_rb.size()
+                }
+                Some(WebGLFramebufferAttachment::Texture { texture: ref att_tex, level } ) => {
+                    let info = att_tex.image_info_at_face(0, level as u32);
+                    Some((info.width() as i32, info.height() as i32))
+                }
+                None => None,
+            };
+
+            // Make sure that, if we've found any other attachment,
+            // that the size matches.
+            if size.is_some() {
+                if fb_size.is_some() && size != fb_size {
+                    self.status.set(constants::FRAMEBUFFER_INCOMPLETE_DIMENSIONS);
+                    return;
+                } else {
+                    fb_size = size;
+                }
+            }
+        }
+        self.size.set(fb_size);
+
         if has_c || has_z || has_zs || has_s {
             self.status.set(constants::FRAMEBUFFER_COMPLETE);
         } else {
@@ -190,8 +228,6 @@ impl WebGLFramebuffer {
             // Note, from the GLES 2.0.25 spec, page 113:
             //      "If texture is zero, then textarget and level are ignored."
             Some(texture) => {
-                *binding.borrow_mut() = Some(WebGLFramebufferAttachment::Texture(JS::from_ref(texture)));
-
                 // From the GLES 2.0.25 spec, page 113:
                 //
                 //     "level specifies the mipmap level of the texture image
@@ -231,12 +267,16 @@ impl WebGLFramebuffer {
                     _ => return Err(WebGLError::InvalidOperation),
                 }
 
+                *binding.borrow_mut() = Some(WebGLFramebufferAttachment::Texture {
+                    texture: JS::from_ref(texture),
+                    level: level }
+                );
+
                 Some(texture.id())
             }
 
             _ => {
                 *binding.borrow_mut() = None;
-                self.update_status();
                 None
             }
         };
@@ -251,7 +291,9 @@ impl WebGLFramebuffer {
         Ok(())
     }
 
-    pub fn detach_renderbuffer(&self, rb: &WebGLRenderbuffer) {
+    fn with_matching_renderbuffers<F>(&self, rb: &WebGLRenderbuffer, mut closure: F)
+        where F: FnMut(&DOMRefCell<Option<WebGLFramebufferAttachment>>)
+    {
         let attachments = [&self.color,
                            &self.depth,
                            &self.stencil,
@@ -267,13 +309,14 @@ impl WebGLFramebuffer {
             };
 
             if matched {
-                *attachment.borrow_mut() = None;
+                closure(attachment);
-                self.update_status();
             }
         }
     }
 
-    pub fn detach_texture(&self, texture: &WebGLTexture) {
+    fn with_matching_textures<F>(&self, texture: &WebGLTexture, mut closure: F)
+        where F: FnMut(&DOMRefCell<Option<WebGLFramebufferAttachment>>)
+    {
         let attachments = [&self.color,
                            &self.depth,
                            &self.stencil,
@@ -282,18 +325,44 @@ impl WebGLFramebuffer {
         for attachment in &attachments {
             let matched = {
                 match *attachment.borrow() {
-                    Some(WebGLFramebufferAttachment::Texture(ref att_texture))
+                    Some(WebGLFramebufferAttachment::Texture { texture: ref att_texture, .. })
                         if texture.id() == att_texture.id() => true,
                     _ => false,
                 }
             };
 
             if matched {
-                *attachment.borrow_mut() = None;
+                closure(attachment);
             }
         }
     }
 
+    pub fn detach_renderbuffer(&self, rb: &WebGLRenderbuffer) {
+        self.with_matching_renderbuffers(rb, |att| {
+            *att.borrow_mut() = None;
+            self.update_status();
+        });
+    }
+
+    pub fn detach_texture(&self, texture: &WebGLTexture) {
+        self.with_matching_textures(texture, |att| {
+            *att.borrow_mut() = None;
+            self.update_status();
+        });
+    }
+
+    pub fn invalidate_renderbuffer(&self, rb: &WebGLRenderbuffer) {
+        self.with_matching_renderbuffers(rb, |_att| {
+            self.update_status();
+        });
+    }
+
+    pub fn invalidate_texture(&self, texture: &WebGLTexture) {
+        self.with_matching_textures(texture, |_att| {
+            self.update_status();
+        });
+    }
+
     pub fn target(&self) -> Option<u32> {
         self.target.get()
     }

components/script/dom/webglrenderbuffer.rs

@@ -20,6 +20,7 @@ pub struct WebGLRenderbuffer {
     id: WebGLRenderbufferId,
     ever_bound: Cell<bool>,
     is_deleted: Cell<bool>,
+    size: Cell<Option<(i32, i32)>>,
     internal_format: Cell<Option<u32>>,
     #[ignore_heap_size_of = "Defined in ipc-channel"]
     renderer: IpcSender<CanvasMsg>,
@@ -36,6 +37,7 @@ impl WebGLRenderbuffer {
             is_deleted: Cell::new(false),
             renderer: renderer,
             internal_format: Cell::new(None),
+            size: Cell::new(None),
         }
     }
 
@@ -64,6 +66,10 @@ impl WebGLRenderbuffer {
         self.id
     }
 
+    pub fn size(&self) -> Option<(i32, i32)> {
+        self.size.get()
+    }
+
     pub fn bind(&self, target: u32) {
         self.ever_bound.set(true);
         let msg = CanvasMsg::WebGL(WebGLCommand::BindRenderbuffer(target, Some(self.id)));
@@ -106,6 +112,8 @@ impl WebGLRenderbuffer {
                                                                      internal_format, width, height));
         self.renderer.send(msg).unwrap();
 
+        self.size.set(Some((width, height)));
+
         Ok(())
     }
 }

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 |
@@ -465,7 +475,11 @@ impl WebGLRenderingContext {
 
         self.ipc_renderer
             .send(CanvasMsg::WebGL(msg))
-            .unwrap()
+            .unwrap();
+
+        if let Some(fb) = self.bound_framebuffer.get() {
+            fb.invalidate_texture(&*texture);
+        }
     }
 
     fn tex_sub_image_2d(&self,
@@ -645,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);
+                }
+            }
             _ => {}
         }
 
@@ -1773,6 +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)))
@@ -1780,12 +1888,11 @@ impl WebGLRenderingContextMethods for WebGLRenderingContext {
 
         let result = receiver.recv().unwrap();
 
-        if result.len() > data.len() {
+        for i in 0..height {
-            return Ok(self.webgl_error(InvalidOperation));
+            for j in 0..(width * cpp) {
-        }
+                data[(dst_offset + i * stride + j) as usize] =
-
+                    result[(i * width * cpp + j) as usize];
-        for i in 0..result.len() {
+            }
-            data[i] = result[i]
         }
 
         Ok(())
@@ -2621,7 +2728,12 @@ impl WebGLRenderingContextMethods for WebGLRenderingContext {
         }
 
         match self.bound_renderbuffer.get() {
-            Some(rb) => handle_potential_webgl_error!(self, rb.storage(internal_format, width, height)),
+            Some(rb) => {
+                handle_potential_webgl_error!(self, rb.storage(internal_format, width, height));
+                if let Some(fb) = self.bound_framebuffer.get() {
+                    fb.invalidate_renderbuffer(&*rb);
+                }
+            }
             None => self.webgl_error(InvalidOperation),
         };
 

components/script/dom/webgltexture.rs

@@ -332,7 +332,7 @@ impl WebGLTexture {
         self.image_info_at_face(face_index, level)
     }
 
-    fn image_info_at_face(&self, face: u8, level: u32) -> ImageInfo {
+    pub fn image_info_at_face(&self, face: u8, level: u32) -> ImageInfo {
         let pos = (level * self.face_count.get() as u32) + face as u32;
         self.image_info_array.borrow()[pos as usize]
     }

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
 

tests/wpt/metadata/webgl/conformance-1.0.3/conformance/more/functions/readPixelsBadArgs.html.ini

@@ -1,8 +0,0 @@
-[readPixelsBadArgs.html]
-  type: testharness
-  expected:
-    if os == "linux": CRASH
-    if os == "mac": TIMEOUT
-  [WebGL test #0: testReadPixels]
-    expected: FAIL
-

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:
+  [WebGL test #17: pixel should be 255,102,0,255. Was 0,0,0,0.]
-    if os == "linux": CRASH
+    expected: FAIL
-    if os == "mac": TIMEOUT
+
-  [WebGL test #3: successfullyParsed should be true (of type boolean). Was undefined (of type undefined).]
+  [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