Soundness: RenderStateRowCells::graphemes_buf is safe but passes no capacity to the FFI

[molofsky]

Hi — found this while debugging something unrelated in a downstream consumer. It didn't turn out to be the cause of the bug I was chasing, so this is a latent soundness report rather than a live incident.

The issue

RenderStateRowCells::graphemes_buf is a safe pub fn, but it hands the C side only a pointer — never the buffer's length:

https://github.com/Uzaaft/libghostty-rs/blob/master/crates/libghostty-vt/src/render.rs#L822-L831

pub fn graphemes_buf(&self, buf: &mut [char]) -> Result<()> {
    let result = unsafe {
        ffi::ghostty_render_state_row_cells_get(
            self.iter.0.as_raw(),
            ffi::RenderStateRowCellsData::GRAPHEMES_BUF,
            buf.as_mut_ptr().cast(),   // <- no capacity
        )
    };
    from_result(result)
}

GRAPHEMES_BUF therefore cannot bound its write or report OUT_OF_SPACE. Safe Rust can call it with a slice that's too small and the write runs past the end of the allocation:

// no `unsafe` anywhere — but this writes out of bounds on any cell with graphemes
let mut buf: [char; 0] = [];
cell.graphemes_buf(&mut buf)?;

The doc comment states the requirement ("The buffer must be at least graphemes_len() elements"), but a safe function can't rely on a doc comment for memory safety.

The two sibling APIs both do bound it

That contrast is what convinced me this is an oversight rather than a deliberate trade-off:

API	passed to FFI	bounded
GridRef::graphemes (screen.rs:94)	ptr, buf.len(), out-len	yes
RenderStateRowCells::graphemes_utf8 (render.rs:840)	Buffer { ptr, **cap**, len }, retries on OUT_OF_SPACE	yes
RenderStateRowCells::graphemes_buf (render.rs:822)	ptr only	no

There's also a TOCTOU in the safe wrapper

graphemes() reads the length, allocates exactly that, then fills:

pub fn graphemes(&self) -> Result<Vec<char>> {
    let len = self.graphemes_len()?;
    let mut graphemes = vec!['\0'; len];
    self.graphemes_buf(&mut graphemes)?;
    Ok(graphemes)
}

Because the write is unbounded, that length is only advisory — anything that changes the cell's grapheme count between the read and the fill overflows the Vec.

Suggested patch

Two parts. Reimplement graphemes() on the bounded path, and mark graphemes_buf unsafe, since it can't uphold safety with the current C signature:

 pub fn graphemes(&self) -> Result<Vec<char>> {
-    let len = self.graphemes_len()?;
-    let mut graphemes = vec!['\0'; len];
-    self.graphemes_buf(&mut graphemes)?;
-    Ok(graphemes)
+    // `graphemes_utf8` passes ptr + capacity and grows on OUT_OF_SPACE, so it
+    // cannot overflow; `graphemes_buf` has no capacity to check against.
+    let mut s = String::new();
+    self.graphemes_utf8(&mut s)?;
+    Ok(s.chars().collect())
 }

+/// # Safety
+///
+/// The C API for `GRAPHEMES_BUF` receives only a pointer — it is never told how large
+/// `buf` is and cannot bound its write or report `OUT_OF_SPACE`. The caller must
+/// guarantee `buf` has at least [`Self::graphemes_len`] elements *at the moment of this
+/// call*. Prefer [`Self::graphemes_utf8`].
-pub fn graphemes_buf(&self, buf: &mut [char]) -> Result<()> {
+pub unsafe fn graphemes_buf(&self, buf: &mut [char]) -> Result<()> {

I recognise marking a published pub fn unsafe is a breaking change, so you may prefer only the graphemes() half for now, or adding a length parameter to the GRAPHEMES_BUF tag on the C side so it can return OUT_OF_SPACE like the UTF-8 variant does.

Happy to open a PR for whichever shape you'd prefer — I didn't want to presume with a breaking API change. Verified present on master (72ac98f) and in the published 0.2.1.

[Uzaaft]

Thanks for the report. I’ll investigate the soundness issue.

I don’t think the proposed patch is acceptable as is: making an existing safe public function unsafe is a breaking change, and routing graphemes() through UTF-8 seems unnecessary.

One correction: I don’t believe the TOCTOU claim about graphemes() is valid. The borrowing and lifetime structure should prevent the render state or iterator position from changing between the length and buffer calls. I’ll verify this while I investigate this.

[molofsky]

I withdraw the TOCTOU claim. Object holds a NonNull. Nothing impls Send or Sync, so CellIterator stays on one thread. Both accessors take &self, and the count cannot change.

You are right about the UTF-8 routing. It encodes in C, then decodes back, which costs two allocations per cell.

Instead, drop the unsafe marking and check the length in the wrapper.

pub fn graphemes_buf(&self, buf: &mut [char]) -> Result<()> {
    let required = self.graphemes_len()?;
    if buf.len() < required {
        return Err(Error::OutOfSpace { required });
    }
    // SAFETY: buf holds at least `required` elements, checked
    // above. CellIterator is !Send and !Sync, and both calls
    // take &self, so the count cannot change.
    let result = unsafe { /* unchanged */ };
    from_result(result)
}

This stays safe and public. graphemes() keeps the char path, and it adds one FFI call per cell.