From cef617ba029bd5922043bc7bb3a4039cfa92357e Mon Sep 17 00:00:00 2001 From: Jonathan Spira Date: Tue, 12 Sep 2023 13:15:34 -0400 Subject: [PATCH] unsafety in text fix --- imgui/src/lib.rs | 11 ++++++++-- imgui/src/string.rs | 35 ++++++++++++++++++++++---------- imgui/src/widget/drag.rs | 43 ++++++++++++++++++++-------------------- 3 files changed, 56 insertions(+), 33 deletions(-) diff --git a/imgui/src/lib.rs b/imgui/src/lib.rs index f250f70..9eb2353 100644 --- a/imgui/src/lib.rs +++ b/imgui/src/lib.rs @@ -721,9 +721,16 @@ impl Ui { let handle = &mut *self.scratch_buffer().get(); handle.refresh_buffer(); - let label_ptr = handle.push(label); + let label_start = handle.push(label); - let items_inner: Vec<_> = items.iter().map(|&v| handle.push(v)).collect(); + // we do this in two allocations + let items_inner: Vec = items.iter().map(|&v| handle.push(v)).collect(); + let items_inner: Vec<*const _> = items_inner + .into_iter() + .map(|v| handle.buffer.as_ptr().add(v) as *const _) + .collect(); + + let label_ptr = handle.buffer.as_ptr().add(label_start) as *const _; (label_ptr, items_inner) }; diff --git a/imgui/src/string.rs b/imgui/src/string.rs index 601682a..f9a08f8 100644 --- a/imgui/src/string.rs +++ b/imgui/src/string.rs @@ -24,7 +24,9 @@ impl UiBuffer { /// Internal method to push a single text to our scratch buffer. pub fn scratch_txt(&mut self, txt: impl AsRef) -> *const sys::cty::c_char { self.refresh_buffer(); - self.push(txt) + + let start_of_substr = self.push(txt); + unsafe { self.offset(start_of_substr) } } /// Internal method to push an option text to our scratch buffer. @@ -42,7 +44,11 @@ impl UiBuffer { txt_1: impl AsRef, ) -> (*const sys::cty::c_char, *const sys::cty::c_char) { self.refresh_buffer(); - (self.push(txt_0), self.push(txt_1)) + + let first_offset = self.push(txt_0); + let second_offset = self.push(txt_1); + + unsafe { (self.offset(first_offset), self.offset(second_offset)) } } /// Helper method, same as [`Self::scratch_txt`] but with one optional value @@ -58,21 +64,30 @@ impl UiBuffer { } /// Attempts to clear the buffer if it's over the maximum length allowed. + /// This is to prevent us from making a giant vec over time. pub fn refresh_buffer(&mut self) { if self.buffer.len() > self.max_len { self.buffer.clear(); } } - /// Pushes a new scratch sheet text, which means it's not handling any clearing at all. - pub fn push(&mut self, txt: impl AsRef) -> *const sys::cty::c_char { - unsafe { - let len = self.buffer.len(); - self.buffer.extend(txt.as_ref().as_bytes()); - self.buffer.push(b'\0'); + /// Given a position, gives an offset from the start of the scatch buffer. + /// + /// # Safety + /// This can return a pointer to undefined data if given a `pos >= self.buffer.len()`. + /// This is marked as unsafe to reflect that. + pub unsafe fn offset(&self, pos: usize) -> *const sys::cty::c_char { + self.buffer.as_ptr().add(pos) as *const _ + } - self.buffer.as_ptr().add(len) as *const _ - } + /// Pushes a new scratch sheet text and return the byte index where the sub-string + /// starts. + pub fn push(&mut self, txt: impl AsRef) -> usize { + let len = self.buffer.len(); + self.buffer.extend(txt.as_ref().as_bytes()); + self.buffer.push(b'\0'); + + len } } diff --git a/imgui/src/widget/drag.rs b/imgui/src/widget/drag.rs index 1c550b8..bbccebe 100644 --- a/imgui/src/widget/drag.rs +++ b/imgui/src/widget/drag.rs @@ -209,22 +209,22 @@ where /// Returns true if the slider value was changed. #[doc(alias = "DragFloatRange2")] pub fn build(self, ui: &Ui, min: &mut f32, max: &mut f32) -> bool { - let label; - let mut display_format = std::ptr::null(); - let mut max_display_format = std::ptr::null(); - // we do this ourselves the long way... unsafe { let buffer = &mut *ui.scratch_buffer().get(); buffer.refresh_buffer(); - label = buffer.push(self.label); - if let Some(v) = self.display_format { - display_format = buffer.push(v); - } - if let Some(v) = self.max_display_format { - max_display_format = buffer.push(v); - } + let label_start = buffer.push(self.label); + let display_format = self.display_format.as_ref().map(|v| buffer.push(v)); + let max_display_format = self.max_display_format.as_ref().map(|v| buffer.push(v)); + + let label = buffer.offset(label_start); + let display_format = display_format + .map(|v| buffer.offset(v)) + .unwrap_or_else(std::ptr::null); + let max_display_format = max_display_format + .map(|v| buffer.offset(v)) + .unwrap_or_else(std::ptr::null); sys::igDragFloatRange2( label, @@ -253,20 +253,21 @@ where #[doc(alias = "DragIntRange2")] pub fn build(self, ui: &Ui, min: &mut i32, max: &mut i32) -> bool { unsafe { - let mut display_format = std::ptr::null(); - let mut max_display_format = std::ptr::null(); - // we do this ourselves the long way... let buffer = &mut *ui.scratch_buffer().get(); buffer.refresh_buffer(); - let label = buffer.push(self.label); - if let Some(v) = self.display_format { - display_format = buffer.push(v); - } - if let Some(v) = self.max_display_format { - max_display_format = buffer.push(v); - } + let label_start = buffer.push(self.label); + let display_format = self.display_format.as_ref().map(|v| buffer.push(v)); + let max_display_format = self.max_display_format.as_ref().map(|v| buffer.push(v)); + + let label = buffer.offset(label_start); + let display_format = display_format + .map(|v| buffer.offset(v)) + .unwrap_or_else(std::ptr::null); + let max_display_format = max_display_format + .map(|v| buffer.offset(v)) + .unwrap_or_else(std::ptr::null); sys::igDragIntRange2( label,