Re: [PATCH 3/6] rust/varint: add safety comments

From: brian m. carlson <sandals@crustytoothpaste.net>
Date: Oct 8, 2025, 00:29 UTC
Message-ID: <aOWwcqyithDKQzVs@fruit.crustytoothpaste.net>
In-Reply-To: <20251007-b4-pks-ci-rust-v1-3-394502abe7ea@pks.im>

On 2025-10-07 at 12:36:31, Patrick Steinhardt wrote:

Show 8 quoted lines

> +/// # Safety
> +///
> +/// The provided buffer must be large enough to store the encoded varint. Callers may either provide
> +/// a `[u8; 16]` here, which is guaranteed to satisfy all encodable numbers. Or they can call this
> +/// function with a `NULL` pointer first to figure out array size.
>  #[no_mangle]
>  pub unsafe extern "C" fn encode_varint(value: u64, buf: *mut u8) -> u8 {
>      let mut varint: [u8; 16] = [0; 16];

I'm planning to do something a little different with this code by refactoring it out into a Rust function, so at that point it will no longer be possible to provide a buffer smaller than 16 bytes. Note that all callers of this function pass a 16-byte buffer, so that should be safe.

That doesn't mean that you can't send this patch (and I think your patch is good), just that we shouldn't tell people we can use a buffer smaller than 16 bytes, since that will at some point no longer be true.

Here's the current version of the patch I'm planning on sending for reference. I can rebase onto your series once Junio picks it up.

-- >% --
From 0000000000000000000000000000000000000000 Mon Sep 17 00:00:00 2001
From: "brian m. carlson" <sandals@crustytoothpaste.net>
Date: Wed, 8 Oct 2025 00:27:56 +0000
Subject: [PATCH] varint: write a safe Rust version of encode_varint

Our original version of encode_varint in Rust used pointers much like the C version did. However, if we end up using this function elsewhere in Rust, it would be better to have a safe version that we can use.

In addition, writing our unsafe C-compatible version in terms of a safe Rust version makes it obvious what our requirements are. For instance, we do not need buf to actually point anywhere and can accept a null pointer if we just want the length, and we can clearly indicate that we require 16 bytes worth of memory to encode data by creating an appropriate slice. All of our existing callers always pass a 16-byte buffer, so we can safely assume that.

We can then improve our Rust version by performing normal bounds checking to make sure that we don't exceed the buffer size and use the standard usize return for lengths, converting as necessary in the C-compatible caller.

Move the C-compatible code to a mod c to keep things tidy and allow us to have a different Rust version.

Signed-off-by: brian m. carlson <sandals@crustytoothpaste.net>
---
 src/varint.rs | 34 ++++++++++++++++++++++++++++------
 1 file changed, 28 insertions(+), 6 deletions(-)

diff --git a/src/varint.rs b/src/varint.rs
index 6e610bdd8e..83990afe7a 100644
--- a/src/varint.rs
+++ b/src/varint.rs
@@ -22,8 +22,7 @@ pub unsafe extern "C" fn decode_varint(bufp: *mut *const u8) -> u64 {
     val
 }
 
-#[no_mangle]
-pub unsafe extern "C" fn encode_varint(value: u64, buf: *mut u8) -> u8 {
+pub fn encode_varint(value: u64, buf: Option<&mut [u8]>) -> usize {
     let mut varint: [u8; 16] = [0; 16];
     let mut pos = varint.len() - 1;
 
@@ -37,16 +36,19 @@ pub unsafe extern "C" fn encode_varint(value: u64, buf: *mut u8) -> u8 {
         value >>= 7;
     }
 
-    if !buf.is_null() {
-        std::ptr::copy_nonoverlapping(varint.as_ptr().add(pos), buf, varint.len() - pos);
+    let len = varint.len() - pos;
+
+    if let Some(buf) = buf {
+        buf[0..len].copy_from_slice(&varint[pos..pos + len]);
     }
 
-    (varint.len() - pos) as u8
+    len
 }
 
 #[cfg(test)]
 mod tests {
-    use super::*;
+    use super::c::encode_varint;
+    use super::decode_varint;
 
     #[test]
     fn test_decode_varint() {
@@ -90,3 +92,23 @@ mod tests {
         }
     }
 }
+
+mod c {
+    /// Encode `value` into `buf` as a variable-length integer unless `buf` is null.
+    ///
+    /// Returns the number of bytes written, or, if `buf` is null, the number of bytes that would be
+    /// used to encode the integer.
+    ///
+    /// # Safety
+    ///
+    /// `buf` must either be null or point to at least 16 bytes of memory.
+    #[no_mangle]
+    pub unsafe extern "C" fn encode_varint(value: u64, buf: *mut u8) -> u8 {
+        let buffer = if buf.is_null() {
+            None
+        } else {
+            Some(std::slice::from_raw_parts_mut(buf, 16))
+        };
+        super::encode_varint(value, buffer) as u8
+    }
+}
-- >% --

-- 
brian m. carlson (they/them)
Toronto, Ontario, CA