diff --git a/src/vcl/backend.rs b/src/vcl/backend.rs index 529d7f1..5c2f226 100644 --- a/src/vcl/backend.rs +++ b/src/vcl/backend.rs @@ -307,12 +307,12 @@ unsafe extern "C" fn vfp_pull( Err(e) => { let msg = e.to_string(); // TODO: we should grow a VSL object - let t = ffi::txt { - b: msg.as_ptr().cast::(), - e: msg.as_ptr().add(msg.len()).cast::(), - }; - 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) => { diff --git a/src/vcl/ctx.rs b/src/vcl/ctx.rs index 588ad1a..e884091 100644 --- a/src/vcl/ctx.rs +++ b/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::(), - e: msg.as_ptr().add(msg.len()).cast::(), - }; - 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), }; diff --git a/src/vcl/http.rs b/src/vcl/http.rs index cf2ed36..0a981fd 100644 --- a/src/vcl/http.rs +++ b/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::(), 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::(), 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)) -} diff --git a/src/vcl/utils.rs b/src/vcl/utils.rs index c5964d5..46f1b88 100644 --- a/src/vcl/utils.rs +++ b/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::(), + e: unsafe { s.as_ptr().add(s.len()).cast::() }, + } + } + + /// 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::(), + 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);