feat(ffi): improve error buffer writing more (#331)

Builds on #276, but adds some improvements:

No longer silently truncate to ERR_BUF_MAX_LEN, errors can now be any size the caller specifies
Fixes a regression, there's no need for the error to be null terminated, and the value stored at *errbuf_len again matches the length of the actual error text, not the text plus a null terminator
Runs safely under miri

Closes #276

Commits: 

* feat(ffi): improve error buf writing

* chore: make clippy happy

* chore(ffi): Improve error buffer writing more

- Take a `&mut usize` for errbuf_len, since the requirements we have for
  it match the requirements for &mut
- Write directly into the provided buffer, rather than going through an
  intermediate string and cstring
- Simplify doc strings - `size_of::<u8>()` is 1 by definition, and has
  no alignment requirements.

* chore(docs): correct required length of `uuid_hex` parameter

* mark ERR_BUF_MAX_LEN as deprecated

---------

Co-authored-by: Shiroko <hhx.xxm@gmail.com>

Author: Dr-Emann

src/ffi/context.rs

@@ -1,8 +1,7 @@
 use crate::ast::Value;
 use crate::context::Context;
-use crate::ffi::{CValue, ERR_BUF_MAX_LEN};
+use crate::ffi::{write_errbuf, CValue};
 use crate::schema::Schema;
-use std::cmp::min;
 use std::ffi;
 use std::os::raw::c_char;
 use std::slice::from_raw_parts_mut;
@@ -78,28 +77,24 @@ pub unsafe extern "C" fn context_free(context: *mut Context) {
 /// * `field` must be a valid pointer to a C-style string,
 ///   must be properply aligned, and must not have '\0' in the middle.
 /// * `value` must be a valid pointer to a [`CValue`].
-/// * `errbuf` must be valid to read and write for `errbuf_len * size_of::<u8>()` bytes,
-///   and it must be properly aligned.
-/// * `errbuf_len` must be vlaid to read and write for `size_of::<usize>()` bytes,
+/// * `errbuf` must be valid to read and write for `*errbuf_len` bytes.
+/// * `errbuf_len` must be valid to read and write for `size_of::<usize>()` bytes,
 ///   and it must be properly aligned.
 #[no_mangle]
 pub unsafe extern "C" fn context_add_value(
     context: &mut Context,
     field: *const i8,
     value: &CValue,
     errbuf: *mut u8,
-    errbuf_len: *mut usize,
+    errbuf_len: &mut usize,
 ) -> bool {
     let field = ffi::CStr::from_ptr(field as *const c_char)
         .to_str()
         .unwrap();
-    let errbuf = from_raw_parts_mut(errbuf, ERR_BUF_MAX_LEN);
 
     let value: Result<Value, _> = value.try_into();
     if let Err(e) = value {
-        let errlen = min(e.len(), *errbuf_len);
-        errbuf[..errlen].copy_from_slice(&e.as_bytes()[..errlen]);
-        *errbuf_len = errlen;
+        write_errbuf(e, errbuf, errbuf_len);
         return false;
     }
 
@@ -165,9 +160,9 @@ pub unsafe extern "C" fn context_reset(context: &mut Context) {
 ///   must be passed to [`router_execute`] before calling this function,
 ///   and must not be reset by [`context_reset`] before calling this function.
 /// - If `uuid_hex` is not `NULL`, `uuid_hex` must be valid to read and write for
-///   `16 * size_of::<u8>()` bytes, and it must be properly aligned.
+///   `36` bytes.
 /// - If `matched_field` is not `NULL`,
-///   `matched_field` must be a vlaid pointer to a C-style string,
+///   `matched_field` must be a valid pointer to a C-style string,
 ///   must be properly aligned, and must not have '\0' in the middle.
 /// - If `matched_value` is not `NULL`,
 ///   `matched_value` must be valid to read and write for

src/ffi/expression.rs

@@ -1,8 +1,7 @@
 use crate::ast::{BinaryOperator, Expression, LogicalExpression, Predicate};
-use crate::ffi::ERR_BUF_MAX_LEN;
+use crate::ffi::write_errbuf;
 use crate::schema::Schema;
 use bitflags::bitflags;
-use std::cmp::min;
 use std::ffi;
 use std::os::raw::c_char;
 use std::slice::from_raw_parts_mut;
@@ -137,11 +136,11 @@ pub const ATC_ROUTER_EXPRESSION_VALIDATE_BUF_TOO_SMALL: i64 = 2;
 ///
 /// - `atc` must be a valid pointer to a C-style string, properly aligned, and must not contain an internal `\0`.
 /// - `schema` must be a valid pointer returned by [`schema_new`].
-/// - `fields_buf`, must be valid for writing `fields_buf_len * size_of::<u8>()` bytes and properly aligned.
+/// - `fields_buf`, must be valid for writing `*fields_buf_len` bytes.
 /// - `fields_buf_len` must be a valid pointer to write `size_of::<usize>()` bytes and properly aligned.
 /// - `fields_total` must be a valid pointer to write `size_of::<usize>()` bytes and properly aligned.
 /// - `operators` must be a valid pointer to write `size_of::<u64>()` bytes and properly aligned.
-/// - `errbuf` must be valid for reading and writing `errbuf_len * size_of::<u8>()` bytes and properly aligned.
+/// - `errbuf` must be valid for reading and writing `*errbuf_len` bytes.
 /// - `errbuf_len` must be a valid pointer for reading and writing `size_of::<usize>()` bytes and properly aligned.
 ///
 /// [`schema_new`]: crate::ffi::schema::schema_new
@@ -154,32 +153,27 @@ pub unsafe extern "C" fn expression_validate(
     fields_total: *mut usize,
     operators: *mut u64,
     errbuf: *mut u8,
-    errbuf_len: *mut usize,
+    errbuf_len: &mut usize,
 ) -> i64 {
     use std::collections::HashSet;
 
     use crate::parser::parse;
     use crate::semantics::Validate;
 
     let atc = ffi::CStr::from_ptr(atc as *const c_char).to_str().unwrap();
-    let errbuf = from_raw_parts_mut(errbuf, ERR_BUF_MAX_LEN);
 
     // Parse the expression
-    let result = parse(atc).map_err(|e| e.to_string());
+    let result = parse(atc);
     if let Err(e) = result {
-        let errlen = min(e.len(), *errbuf_len);
-        errbuf[..errlen].copy_from_slice(&e.as_bytes()[..errlen]);
-        *errbuf_len = errlen;
+        write_errbuf(e, errbuf, errbuf_len);
         return ATC_ROUTER_EXPRESSION_VALIDATE_FAILED;
     }
     // Unwrap is safe since we've already checked for error
     let ast = result.unwrap();
 
     // Validate expression with schema
-    if let Err(e) = ast.validate(schema).map_err(|e| e.to_string()) {
-        let errlen = min(e.len(), *errbuf_len);
-        errbuf[..errlen].copy_from_slice(&e.as_bytes()[..errlen]);
-        *errbuf_len = errlen;
+    if let Err(e) = ast.validate(schema) {
+        write_errbuf(e, errbuf, errbuf_len);
         return ATC_ROUTER_EXPRESSION_VALIDATE_FAILED;
     }
 
@@ -224,6 +218,7 @@ pub unsafe extern "C" fn expression_validate(
 mod tests {
     use super::*;
     use crate::ast::Type;
+    use crate::ffi::ERR_BUF_MAX_LEN;
 
     fn expr_validate_on(
         schema: &Schema,
@@ -241,7 +236,7 @@ mod tests {
 
         let result = unsafe {
             expression_validate(
-                atc.as_bytes().as_ptr(),
+                atc.as_ptr().cast(),
                 schema,
                 fields_buf.as_mut_ptr(),
                 &mut fields_buf_len,

src/ffi/mod.rs

@@ -7,10 +7,17 @@ use crate::ast::Value;
 use cidr::IpCidr;
 use std::convert::TryFrom;
 use std::ffi;
+use std::fmt::Display;
 use std::net::IpAddr;
 use std::os::raw::c_char;
 use std::slice::from_raw_parts;
+use std::slice::from_raw_parts_mut;
 
+/// A _suggestion_ of the value to use for `errbuf_len`
+///
+/// This value is actually not used for anything in this library,
+/// any length can be passed.
+#[deprecated]
 pub const ERR_BUF_MAX_LEN: usize = 4096;
 
 #[derive(Debug)]
@@ -56,3 +63,27 @@ impl TryFrom<&CValue> for Value {
         })
     }
 }
+
+/// Write displayable error message to the error buffer.
+///
+/// # Arguments
+///
+/// - `err`: the displayable error message.
+/// - `errbuf`: a buffer to store the error message.
+/// - `errbuf_len`: a pointer to the length of the error message buffer.
+///
+/// # Safety
+///
+/// Violating any of the following constraints will result in undefined behavior:
+///
+/// * `errbuf` must be valid to read and write for `*errbuf_len` bytes.
+unsafe fn write_errbuf(err: impl Display, errbuf: *mut u8, errbuf_len: &mut usize) {
+    use std::io::Write;
+
+    let orig_len = *errbuf_len;
+    let mut errbuf = from_raw_parts_mut(errbuf, orig_len);
+    // Ignore truncation error
+    let _ = write!(errbuf, "{}", err);
+    let remaining_len = errbuf.len();
+    *errbuf_len = orig_len - remaining_len;
+}

src/ffi/router.rs

@@ -1,8 +1,7 @@
 use crate::context::Context;
-use crate::ffi::ERR_BUF_MAX_LEN;
+use crate::ffi::write_errbuf;
 use crate::router::Router;
 use crate::schema::Schema;
-use std::cmp::min;
 use std::ffi;
 use std::os::raw::c_char;
 use std::slice::from_raw_parts_mut;
@@ -87,8 +86,7 @@ pub unsafe extern "C" fn router_free(router: *mut Router<&Schema>) {
 ///   and must not have '\0' in the middle.
 /// - `atc` must be a valid pointer to a C-style string, must be properly aligned,
 ///   and must not have '\0' in the middle.
-/// - `errbuf` must be valid to read and write for `errbuf_len * size_of::<u8>()` bytes,
-///   and it must be properly aligned.
+/// - `errbuf` must be valid to read and write for `*errbuf_len` bytes.
 /// - `errbuf_len` must be valid to read and write for `size_of::<usize>()` bytes,
 ///   and it must be properly aligned.
 #[no_mangle]
@@ -98,18 +96,15 @@ pub unsafe extern "C" fn router_add_matcher(
     uuid: *const i8,
     atc: *const i8,
     errbuf: *mut u8,
-    errbuf_len: *mut usize,
+    errbuf_len: &mut usize,
 ) -> bool {
     let uuid = ffi::CStr::from_ptr(uuid as *const c_char).to_str().unwrap();
     let atc = ffi::CStr::from_ptr(atc as *const c_char).to_str().unwrap();
-    let errbuf = from_raw_parts_mut(errbuf, ERR_BUF_MAX_LEN);
 
     let uuid = Uuid::try_parse(uuid).expect("invalid UUID format");
 
     if let Err(e) = router.add_matcher(priority, uuid, atc) {
-        let errlen = min(e.len(), *errbuf_len);
-        errbuf[..errlen].copy_from_slice(&e.as_bytes()[..errlen]);
-        *errbuf_len = errlen;
+        write_errbuf(e, errbuf, errbuf_len);
         return false;
     }
 
@@ -246,6 +241,31 @@ pub unsafe extern "C" fn router_get_fields(
 #[cfg(test)]
 mod tests {
     use super::*;
+    use crate::ffi::ERR_BUF_MAX_LEN;
+
+    #[test]
+    fn test_short_error_buf() {
+        unsafe {
+            let schema = Schema::default();
+            let mut router = Router::new(&schema);
+            let uuid = ffi::CString::new("a921a9aa-ec0e-4cf3-a6cc-1aa5583d150c").unwrap();
+            let junk = ffi::CString::new(vec![b'a'; ERR_BUF_MAX_LEN * 2]).unwrap();
+            let mut errbuf = vec![b'X'; ERR_BUF_MAX_LEN];
+            let mut errbuf_len = 10;
+
+            let result = router_add_matcher(
+                &mut router,
+                1,
+                uuid.as_ptr() as *const i8,
+                junk.as_ptr() as *const i8,
+                errbuf[..errbuf_len].as_mut_ptr(),
+                &mut errbuf_len,
+            );
+            assert!(!result);
+            assert_eq!(errbuf_len, 10);
+            assert_eq!(errbuf[10], b'X');
+        }
+    }
 
     #[test]
     fn test_long_error_message() {