Skip to content

Commit

Permalink
Backend cleanups
Browse files Browse the repository at this point in the history
One of my main goals of several recent refactorings is to get rid of as many `*(*(*foo).bar).baz` as possible, replacing them with proper `.as_ref().unwrap()` -- in other words replacing pointer dereferences with proper references. This ensures a single NULL validation location in the `.unwrap()`, and allows Rust to validate all references thereafter.  All these PRs should be considered as intermediary steps - I'm sure we can come up with a better overall abstraction that ensures that no `unsafe` API is exposed by default.

* consolidate `get_backend` into a single validatable function
* a few more wrapper validation functions for clearer intent
* Added a few more `FIXME` -- these are more like notes for future refactorings, to make sure we don't forget about them. Some of them could be fixed with just a comment, explaining why something is done that way.
  • Loading branch information
nyurik authored and gquintard committed Sep 11, 2024
1 parent 7fe71a0 commit ced832d
Show file tree
Hide file tree
Showing 2 changed files with 83 additions and 78 deletions.
87 changes: 30 additions & 57 deletions src/vcl/backend.rs
Original file line number Diff line number Diff line change
Expand Up @@ -82,7 +82,7 @@ use crate::ffi;
use crate::vcl::convert::IntoVCL;
use crate::vcl::ctx::{Ctx, Event, LogTag};
use crate::vcl::utils::{
validate_director, validate_vfp_ctx, validate_vfp_entry, validate_vrt_ctx,
validate_director, validate_vdir, validate_vfp_ctx, validate_vfp_entry, validate_vrt_ctx,
};
use crate::vcl::vsb::Vsb;
use crate::vcl::ws::WS;
Expand Down Expand Up @@ -327,11 +327,8 @@ unsafe extern "C" fn vfp_pull<T: Transfer>(
}

unsafe extern "C" fn wrap_event<S: Serve<T>, T: Transfer>(be: VCLBackendPtr, ev: ffi::vcl_event_e) {
let be = validate_director(be);
assert!(!be.priv_.is_null());
let backend = be.priv_ as *const S;

(*backend).event(Event::new(ev));
let backend: &S = validate_director(be).get_backend();
backend.event(Event::new(ev));
}

