Make ffi::Txt safer and simpler to use

Add a few helper methods to help with converting txt to str and back, which also helps with the header parsing.  As a tiny addition, also simplified `TestCtx` instantiation.

src/vcl/backend.rs

@@ -307,12 +307,12 @@ unsafe extern "C" fn vfp_pull<T: Transfer>(
         Err(e) => {
             let msg = e.to_string();
             // TODO: we should grow a VSL object
-            let t = ffi::txt {
-                b: msg.as_ptr().cast::<c_char>(),
-                e: msg.as_ptr().add(msg.len()).cast::<c_char>(),
-            };
-            ffi::VSLbt(ctx.req.as_ref().unwrap().vsl, ffi::VSL_tag_e_SLT_Error, t);
-
+            ffi::VSLbt(
+                ctx.req.as_ref().unwrap().vsl,
+                ffi::VSL_tag_e_SLT_Error,
+                // SAFETY: we assume ffi::VSLbt() will not store the pointer to the string's content
+                ffi::txt::from_str(msg.as_str()),
+            );
             ffi::vfp_status_VFP_ERROR
         }
         Ok(0) => {

src/vcl/ctx.rs

@@ -1,7 +1,6 @@
 //! Expose the Varnish context [`vrt_ctx`] as a Rust object
 //!
-use std::ffi::{c_char, c_int, c_uint, c_void, CString};
-use std::ptr::null_mut;
+use std::ffi::{c_int, c_uint, c_void, CString};
 
 use crate::ffi;
 use crate::ffi::{
@@ -122,11 +121,7 @@ impl<'a> Ctx<'a> {
             if vsl.is_null() {
                 log(logtag, msg);
             } else {
-                let t = ffi::txt {
-                    b: msg.as_ptr().cast::<c_char>(),
-                    e: msg.as_ptr().add(msg.len()).cast::<c_char>(),
-                };
-                ffi::VSLbt(vsl, logtag.into_u32(), t);
+                ffi::VSLbt(vsl, logtag.into_u32(), ffi::txt::from_str(msg));
             }
         }
     }
@@ -183,25 +178,7 @@ impl TestCtx {
         let mut test_ctx = TestCtx {
             vrt_ctx: vrt_ctx {
                 magic: VRT_CTX_MAGIC,
-                syntax: 0,
-                method: 0,
-                vclver: 0,
-                msg: null_mut(),
-                vsl: null_mut(),
-                vcl: null_mut(),
-                ws: null_mut(),
-                sp: null_mut(),
-                req: null_mut(),
-                http_req: null_mut(),
-                http_req_top: null_mut(),
-                http_resp: null_mut(),
-                bo: null_mut(),
-                http_bereq: null_mut(),
-                http_beresp: null_mut(),
-                now: 0.0,
-                specific: null_mut(),
-                called: null_mut(),
-                vpi: null_mut(),
+                ..vrt_ctx::default()
             },
             test_ws: TestWS::new(sz),
         };

src/vcl/http.rs

@@ -13,8 +13,7 @@
 
 #![allow(clippy::not_unsafe_ptr_arg_deref)]
 use std::ffi::c_uint;
-use std::slice::{from_raw_parts, from_raw_parts_mut};
-use std::str::from_utf8;
+use std::slice::from_raw_parts_mut;
 
 use crate::ffi;
 use crate::vcl::ws::WS;
@@ -50,12 +49,9 @@ impl<'a> HTTP<'a> {
         /* XXX: aliasing warning, it's the same pointer as the one in Ctx */
         let mut ws = WS::new(self.raw.ws);
         unsafe {
-            let hdr_buf = ws.copy_bytes_with_null(&value)?;
-            let hd = self.raw.hd.offset(idx as isize);
-            (*hd).b = hdr_buf.as_ptr();
-            // .e points to the NULL byte at the end of the string
-            (*hd).e = hdr_buf.as_ptr().add(hdr_buf.count_bytes());
-            let hdf = self.raw.hdf.offset(idx as isize);
+            let hd = self.raw.hd.offset(idx as isize).as_mut().unwrap();
+            *hd = ffi::txt::from_cstr(ws.copy_bytes_with_null(&value)?);
+            let hdf = self.raw.hdf.offset(idx as isize).as_mut().unwrap();
             *hdf = 0;
         }
         Ok(())
@@ -93,7 +89,7 @@ impl<'a> HTTP<'a> {
 
         let mut idx_empty = 0;
         for (idx, hd) in hdrs.iter().enumerate() {
-            let (n, _) = header_from_hd(hd).unwrap();
+            let (n, _) = hd.parse_header().unwrap();
             if name.eq_ignore_ascii_case(n) {
                 unsafe {
                     ffi::VSLbt(
@@ -129,14 +125,7 @@ impl<'a> HTTP<'a> {
             if idx >= self.raw.nhd {
                 None
             } else {
-                let b = (*self.raw.hd.offset(idx as isize)).b;
-                if b.is_null() {
-                    None
-                } else {
-                    let e = (*self.raw.hd.offset(idx as isize)).e;
-                    let buf = from_raw_parts(b.cast::<u8>(), e.offset_from(b) as usize);
-                    Some(from_utf8(buf).unwrap())
-                }
+                self.raw.hd.offset(idx as isize).as_ref().unwrap().to_str()
             }
         }
     }
@@ -240,41 +229,11 @@ impl<'a> Iterator for HTTPIter<'a> {
             if self.cursor >= nhd as isize {
                 return None;
             }
-            let hd = unsafe { self.http.raw.hd.offset(self.cursor) };
+            let hd = unsafe { self.http.raw.hd.offset(self.cursor).as_ref().unwrap() };
             self.cursor += 1;
-            if let Some(hdr) = header_from_hd(hd) {
+            if let Some(hdr) = hd.parse_header() {
                 return Some(hdr);
             }
         }
     }
 }
-
-fn header_from_hd<'a>(txt: *const ffi::txt) -> Option<(&'a str, &'a str)> {
-    let name;
-    let value;
-
-    unsafe {
-        let b = (*txt).b;
-        if b.is_null() {
-            return None;
-        }
-        let e = (*txt).e;
-        let buf = from_raw_parts(b.cast::<u8>(), e.offset_from(b) as usize);
-        // We expect varnishd to always given us a string with a ':' in it
-        // If it's not the case, blow up as it might be a sign of a bigger problem.
-        let colon = buf.iter().position(|x| *x == b':').unwrap();
-
-        name = from_utf8(&buf[..colon]).unwrap();
-
-        if colon == buf.len() - 1 {
-            value = "";
-        } else {
-            let start = &buf[colon + 1..]
-                .iter()
-                .position(|x| !char::is_whitespace(*x as char))
-                .unwrap_or(0);
-            value = from_utf8(&buf[(colon + start + 1)..]).unwrap();
-        }
-    }
-    Some((name, value))
-}

src/vcl/utils.rs

@@ -1,9 +1,67 @@
+use std::ffi::{c_char, CStr};
+use std::slice::from_raw_parts;
+use std::str::from_utf8;
+
 use crate::ffi::{
-    director, req, sess, vcldir, vfp_ctx, vfp_entry, vrt_ctx, ws, DIRECTOR_MAGIC, REQ_MAGIC,
+    director, req, sess, txt, vcldir, vfp_ctx, vfp_entry, vrt_ctx, ws, DIRECTOR_MAGIC, REQ_MAGIC,
     SESS_MAGIC, VCLDIR_MAGIC, VFP_CTX_MAGIC, VFP_ENTRY_MAGIC, VRT_CTX_MAGIC, WS_MAGIC,
 };
 use crate::vcl::backend::{Serve, Transfer, VCLBackendPtr};
 
+impl txt {
+    pub fn from_bytes(s: &[u8]) -> Self {
+        Self {
+            b: s.as_ptr().cast::<c_char>(),
+            e: unsafe { s.as_ptr().add(s.len()).cast::<c_char>() },
+        }
+    }
+
+    /// FIXME: This method is only used when calling [`crate::ffi::VSLbt`],
+    /// and current implementation creates a string without a null terminator to pass it in.
+    /// Going forward, we should probably refactor it to avoid extra string allocation.
+    #[allow(clippy::should_implement_trait)]
+    pub fn from_str(s: &str) -> Self {
+        Self::from_bytes(s.as_bytes())
+    }
+
+    pub fn from_cstr(s: &CStr) -> Self {
+        Self::from_bytes(s.to_bytes())
+    }
+
+    /// Convert the `txt` struct to a `&[u8]`.
+    /// We want to explicitly differentiate between empty (`None`) and null (`Some([])`) strings.
+    pub fn to_slice<'a>(&self) -> Option<&'a [u8]> {
+        if self.b.is_null() {
+            None
+        } else {
+            // SAFETY: We assume that txt instance was created correctly,
+            //         so the pointers are valid and the end is after the beginning.
+            //         Txt instances are part of ffi, so inherently unsafe.
+            unsafe {
+                Some(from_raw_parts(
+                    self.b.cast::<u8>(),
+                    self.e.offset_from(self.b) as usize,
+                ))
+            }
+        }
+    }
+
+    /// Convert the `txt` struct to a `&str`.  Will panic if the string is not valid UTF-8.
+    pub fn to_str<'a>(&self) -> Option<&'a str> {
+        self.to_slice().map(|s| from_utf8(s).unwrap())
+    }
+
+    /// Parse the `txt` struct as a header, returning a tuple with the key and value,
+    /// trimming the value of leading whitespace.
+    pub fn parse_header<'a>(&self) -> Option<(&'a str, &'a str)> {
+        // We expect varnishd to always given us a string with a ':' in it
+        // If it's not the case, blow up as it might be a sign of a bigger problem.
+        let (key, value) = self.to_str()?.split_once(':').unwrap();
+        // FIXME: Consider `.trim_ascii_start()` if unicode is not a concern
+        Some((key, value.trim_start()))
+    }
+}
+
 pub(crate) unsafe fn validate_vfp_ctx(ctxp: *mut vfp_ctx) -> &'static mut vfp_ctx {
     let val = ctxp.as_mut().unwrap();
     assert_eq!(val.magic, VFP_CTX_MAGIC);