Merge pull request #38 from rust3ds/fix/instance-render-target-lifetime

citro3d/examples/triangle.rs

@@ -65,17 +65,20 @@ fn main() {
     let (mut top_left, mut top_right) = top_screen.split_mut();
 
     let RawFrameBuffer { width, height, .. } = top_left.raw_framebuffer();
-    let mut top_left_target =
-        render::Target::new(width, height, top_left, None).expect("failed to create render target");
+    let mut top_left_target = instance
+        .render_target(width, height, top_left, None)
+        .expect("failed to create render target");
 
     let RawFrameBuffer { width, height, .. } = top_right.raw_framebuffer();
-    let mut top_right_target = render::Target::new(width, height, top_right, None)
+    let mut top_right_target = instance
+        .render_target(width, height, top_right, None)
         .expect("failed to create render target");
 
     let mut bottom_screen = gfx.bottom_screen.borrow_mut();
     let RawFrameBuffer { width, height, .. } = bottom_screen.raw_framebuffer();
 
-    let mut bottom_target = render::Target::new(width, height, bottom_screen, None)
+    let mut bottom_target = instance
+        .render_target(width, height, bottom_screen, None)
         .expect("failed to create bottom screen render target");
 
     let shader = shader::Library::from_bytes(SHADER_BYTES).unwrap();

citro3d/src/lib.rs

@@ -25,9 +25,11 @@ pub mod shader;
 pub mod texenv;
 pub mod uniform;
 
-use std::cell::OnceCell;
+use std::cell::{OnceCell, RefMut};
 use std::fmt;
+use std::rc::Rc;
 
+use ctru::services::gfx::Screen;
 pub use error::{Error, Result};
 
 use self::texenv::TexEnv;
@@ -44,8 +46,15 @@ pub mod macros {
 #[must_use]
 pub struct Instance {
     texenvs: [OnceCell<TexEnv>; texenv::TEXENV_COUNT],
+    queue: Rc<RenderQueue>,
 }
 
+/// Representation of `citro3d`'s internal render queue. This is something that
+/// lives in the global context, but it keeps references to resources that are
+/// used for rendering, so it's useful for us to have something to represent its
+/// lifetime.
+struct RenderQueue;
+
 impl fmt::Debug for Instance {
     fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
         f.debug_struct("Instance").finish_non_exhaustive()
@@ -80,17 +89,39 @@ impl Instance {
                     OnceCell::new(),
                     OnceCell::new(),
                 ],
+                queue: Rc::new(RenderQueue),
             })
         } else {
             Err(Error::FailedToInitialize)
         }
     }
 
-    /// Select the given render target for drawing the frame.
+    /// Create a new render target with the specified size, color format,
+    /// and depth format.
+    ///
+    /// # Errors
+    ///
+    /// Fails if the target could not be created with the given parameters.
+    #[doc(alias = "C3D_RenderTargetCreate")]
+    #[doc(alias = "C3D_RenderTargetSetOutput")]
+    pub fn render_target<'screen>(
+        &self,
+        width: usize,
+        height: usize,
+        screen: RefMut<'screen, dyn Screen>,
+        depth_format: Option<render::DepthFormat>,
+    ) -> Result<render::Target<'screen>> {
+        render::Target::new(width, height, screen, depth_format, Rc::clone(&self.queue))
+    }
+
+    /// Select the given render target for drawing the frame. This must be called
+    /// as pare of a render call (i.e. within the call to
+    /// [`render_frame_with`](Self::render_frame_with)).
     ///
     /// # Errors
     ///
-    /// Fails if the given target cannot be used for drawing.
+    /// Fails if the given target cannot be used for drawing, or called outside
+    /// the context of a frame render.
     #[doc(alias = "C3D_FrameDrawOn")]
     pub fn select_render_target(&mut self, target: &render::Target<'_>) -> Result<()> {
         let _ = self;
@@ -236,11 +267,42 @@ impl Instance {
     }
 }
 
+// This only exists to be an alias, which admittedly is kinda silly. The default
+// impl should be equivalent though, since RenderQueue has a drop impl too.
 impl Drop for Instance {
     #[doc(alias = "C3D_Fini")]
+    fn drop(&mut self) {}
+}
+
+impl Drop for RenderQueue {
     fn drop(&mut self) {
         unsafe {
             citro3d_sys::C3D_Fini();
         }
     }
 }
+
+#[cfg(test)]
+mod tests {
+    use ctru::services::gfx::Gfx;
+
+    use super::*;
+
+    #[test]
+    fn select_render_target() {
+        let gfx = Gfx::new().unwrap();
+        let screen = gfx.top_screen.borrow_mut();
+
+        let mut instance = Instance::new().unwrap();
+        let target = instance.render_target(10, 10, screen, None).unwrap();
+
+        instance.render_frame_with(|instance| {
+            instance.select_render_target(&target).unwrap();
+        });
+
+        // Check that we don't get a double-free or use-after-free by dropping
+        // the global instance before dropping the target.
+        drop(instance);
+        drop(target);
+    }
+}

citro3d/src/render.rs

@@ -2,6 +2,7 @@
 //! of data to the GPU, including the format of color and depth data to be rendered.
 
 use std::cell::RefMut;
+use std::rc::Rc;
 
 use citro3d_sys::{
     C3D_RenderTarget, C3D_RenderTargetCreate, C3D_RenderTargetDelete, C3D_DEPTHTYPE,
@@ -10,7 +11,7 @@ use ctru::services::gfx::Screen;
 use ctru::services::gspgpu::FramebufferFormat;
 use ctru_sys::{GPU_COLORBUF, GPU_DEPTHBUF};
 
-use crate::{Error, Result};
+use crate::{Error, RenderQueue, Result};
 
 mod transfer;
 
@@ -22,6 +23,7 @@ pub struct Target<'screen> {
     // This is unused after construction, but ensures unique access to the
     // screen this target writes to during rendering
     _screen: RefMut<'screen, dyn Screen>,
+    _queue: Rc<RenderQueue>,
 }
 
 impl Drop for Target<'_> {
@@ -34,19 +36,15 @@ impl Drop for Target<'_> {
 }
 
 impl<'screen> Target<'screen> {
-    /// Create a new render target with the specified size, color format,
-    /// and depth format.
-    ///
-    /// # Errors
-    ///
-    /// Fails if the target could not be created.
-    #[doc(alias = "C3D_RenderTargetCreate")]
-    #[doc(alias = "C3D_RenderTargetSetOutput")]
-    pub fn new(
+    /// Create a new render target with the given parameters. This takes a
+    /// [`RenderQueue`] parameter to make sure this  [`Target`] doesn't outlive
+    /// the render queue.
+    pub(crate) fn new(
         width: usize,
         height: usize,
         screen: RefMut<'screen, dyn Screen>,
         depth_format: Option<DepthFormat>,
+        queue: Rc<RenderQueue>,
     ) -> Result<Self> {
         let color_format: ColorFormat = screen.framebuffer_format().into();
 
@@ -80,6 +78,7 @@ impl<'screen> Target<'screen> {
         Ok(Self {
             raw,
             _screen: screen,
+            _queue: queue,
         })
     }