unsafe extern "C" fn wrap_list<S: Serve<T>, T: Transfer>(
Expand All @@ -343,40 +340,29 @@ unsafe extern "C" fn wrap_list<S: Serve<T>, T: Transfer>(
) {
let mut ctx = Ctx::from_ptr(ctxp);
let mut vsb = Vsb::new(vsbp);
let be = validate_director(be);
assert!(!be.priv_.is_null());
let backend = be.priv_ as *const S;

(*backend).list(&mut ctx, &mut vsb, detailed != 0, json != 0);
let backend: &S = validate_director(be).get_backend();
backend.list(&mut ctx, &mut vsb, detailed != 0, json != 0);
}

unsafe extern "C" fn wrap_panic<S: Serve<T>, T: Transfer>(be: VCLBackendPtr, vsbp: *mut ffi::vsb) {
let mut vsb = Vsb::new(vsbp);
let be = validate_director(be);
assert!(!be.priv_.is_null());
let backend = be.priv_ as *const S;

(*backend).panic(&mut vsb);
let backend: &S = validate_director(be).get_backend();
backend.panic(&mut vsb);
}

unsafe extern "C" fn wrap_pipe<S: Serve<T>, T: Transfer>(
ctxp: *const ffi::vrt_ctx,
be: VCLBackendPtr,
) -> ffi::stream_close_t {
let mut ctx = Ctx::from_ptr(ctxp);
assert!(!(*ctxp).req.is_null());
assert_eq!((*(*ctxp).req).magic, ffi::REQ_MAGIC);
assert!(!(*(*ctxp).req).sp.is_null());
assert_eq!((*(*(*ctxp).req).sp).magic, ffi::SESS_MAGIC);
let fd = (*(*(*ctxp).req).sp).fd;
let req = ctx.raw.validated_req();
let sp = req.validated_session();
let fd = sp.fd;
assert_ne!(fd, 0);
let tcp_stream = TcpStream::from_raw_fd(fd);

let be = validate_director(be);
assert!(!be.priv_.is_null());
let backend = be.priv_ as *const S;

sc_to_ptr((*backend).pipe(&mut ctx, tcp_stream))
let backend: &S = validate_director(be).get_backend();
sc_to_ptr(backend.pipe(&mut ctx, tcp_stream))
}

#[allow(clippy::too_many_lines)] // fixme
Expand All @@ -386,13 +372,11 @@ unsafe extern "C" fn wrap_gethdrs<S: Serve<T>, T: Transfer>(
) -> c_int {
let mut ctx = Ctx::from_ptr(ctxp);
let be = validate_director(be);
assert!(!be.vcl_name.is_null());
assert!(!be.priv_.is_null());
assert!(!be.vdir.is_null());
assert_eq!((*be.vdir).magic, ffi::VCLDIR_MAGIC);
let backend: &S = be.get_backend();
assert!(!be.vcl_name.is_null()); // FIXME: is this validation needed?
validate_vdir(be); // FIXME: is this validation needed?

let backend = be.priv_ as *const S;
match (*backend).get_headers(&mut ctx) {
match backend.get_headers(&mut ctx) {
Ok(res) => {
// default to HTTP/1.1 200 if the backend didn't provide anything
let beresp = ctx.http_beresp.as_mut().unwrap();
Expand All @@ -401,7 +385,7 @@ unsafe extern "C" fn wrap_gethdrs<S: Serve<T>, T: Transfer>(
}
if beresp.proto().is_none() {
if let Err(e) = beresp.set_proto("HTTP/1.1") {
ctx.fail(&format!("{}: {e}", (*backend).get_type()));
ctx.fail(&format!("{}: {e}", backend.get_type()));
return 1;
}
}
Expand All @@ -412,10 +396,7 @@ unsafe extern "C" fn wrap_gethdrs<S: Serve<T>, T: Transfer>(
)
.cast::<ffi::http_conn>();
if htc.is_null() {
ctx.fail(&format!(
"{}: insufficient workspace",
(*backend).get_type()
));
ctx.fail(&format!("{}: insufficient workspace", backend.get_type()));
return -1;
}
(*htc).magic = ffi::HTTP_CONN_MAGIC;
Expand Down Expand Up @@ -448,19 +429,13 @@ unsafe extern "C" fn wrap_gethdrs<S: Serve<T>, T: Transfer>(
)
.cast::<ffi::vfp>();
if vfp.is_null() {
ctx.fail(&format!(
"{}: insufficient workspace",
(*backend).get_type()
));
ctx.fail(&format!("{}: insufficient workspace", backend.get_type()));
return -1;
}
let Ok(t) = WS::new((*ctx.raw.bo).ws.as_mut_ptr())
.copy_bytes_with_null(&(*backend).get_type())
.copy_bytes_with_null(&backend.get_type())
else {
ctx.fail(&format!(
"{}: insufficient workspace",
(*backend).get_type()
));
ctx.fail(&format!("{}: insufficient workspace", backend.get_type()));
return -1;
};
(*vfp).name = t.as_ptr();
Expand All @@ -470,7 +445,7 @@ unsafe extern "C" fn wrap_gethdrs<S: Serve<T>, T: Transfer>(
(*vfp).priv1 = null();
let vfe = ffi::VFP_Push((*ctx.raw.bo).vfc, vfp);
if vfe.is_null() {
ctx.fail(&format!("{}: couldn't insert vfp", (*backend).get_type()));
ctx.fail(&format!("{}: couldn't insert vfp", backend.get_type()));
return -1;
}
// we don't need to clean (*vfe).priv1 at the vfp level, the backend will
Expand All @@ -486,7 +461,7 @@ unsafe extern "C" fn wrap_gethdrs<S: Serve<T>, T: Transfer>(
Err(s) => {
ctx.log(
LogTag::FetchError,
&format!("{}: {}", (*backend).get_type(), &s.to_string()),
&format!("{}: {}", backend.get_type(), &s.to_string()),
);
1
}
Expand All @@ -498,12 +473,10 @@ unsafe extern "C" fn wrap_healthy<S: Serve<T>, T: Transfer>(
be: ffi::VCL_BACKEND,
changed: *mut ffi::VCL_TIME,
) -> ffi::VCL_BOOL {
let be = validate_director(be);
assert!(!be.priv_.is_null());
let backend: &S = validate_director(be).get_backend();

let mut ctx = Ctx::from_ptr(ctxp);
let backend = be.priv_ as *const S;
let (healthy, when) = (*backend).healthy(&mut ctx);
let (healthy, when) = backend.healthy(&mut ctx);
if !changed.is_null() {
*changed = when.duration_since(UNIX_EPOCH).unwrap().as_secs_f64();
}
Expand Down Expand Up @@ -537,11 +510,11 @@ unsafe extern "C" fn wrap_finish<S: Serve<T>, T: Transfer>(
ctxp: *const ffi::vrt_ctx,
be: VCLBackendPtr,
) {
let be = validate_director(be);
let prev_backend = be.priv_.cast::<S>().as_ref().unwrap();
let prev_backend: &S = validate_director(be).get_backend();

let ctxp = ctxp.as_ref().unwrap();
let bo = ctxp.bo.as_mut().unwrap();
// FIXME: shouldn't the ctx magic number be checked? If so, use validate_vrt_ctx()
let ctx = ctxp.as_ref().unwrap();
let bo = ctx.bo.as_mut().unwrap();

// drop the Transfer
// FIXME: can htc be null? We do set it to null later...
Expand All @@ -552,7 +525,7 @@ unsafe extern "C" fn wrap_finish<S: Serve<T>, T: Transfer>(
bo.htc = null_mut();

// FIXME?: should _prev be set to NULL?
prev_backend.finish(&mut Ctx::from_ptr(ctxp));
prev_backend.finish(&mut Ctx::from_ptr(ctx));
}

impl<S: Serve<T>, T: Transfer> Drop for Backend<S, T> {
Expand Down
74 changes: 53 additions & 21 deletions src/vcl/utils.rs
Original file line number Diff line number Diff line change
@@ -1,33 +1,65 @@
use crate::ffi;
use crate::ffi::{vfp_entry, vrt_ctx};
use crate::vcl::backend::VCLBackendPtr;
use crate::ffi::{
director, req, sess, 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};

pub(crate) unsafe fn validate_vfp_ctx(ctxp: *mut ffi::vfp_ctx) -> &'static mut ffi::vfp_ctx {
let ctx = ctxp.as_mut().unwrap();
assert_eq!(ctx.magic, ffi::VFP_CTX_MAGIC);
ctx
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);
val
}

pub(crate) unsafe fn validate_vrt_ctx(ctxp: *const vrt_ctx) -> &'static vrt_ctx {
let ctxp = ctxp.as_ref().unwrap();
assert_eq!(ctxp.magic, ffi::VRT_CTX_MAGIC);
ctxp
let val = ctxp.as_ref().unwrap();
assert_eq!(val.magic, VRT_CTX_MAGIC);
val
}

pub(crate) unsafe fn validate_vfp_entry(vfep: *mut vfp_entry) -> &'static mut vfp_entry {
let vfe = vfep.as_mut().unwrap();
assert_eq!(vfe.magic, ffi::VFP_ENTRY_MAGIC);
vfe
let val = vfep.as_mut().unwrap();
assert_eq!(val.magic, VFP_ENTRY_MAGIC);
val
}

pub(crate) unsafe fn validate_director(be: VCLBackendPtr) -> &'static ffi::director {
let be = be.as_ref().unwrap();
assert_eq!(be.magic, ffi::DIRECTOR_MAGIC);
be
pub(crate) unsafe fn validate_director(be: VCLBackendPtr) -> &'static director {
let val = be.as_ref().unwrap();
assert_eq!(val.magic, DIRECTOR_MAGIC);
val
}

pub(crate) unsafe fn validate_ws(wsp: *mut ffi::ws) -> &'static mut ffi::ws {
let ws = wsp.as_mut().unwrap();
assert_eq!(ws.magic, ffi::WS_MAGIC);
ws
pub(crate) unsafe fn validate_ws(wsp: *mut ws) -> &'static mut ws {
let val = wsp.as_mut().unwrap();
assert_eq!(val.magic, WS_MAGIC);
val
}

pub(crate) unsafe fn validate_vdir(be: &director) -> &'static mut vcldir {
let val = be.vdir.as_mut().unwrap();
assert_eq!(val.magic, VCLDIR_MAGIC);
val
}

impl vrt_ctx {
pub fn validated_req(&mut self) -> &mut req {
let val = unsafe { self.req.as_mut().unwrap() };
assert_eq!(val.magic, REQ_MAGIC);
val
}
}

impl req {
pub fn validated_session(&mut self) -> &sess {
let val = unsafe { self.sp.as_ref().unwrap() };
assert_eq!(val.magic, SESS_MAGIC);
val
}
}

impl director {
/// Return the private pointer as a reference to the [`Serve`] object
/// FIXME: should it return a `&mut` instead?
pub fn get_backend<S: Serve<T>, T: Transfer>(&self) -> &S {
unsafe { self.priv_.cast::<S>().as_ref().unwrap() }
}
}

0 comments on commit ced832d

Please sign in to comment.