From: "brian m. carlson" <sandals@crustytoothpaste•net>
To: Patrick Steinhardt <ps@pks•im>
Cc: git@vger•kernel.org, Ezekiel Newren <ezekielnewren@gmail•com>,
Johannes Schindelin <Johannes.Schindelin@gmx•de>
Subject: Re: [PATCH 3/6] rust/varint: add safety comments
Date: Wed, 8 Oct 2025 00:29:38 +0000 [thread overview]
Message-ID: <aOWwcqyithDKQzVs@fruit.crustytoothpaste.net> (raw)
In-Reply-To: <20251007-b4-pks-ci-rust-v1-3-394502abe7ea@pks.im>
[-- Attachment #1: Type: text/plain, Size: 4468 bytes --]
On 2025-10-07 at 12:36:31, Patrick Steinhardt wrote:
> +/// # 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
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 262 bytes --]
next prev parent reply other threads:[~2025-10-08 0:29 UTC|newest]
Thread overview: 37+ messages / expand[flat|nested] mbox.gz Atom feed top
2025-10-07 12:36 [PATCH 0/6] ci: improvements to our Rust infrastructure Patrick Steinhardt
2025-10-07 12:36 ` [PATCH 1/6] ci: deduplicate calls to `apt-get update` Patrick Steinhardt
2025-10-07 12:54 ` Karthik Nayak
2025-10-14 20:56 ` Justin Tobler
2025-10-07 12:36 ` [PATCH 2/6] ci: check formatting of our Rust code Patrick Steinhardt
2025-10-07 13:04 ` Karthik Nayak
2025-10-07 13:50 ` Patrick Steinhardt
2025-10-07 17:13 ` Eric Sunshine
2025-10-07 17:38 ` Junio C Hamano
2025-10-07 18:03 ` Eric Sunshine
2025-10-07 22:42 ` brian m. carlson
2025-10-07 22:58 ` Chris Torek
2025-10-08 4:46 ` Patrick Steinhardt
2025-10-08 15:34 ` Junio C Hamano
2025-10-09 5:29 ` Patrick Steinhardt
2025-10-29 22:54 ` SZEDER Gábor
2025-10-07 22:07 ` brian m. carlson
2025-10-08 20:55 ` SZEDER Gábor
2025-10-09 5:29 ` Patrick Steinhardt
2025-10-29 21:19 ` SZEDER Gábor
2025-10-07 12:36 ` [PATCH 3/6] rust/varint: add safety comments Patrick Steinhardt
2025-10-08 0:29 ` brian m. carlson [this message]
2025-10-08 4:46 ` Patrick Steinhardt
2025-10-07 12:36 ` [PATCH 4/6] ci: check for common Rust mistakes via Clippy Patrick Steinhardt
2025-10-07 12:36 ` [PATCH 5/6] ci: verify minimum supported Rust version Patrick Steinhardt
2025-10-07 12:36 ` [PATCH 6/6] rust: support for Windows Patrick Steinhardt
2025-10-15 6:04 ` [PATCH v3 0/6] ci: improvements to our Rust infrastructure Patrick Steinhardt
2025-10-15 6:04 ` [PATCH v3 1/6] ci: deduplicate calls to `apt-get update` Patrick Steinhardt
2025-10-15 6:04 ` [PATCH v3 2/6] ci: check formatting of our Rust code Patrick Steinhardt
2025-10-15 6:04 ` [PATCH v3 3/6] rust/varint: add safety comments Patrick Steinhardt
2025-10-15 6:04 ` [PATCH v3 4/6] ci: check for common Rust mistakes via Clippy Patrick Steinhardt
2025-10-15 6:04 ` [PATCH v3 5/6] ci: verify minimum supported Rust version Patrick Steinhardt
2025-10-15 6:04 ` [PATCH v3 6/6] rust: support for Windows Patrick Steinhardt
2025-11-20 19:45 ` Ezekiel Newren
2025-11-21 8:18 ` Johannes Schindelin
2025-11-21 21:39 ` Junio C Hamano
2025-10-15 15:21 ` [PATCH v3 0/6] ci: improvements to our Rust infrastructure Junio C Hamano
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=aOWwcqyithDKQzVs@fruit.crustytoothpaste.net \
--to=sandals@crustytoothpaste$(echo .)net \
--cc=Johannes.Schindelin@gmx$(echo .)de \
--cc=ezekielnewren@gmail$(echo .)com \
--cc=git@vger$(echo .)kernel.org \
--cc=ps@pks$(echo .)im \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox