Fix segfault when drawing paths (#4084)

Antonio Scandurra created 2 years ago

Previously, we were using `size_of` but passing the wrong type in
(`MonochromeSprite` instead of `PathSprite`). This caused us to read
outside of the `sprites` smallvec and triggered the segfault. This
reverts #4078 because I don't think using a `SmallVec` was the issue (it
might have masked this problem though, because we would most of the time
copy from the stack and not from the heap).

With this pull request we are also fixing another potential source of
segfaults, due to checking if we exhausted the instance buffer too late
when drawing underlines.

Release Notes:

- Fixed a crash that could happen during rendering.

Change summary

crates/gpui/src/platform/mac/metal_renderer.rs | 34 ++++++++++---------
1 file changed, 18 insertions(+), 16 deletions(-)

Detailed changes

crates/gpui/src/platform/mac/metal_renderer.rs

@@ -14,6 +14,7 @@ use foreign_types::ForeignType;
 use media::core_video::CVMetalTextureCache;
 use metal::{CommandQueue, MTLPixelFormat, MTLResourceOptions, NSRange};
 use objc::{self, msg_send, sel, sel_impl};
+use smallvec::SmallVec;
 use std::{ffi::c_void, mem, ptr, sync::Arc};
 
 const SHADERS_METALLIB: &[u8] = include_bytes!(concat!(env!("OUT_DIR"), "/shaders.metallib"));
@@ -81,7 +82,7 @@ impl MetalRenderer {
         ];
         let unit_vertices = device.new_buffer_with_data(
             unit_vertices.as_ptr() as *const c_void,
-            (unit_vertices.len() * mem::size_of::<u64>()) as u64,
+            mem::size_of_val(&unit_vertices) as u64,
             MTLResourceOptions::StorageModeManaged,
         );
         let instances = device.new_buffer(
@@ -339,7 +340,8 @@ impl MetalRenderer {
 
         for (texture_id, vertices) in vertices_by_texture_id {
             align_offset(offset);
-            let next_offset = *offset + vertices.len() * mem::size_of::<PathVertex<ScaledPixels>>();
+            let vertices_bytes_len = mem::size_of_val(vertices.as_slice());
+            let next_offset = *offset + vertices_bytes_len;
             if next_offset > INSTANCE_BUFFER_SIZE {
                 return None;
             }
@@ -372,7 +374,6 @@ impl MetalRenderer {
                 &texture_size as *const Size<DevicePixels> as *const _,
             );
 
-            let vertices_bytes_len = mem::size_of::<PathVertex<ScaledPixels>>() * vertices.len();
             let buffer_contents = unsafe { (self.instances.contents() as *mut u8).add(*offset) };
             unsafe {
                 ptr::copy_nonoverlapping(
@@ -429,7 +430,7 @@ impl MetalRenderer {
             &viewport_size as *const Size<DevicePixels> as *const _,
         );
 
-        let shadow_bytes_len = std::mem::size_of_val(shadows);
+        let shadow_bytes_len = mem::size_of_val(shadows);
         let buffer_contents = unsafe { (self.instances.contents() as *mut u8).add(*offset) };
 
         let next_offset = *offset + shadow_bytes_len;
@@ -490,7 +491,7 @@ impl MetalRenderer {
             &viewport_size as *const Size<DevicePixels> as *const _,
         );
 
-        let quad_bytes_len = std::mem::size_of_val(quads);
+        let quad_bytes_len = mem::size_of_val(quads);
         let buffer_contents = unsafe { (self.instances.contents() as *mut u8).add(*offset) };
 
         let next_offset = *offset + quad_bytes_len;
@@ -537,7 +538,7 @@ impl MetalRenderer {
         );
 
         let mut prev_texture_id = None;
-        let mut sprites = Vec::new();
+        let mut sprites = SmallVec::<[_; 1]>::new();
         let mut paths_and_tiles = paths
             .iter()
             .map(|path| (path, tiles_by_path_id.get(&path.id).unwrap()))
@@ -590,7 +591,7 @@ impl MetalRenderer {
                 command_encoder
                     .set_fragment_texture(SpriteInputIndex::AtlasTexture as u64, Some(&texture));
 
-                let sprite_bytes_len = mem::size_of::<MonochromeSprite>() * sprites.len();
+                let sprite_bytes_len = mem::size_of_val(sprites.as_slice());
                 let next_offset = *offset + sprite_bytes_len;
                 if next_offset > INSTANCE_BUFFER_SIZE {
                     return false;
@@ -655,21 +656,22 @@ impl MetalRenderer {
             &viewport_size as *const Size<DevicePixels> as *const _,
         );
 
-        let quad_bytes_len = std::mem::size_of_val(underlines);
+        let underline_bytes_len = mem::size_of_val(underlines);
         let buffer_contents = unsafe { (self.instances.contents() as *mut u8).add(*offset) };
+
+        let next_offset = *offset + underline_bytes_len;
+        if next_offset > INSTANCE_BUFFER_SIZE {
+            return false;
+        }
+
         unsafe {
             ptr::copy_nonoverlapping(
                 underlines.as_ptr() as *const u8,
                 buffer_contents,
-                quad_bytes_len,
+                underline_bytes_len,
             );
         }
 
-        let next_offset = *offset + quad_bytes_len;
-        if next_offset > INSTANCE_BUFFER_SIZE {
-            return false;
-        }
-
         command_encoder.draw_primitives_instanced(
             metal::MTLPrimitiveType::Triangle,
             0,
@@ -726,7 +728,7 @@ impl MetalRenderer {
         );
         command_encoder.set_fragment_texture(SpriteInputIndex::AtlasTexture as u64, Some(&texture));
 
-        let sprite_bytes_len = std::mem::size_of_val(sprites);
+        let sprite_bytes_len = mem::size_of_val(sprites);
         let buffer_contents = unsafe { (self.instances.contents() as *mut u8).add(*offset) };
 
         let next_offset = *offset + sprite_bytes_len;
@@ -798,7 +800,7 @@ impl MetalRenderer {
         );
         command_encoder.set_fragment_texture(SpriteInputIndex::AtlasTexture as u64, Some(&texture));
 
-        let sprite_bytes_len = std::mem::size_of_val(sprites);
+        let sprite_bytes_len = mem::size_of_val(sprites);
         let buffer_contents = unsafe { (self.instances.contents() as *mut u8).add(*offset) };
 
         let next_offset = *offset + sprite_bytes_len;