servo: Merge #14081 - webgl: out-of-bounds readPixels() fixes (from anholt:webgl-readpix-negative-width); r=emilio
authorEric Anholt <eric@anholt.net>
Wed, 09 Nov 2016 10:03:42 -0600
changeset 340114 5c8b209edbdb8951d0d95f4457e5c880d1fa6875
parent 340113 fc7683fdb6615716ab26a80f39ffb4c6cc15306c
child 340115 24b8e98e68d873b5d35f53ab361cd6d411b81aca
push id31307
push usergszorc@mozilla.com
push dateSat, 04 Feb 2017 00:59:06 +0000
treeherdermozilla-central@94079d43835f [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersemilio
servo: Merge #14081 - webgl: out-of-bounds readPixels() fixes (from anholt:webgl-readpix-negative-width); r=emilio <!-- 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. --> Source-Repo: https://github.com/servo/servo Source-Revision: 2601d8eb8b5988f1f72a2449b904a75f5dab0c59
servo/components/script/dom/webglframebuffer.rs
servo/components/script/dom/webglrenderbuffer.rs
servo/components/script/dom/webglrenderingcontext.rs
servo/components/script/dom/webgltexture.rs
--- a/servo/components/script/dom/webglframebuffer.rs
+++ b/servo/components/script/dom/webglframebuffer.rs
@@ -16,28 +16,29 @@ use dom::webgltexture::WebGLTexture;
 use ipc_channel::ipc::{self, IpcSender};
 use std::cell::Cell;
 use webrender_traits::{WebGLCommand, WebGLFramebufferBindingRequest, WebGLFramebufferId, WebGLResult, WebGLError};
 
 #[must_root]
 #[derive(JSTraceable, Clone, HeapSizeOf)]
 enum WebGLFramebufferAttachment {
     Renderbuffer(JS<WebGLRenderbuffer>),
-    Texture(JS<WebGLTexture>),
+    Texture { texture: JS<WebGLTexture>, level: i32 },
 }
 
 impl HeapGCValue for WebGLFramebufferAttachment {}
 
 #[dom_struct]
 pub struct WebGLFramebuffer {
     webgl_object: WebGLObject,
     id: WebGLFramebufferId,
     /// 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>,
 
     // The attachment points for textures and renderbuffers on this
     // FBO.
     color: DOMRefCell<Option<WebGLFramebufferAttachment>>,
     depth: DOMRefCell<Option<WebGLFramebufferAttachment>>,
@@ -50,16 +51,17 @@ impl WebGLFramebuffer {
                      id: WebGLFramebufferId)
                      -> WebGLFramebuffer {
         WebGLFramebuffer {
             webgl_object: WebGLObject::new_inherited(),
             id: id,
             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),
             stencil: DOMRefCell::new(None),
             depthstencil: DOMRefCell::new(None),
         }
     }
 
@@ -105,21 +107,30 @@ impl WebGLFramebuffer {
             let _ = self.renderer.send(CanvasMsg::WebGL(WebGLCommand::DeleteFramebuffer(self.id)));
         }
     }
 
     pub fn is_deleted(&self) -> bool {
         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 has_z = self.depth.borrow().is_some();
-        let has_s = self.stencil.borrow().is_some();
-        let has_zs = self.depthstencil.borrow().is_some();
+        let c = self.color.borrow();
+        let z = self.depth.borrow();
+        let s = self.stencil.borrow();
+        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"):
         //
         //    "In the WebGL API, it is an error to concurrently attach
         //     renderbuffers to the following combinations of
         //     attachment points:
         //
         //     DEPTH_ATTACHMENT + DEPTH_STENCIL_ATTACHMENT
@@ -130,16 +141,43 @@ impl WebGLFramebuffer {
         //
         //     checkFramebufferStatus must return FRAMEBUFFER_UNSUPPORTED."
         if (has_zs && (has_z || has_s)) ||
             (has_z && has_s) {
             self.status.set(constants::FRAMEBUFFER_UNSUPPORTED);
             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 {
             self.status.set(constants::FRAMEBUFFER_UNSUPPORTED);
         }
     }
 
     pub fn check_status(&self) -> u32 {
@@ -185,18 +223,16 @@ impl WebGLFramebuffer {
             constants::DEPTH_STENCIL_ATTACHMENT => &self.depthstencil,
             _ => return Err(WebGLError::InvalidEnum),
         };
 
         let tex_id = match texture {
             // 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
                 //      to be attached to the framebuffer and must be
                 //      0. Otherwise, INVALID_VALUE is generated."
                 if level != 0 {
                     return Err(WebGLError::InvalidValue);
                 }
@@ -226,79 +262,112 @@ impl WebGLFramebuffer {
                 };
 
                 match texture.target() {
                     Some(constants::TEXTURE_CUBE_MAP) if is_cube => {}
                     Some(_) if !is_cube => {}
                     _ => 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
             }
         };
 
         self.renderer.send(CanvasMsg::WebGL(WebGLCommand::FramebufferTexture2D(constants::FRAMEBUFFER,
                                                                                attachment,
                                                                                textarget,
                                                                                tex_id,
                                                                                level))).unwrap();
 
         self.update_status();
         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,
                            &self.depthstencil];
 
         for attachment in &attachments {
             let matched = {
                 match *attachment.borrow() {
                     Some(WebGLFramebufferAttachment::Renderbuffer(ref att_rb))
                         if rb.id() == att_rb.id() => true,
                     _ => false,
                 }
             };
 
             if matched {
-                *attachment.borrow_mut() = None;
-                self.update_status();
+                closure(attachment);
             }
         }
     }
 
-    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,
                            &self.depthstencil];
 
         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()
     }
 }
 
 impl Drop for WebGLFramebuffer {
     fn drop(&mut self) {
         self.delete();
--- a/servo/components/script/dom/webglrenderbuffer.rs
+++ b/servo/components/script/dom/webglrenderbuffer.rs
@@ -15,32 +15,34 @@ use std::cell::Cell;
 use webrender_traits::{WebGLCommand, WebGLRenderbufferId, WebGLResult, WebGLError};
 
 #[dom_struct]
 pub struct WebGLRenderbuffer {
     webgl_object: WebGLObject,
     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>,
 }
 
 impl WebGLRenderbuffer {
     fn new_inherited(renderer: IpcSender<CanvasMsg>,
                      id: WebGLRenderbufferId)
                      -> WebGLRenderbuffer {
         WebGLRenderbuffer {
             webgl_object: WebGLObject::new_inherited(),
             id: id,
             ever_bound: Cell::new(false),
             is_deleted: Cell::new(false),
             renderer: renderer,
             internal_format: Cell::new(None),
+            size: Cell::new(None),
         }
     }
 
     pub fn maybe_new(global: &GlobalScope, renderer: IpcSender<CanvasMsg>)
                      -> Option<Root<WebGLRenderbuffer>> {
         let (sender, receiver) = ipc::channel().unwrap();
         renderer.send(CanvasMsg::WebGL(WebGLCommand::CreateRenderbuffer(sender))).unwrap();
 
@@ -59,16 +61,20 @@ impl WebGLRenderbuffer {
 }
 
 
 impl WebGLRenderbuffer {
     pub fn id(&self) -> WebGLRenderbufferId {
         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)));
         self.renderer.send(msg).unwrap();
     }
 
     pub fn delete(&self) {
         if !self.is_deleted.get() {
@@ -101,11 +107,13 @@ impl WebGLRenderbuffer {
         // FIXME: Check that w/h are < MAX_RENDERBUFFER_SIZE
 
         // FIXME: Invalidate completeness after the call
 
         let msg = CanvasMsg::WebGL(WebGLCommand::RenderbufferStorage(constants::RENDERBUFFER,
                                                                      internal_format, width, height));
         self.renderer.send(msg).unwrap();
 
+        self.size.set(Some((width, height)));
+
         Ok(())
     }
 }
--- a/servo/components/script/dom/webglrenderingcontext.rs
+++ b/servo/components/script/dom/webglrenderingcontext.rs
@@ -278,16 +278,26 @@ impl WebGLRenderingContext {
             self.current_vertex_attrib_0.set((x, y, z, w))
         }
 
         self.ipc_renderer
             .send(CanvasMsg::WebGL(WebGLCommand::VertexAttrib(indx, x, y, z, w)))
             .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 |
             constants::INVERT | constants::INCR_WRAP | constants::DECR_WRAP => true,
             _ => false,
         }
     }
 
@@ -460,17 +470,21 @@ impl WebGLRenderingContext {
         let msg = WebGLCommand::TexImage2D(target.as_gl_constant(), level as i32,
                                            internal_format.as_gl_constant() as i32,
                                            width as i32, height as i32,
                                            internal_format.as_gl_constant(),
                                            data_type.as_gl_constant(), pixels);
 
         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,
                         texture: Root<WebGLTexture>,
                         target: TexImageTarget,
                         level: u32,
                         xoffset: i32,
                         yoffset: i32,
@@ -640,16 +654,36 @@ impl WebGLRenderingContextMethods for We
             constants::FRAMEBUFFER_BINDING =>
                 return object_binding_to_js_or_null!(cx, &self.bound_framebuffer),
             constants::RENDERBUFFER_BINDING =>
                 return object_binding_to_js_or_null!(cx, &self.bound_renderbuffer),
             constants::TEXTURE_BINDING_2D =>
                 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);
+                }
+            }
             _ => {}
         }
 
         let (sender, receiver) = ipc::channel().unwrap();
         self.ipc_renderer
             .send(CanvasMsg::WebGL(WebGLCommand::GetParameter(parameter, sender)))
             .unwrap();
         match handle_potential_webgl_error!(self, receiver.recv().unwrap(), WebGLParameter::Invalid) {
@@ -1768,29 +1802,102 @@ impl WebGLRenderingContextMethods for We
             return Ok(());
         }
 
         match unsafe { JS_GetArrayBufferViewType(pixels) } {
             Type::Uint8 => (),
             _ => 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)))
             .unwrap();
 
         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(())
     }
 
     // https://www.khronos.org/registry/webgl/specs/latest/1.0/#5.14.3
     fn SampleCoverage(&self, value: f32, invert: bool) {
         self.ipc_renderer.send(CanvasMsg::WebGL(WebGLCommand::SampleCoverage(value, invert))).unwrap();
@@ -2616,17 +2723,22 @@ impl WebGLRenderingContextMethods for We
         //
         // FIXME: Handle max_renderbuffer_size, which doesn't seem to
         // be in limits.
         if width < 0 || height < 0 {
             return self.webgl_error(InvalidValue);
         }
 
         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),
         };
 
         // FIXME: We need to clear the renderbuffer before it can be
         // accessed.  See https://github.com/servo/servo/issues/13710
     }
 
     fn FramebufferRenderbuffer(&self, target: u32, attachment: u32,
--- a/servo/components/script/dom/webgltexture.rs
+++ b/servo/components/script/dom/webgltexture.rs
@@ -327,17 +327,17 @@ impl WebGLTexture {
 
     pub fn image_info_for_target(&self,
                                  target: &TexImageTarget,
                                  level: u32) -> ImageInfo {
         let face_index = self.face_index_for_target(&target);
         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]
     }
 
     fn set_image_infos_at_level(&self, level: u32, image_info: ImageInfo) {
         for face in 0..self.face_count.get() {
             self.set_image_infos_at_level_and_face(level, face, image_info);
         }