From e95e2b85492aa9883e0aee1a7105a915fcb8467e Mon Sep 17 00:00:00 2001 From: Evgeny Shirykalov Date: Tue, 16 Sep 2025 16:13:34 -0700 Subject: [PATCH 1/4] refactor: use NgxResult in handlers --- examples/async.rs | 20 +++++----- examples/awssig.rs | 10 ++--- examples/curl.rs | 4 +- examples/httporigdst.rs | 20 +++++----- src/core/pool.rs | 4 +- src/core/status.rs | 34 +++++++++++++++++ src/http/module.rs | 6 +-- src/http/request.rs | 85 +++++++++++++++++++++-------------------- src/http/status.rs | 14 ++++++- 9 files changed, 121 insertions(+), 76 deletions(-) diff --git a/examples/async.rs b/examples/async.rs index 47f5de8e..6ef22f6a 100644 --- a/examples/async.rs +++ b/examples/async.rs @@ -1,5 +1,5 @@ use std::ffi::{c_char, c_void}; -use std::ptr::{addr_of, addr_of_mut}; +use std::ptr::addr_of_mut; use std::sync::atomic::{AtomicBool, AtomicPtr, Ordering}; use std::sync::{Arc, OnceLock}; use std::time::Instant; @@ -145,24 +145,22 @@ http_request_handler!(async_access_handler, |request: &mut http::Request| { ngx_log_debug_http!(request, "async module enabled: {}", co.enable); if !co.enable { - return core::Status::NGX_DECLINED; + return core::Status::NGX_DECLINED.into(); } - if let Some(ctx) = - unsafe { request.get_module_ctx::(&*addr_of!(ngx_http_async_module)) } - { + if let Some(ctx) = request.get_module_ctx::(Module::module()) { if !ctx.done.load(Ordering::Relaxed) { - return core::Status::NGX_AGAIN; + return core::Status::NGX_AGAIN.into(); } - return core::Status::NGX_OK; + return core::Status::NGX_OK.into(); } - let ctx = request.pool().allocate(RequestCTX::default()); + let ctx = request.pool().alloc_with_cleanup(RequestCTX::default()); if ctx.is_null() { - return core::Status::NGX_ERROR; + return core::Status::NGX_ERROR.into(); } - request.set_module_ctx(ctx.cast(), unsafe { &*addr_of!(ngx_http_async_module) }); + request.set_module_ctx(ctx.cast(), Module::module()); let ctx = unsafe { &mut *ctx }; ctx.event.handler = Some(check_async_work_done); @@ -194,7 +192,7 @@ http_request_handler!(async_access_handler, |request: &mut http::Request| { // and use the same trick as the thread pool) })); - core::Status::NGX_AGAIN + core::Status::NGX_AGAIN.into() }); extern "C" fn ngx_http_async_commands_set_enable( diff --git a/examples/awssig.rs b/examples/awssig.rs index cbbea102..3607a8fb 100644 --- a/examples/awssig.rs +++ b/examples/awssig.rs @@ -272,7 +272,7 @@ http_request_handler!(awssigv4_header_handler, |request: &mut Request| { } }); if !conf.enable { - return core::Status::NGX_DECLINED; + return core::Status::NGX_DECLINED.into(); } // TODO: build url properly from the original URL from client @@ -284,7 +284,7 @@ http_request_handler!(awssigv4_header_handler, |request: &mut Request| { let datetime = chrono::Utc::now(); let uri = match request.unparsed_uri().to_str() { Ok(v) => format!("https://{}.{}{}", conf.s3_bucket, conf.s3_endpoint, v), - Err(_) => return core::Status::NGX_DECLINED, + Err(_) => return core::Status::NGX_DECLINED.into(), }; let datetime_now = datetime.format("%Y%m%dT%H%M%SZ"); @@ -301,11 +301,11 @@ http_request_handler!(awssigv4_header_handler, |request: &mut Request| { if let Ok(value) = http::HeaderValue::from_bytes(value.as_bytes()) { headers.insert(http::header::HOST, value); } else { - return core::Status::NGX_DECLINED; + return core::Status::NGX_DECLINED.into(); } } } else { - return core::Status::NGX_DECLINED; + return core::Status::NGX_DECLINED.into(); } } headers.insert("X-Amz-Date", datetime_now.parse().unwrap()); @@ -338,5 +338,5 @@ http_request_handler!(awssigv4_header_handler, |request: &mut Request| { ngx_log_debug_http!(request, "headers_in {name}: {value}",); } - core::Status::NGX_OK + core::Status::NGX_OK.into() }); diff --git a/examples/curl.rs b/examples/curl.rs index 3d07002f..ec76f4c3 100644 --- a/examples/curl.rs +++ b/examples/curl.rs @@ -103,10 +103,10 @@ http_request_handler!(curl_access_handler, |request: &mut http::Request| { { http::HTTPStatus::FORBIDDEN.into() } else { - core::Status::NGX_DECLINED + core::Status::NGX_DECLINED.into() } } - false => core::Status::NGX_DECLINED, + false => core::Status::NGX_DECLINED.into(), } }); diff --git a/examples/httporigdst.rs b/examples/httporigdst.rs index 75cc6527..c9989159 100644 --- a/examples/httporigdst.rs +++ b/examples/httporigdst.rs @@ -196,7 +196,7 @@ http_variable_get!( if let Some(obj) = ctx { ngx_log_debug_http!(request, "httporigdst: found context and binding variable",); obj.bind_addr(v); - return core::Status::NGX_OK; + return core::Status::NGX_OK.into(); } // lazy initialization: // get original dest information @@ -207,17 +207,17 @@ http_variable_get!( let r = ngx_get_origdst(request); match r { Err(e) => { - return e; + return e.into(); } Ok((ip, port)) => { // create context, // set context let new_ctx = request .pool() - .allocate::(Default::default()); + .alloc_with_cleanup::(Default::default()); if new_ctx.is_null() { - return core::Status::NGX_ERROR; + return core::Status::NGX_ERROR.into(); } ngx_log_debug_http!( @@ -232,7 +232,7 @@ http_variable_get!( .set_module_ctx(new_ctx as *mut c_void, &*addr_of!(ngx_http_orig_dst_module)); } } - core::Status::NGX_OK + core::Status::NGX_OK.into() } ); @@ -243,7 +243,7 @@ http_variable_get!( if let Some(obj) = ctx { ngx_log_debug_http!(request, "httporigdst: found context and binding variable",); obj.bind_port(v); - return core::Status::NGX_OK; + return core::Status::NGX_OK.into(); } // lazy initialization: // get original dest information @@ -254,17 +254,17 @@ http_variable_get!( let r = ngx_get_origdst(request); match r { Err(e) => { - return e; + return e.into(); } Ok((ip, port)) => { // create context, // set context let new_ctx = request .pool() - .allocate::(Default::default()); + .alloc_with_cleanup::(Default::default()); if new_ctx.is_null() { - return core::Status::NGX_ERROR; + return core::Status::NGX_ERROR.into(); } ngx_log_debug_http!( @@ -279,7 +279,7 @@ http_variable_get!( .set_module_ctx(new_ctx as *mut c_void, &*addr_of!(ngx_http_orig_dst_module)); } } - core::Status::NGX_OK + core::Status::NGX_OK.into() } ); diff --git a/src/core/pool.rs b/src/core/pool.rs index 685178cd..1274e367 100644 --- a/src/core/pool.rs +++ b/src/core/pool.rs @@ -272,7 +272,7 @@ impl Pool { /// /// Returns a typed pointer to the allocated memory if successful, or a null pointer if /// allocation or cleanup handler addition fails. - pub fn allocate(&self, value: T) -> *mut T { + pub fn alloc_with_cleanup(&self, value: T) -> *mut T { unsafe { let p = self.alloc(mem::size_of::()) as *mut T; ptr::write(p, value); @@ -308,7 +308,7 @@ impl Pool { Ok(NonNull::slice_from_raw_parts(ptr, new_layout.size())) } else { let size = core::cmp::min(old_layout.size(), new_layout.size()); - let new_ptr = ::allocate(self, new_layout)?; + let new_ptr = self.allocate(new_layout)?; unsafe { ptr.copy_to_nonoverlapping(new_ptr.cast(), size); self.deallocate(ptr, old_layout); diff --git a/src/core/status.rs b/src/core/status.rs index 4cab8844..6f7caadf 100644 --- a/src/core/status.rs +++ b/src/core/status.rs @@ -1,7 +1,10 @@ +use core::error::Error; use core::ffi::c_char; use core::fmt; use core::ptr; +use allocator_api2::alloc::AllocError; + use crate::ffi::*; /// Status @@ -68,3 +71,34 @@ ngx_codes! { pub const NGX_CONF_ERROR: *mut c_char = ptr::null_mut::().wrapping_offset(-1); /// Configuration handler succeeded. pub const NGX_CONF_OK: *mut c_char = ptr::null_mut(); + +/// Error type encapsulating normal NGINX return codes +#[derive(Debug)] +pub struct NgxError {} + +impl fmt::Display for NgxError { + fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { + write!(f, "NGINX Error") + } +} + +impl Error for NgxError {} + +impl From for NgxError { + fn from(_err: AllocError) -> Self { + NgxError {} + } +} + +/// Result type for NGINX status codes. +pub type NgxResult = core::result::Result; + +impl From for NgxResult { + fn from(val: Status) -> Self { + if val == Status::NGX_ERROR { + Err(NgxError {}) + } else { + Ok(val.0) + } + } +} diff --git a/src/http/module.rs b/src/http/module.rs index 14e019e2..c5c6222f 100644 --- a/src/http/module.rs +++ b/src/http/module.rs @@ -78,7 +78,7 @@ pub trait HttpModule { Self::MainConf: Default, { let pool = Pool::from_ngx_pool((*cf).pool); - pool.allocate::(Default::default()) as *mut c_void + pool.alloc_with_cleanup::(Default::default()) as *mut c_void } /// # Safety @@ -103,7 +103,7 @@ pub trait HttpModule { Self::ServerConf: Default, { let pool = Pool::from_ngx_pool((*cf).pool); - pool.allocate::(Default::default()) as *mut c_void + pool.alloc_with_cleanup::(Default::default()) as *mut c_void } /// # Safety @@ -137,7 +137,7 @@ pub trait HttpModule { Self::LocationConf: Default, { let pool = Pool::from_ngx_pool((*cf).pool); - pool.allocate::(Default::default()) as *mut c_void + pool.alloc_with_cleanup::(Default::default()) as *mut c_void } /// # Safety diff --git a/src/http/request.rs b/src/http/request.rs index 3671717c..9ff3e2d1 100644 --- a/src/http/request.rs +++ b/src/http/request.rs @@ -5,27 +5,29 @@ use core::ptr::NonNull; use core::slice; use core::str::FromStr; +use allocator_api2::alloc::Allocator; + use crate::core::*; use crate::ffi::*; use crate::http::status::*; /// Define a static request handler. /// -/// Handlers are expected to take a single [`Request`] argument and return a [`Status`]. +/// Handlers are expected to take a single [`Request`] argument and return a [`NgxResult`]. #[macro_export] macro_rules! http_request_handler { ( $name: ident, $handler: expr ) => { extern "C" fn $name(r: *mut $crate::ffi::ngx_http_request_t) -> $crate::ffi::ngx_int_t { - let status: $crate::core::Status = + let res: $crate::core::NgxResult = $handler(unsafe { &mut $crate::http::Request::from_ngx_http_request(r) }); - status.0 + res.unwrap_or_else(|_| $crate::core::Status::NGX_ERROR.into()) } }; } /// Define a static post subrequest handler. /// -/// Handlers are expected to take a single [`Request`] argument and return a [`Status`]. +/// Handlers are expected to take a single [`Request`] argument and return a [`NgxResult`]. #[macro_export] macro_rules! http_subrequest_handler { ( $name: ident, $handler: expr ) => { @@ -34,7 +36,8 @@ macro_rules! http_subrequest_handler { data: *mut ::core::ffi::c_void, rc: $crate::ffi::ngx_int_t, ) -> $crate::ffi::ngx_int_t { - $handler(r, data, rc) + let res: $crate::core::NgxResult = $handler(r, data, rc); + res.unwrap_or_else(|_| $crate::core::Status::NGX_ERROR.into()) } }; } @@ -64,8 +67,8 @@ macro_rules! http_variable_set { /// Define a static variable evaluator. /// /// The get handler is responsible for evaluating a variable in the context of a specific request. -/// Variable evaluators accept a [`Request`] input argument and two output -/// arguments: [`ngx_variable_value_t`] and [`usize`]. +/// Variable evaluators accept a [`Request`] and [`usize`] as input arguments and +/// [`ngx_variable_value_t`] as output argument. /// Variables: #[macro_export] macro_rules! http_variable_get { @@ -75,12 +78,12 @@ macro_rules! http_variable_get { v: *mut $crate::ffi::ngx_variable_value_t, data: usize, ) -> $crate::ffi::ngx_int_t { - let status: $crate::core::Status = $handler( + let res: $crate::core::NgxResult = $handler( unsafe { &mut $crate::http::Request::from_ngx_http_request(r) }, v, data, ); - status.0 + res.unwrap_or_else(|_| $crate::core::Status::NGX_ERROR.into()) } }; } @@ -127,6 +130,11 @@ impl Request { &mut *r.cast::() } + /// Get the raw pointer to the underlying [`ngx_http_request_t`]. + pub fn as_ptr(&self) -> *mut ngx_http_request_t { + &self.0 as *const _ as *mut _ + } + /// Is this the main request (as opposed to a subrequest)? pub fn is_main(&self) -> bool { let main = self.0.main.cast(); @@ -164,6 +172,7 @@ impl Request { /// /// [`ngx_log_t`]: https://nginx.org/en/docs/dev/development_guide.html#logging pub fn log(&self) -> *mut ngx_log_t { + // SAFETY: valid request always contains non-NULL connection pointer unsafe { (*self.connection()).log } } @@ -289,30 +298,26 @@ impl Request { /// Set the `last_buf` flag in the last body buffer. /// /// [response body]: https://nginx.org/en/docs/dev/development_guide.html#http_request_body - pub fn output_filter(&mut self, body: &mut ngx_chain_t) -> Status { - unsafe { Status(ngx_http_output_filter(&mut self.0, body)) } + pub fn output_filter(&mut self, body: &mut ngx_chain_t) -> NgxResult { + unsafe { Status(ngx_http_output_filter(&mut self.0, body)) }.into() } /// Perform internal redirect to a location - pub fn internal_redirect(&self, location: &str) -> Status { + pub fn internal_redirect(&self, location: &str) -> NgxResult { assert!(!location.is_empty(), "uri location is empty"); let uri_ptr = unsafe { &mut ngx_str_t::from_str(self.0.pool, location) as *mut _ }; // FIXME: check status of ngx_http_named_location or ngx_http_internal_redirect if location.starts_with('@') { unsafe { - ngx_http_named_location((self as *const Request as *mut Request).cast(), uri_ptr); + ngx_http_named_location(self.as_ptr(), uri_ptr); } } else { unsafe { - ngx_http_internal_redirect( - (self as *const Request as *mut Request).cast(), - uri_ptr, - core::ptr::null_mut(), - ); + ngx_http_internal_redirect(self.as_ptr(), uri_ptr, core::ptr::null_mut()); } } - Status::NGX_DONE + Status::NGX_DONE.into() } /// Send a subrequest @@ -325,53 +330,51 @@ impl Request { *mut c_void, ngx_int_t, ) -> ngx_int_t, - ) -> Status { + ) -> NgxResult { let uri_ptr = unsafe { &mut ngx_str_t::from_str(self.0.pool, uri) as *mut _ }; // ------------- // allocate memory and set values for ngx_http_post_subrequest_t - let sub_ptr = self + let post_subreq = self .pool() - .alloc(core::mem::size_of::()); + .allocate(core::alloc::Layout::new::())? + .as_ptr() as *mut ngx_http_post_subrequest_t; - // assert!(sub_ptr.is_null()); - let post_subreq = - sub_ptr as *const ngx_http_post_subrequest_t as *mut ngx_http_post_subrequest_t; + // SAFETY: parent request context must be set already. unsafe { (*post_subreq).handler = Some(post_callback); - (*post_subreq).data = self.get_module_ctx_ptr(module); // WARN: safety! ensure that ctx - // is already set + (*post_subreq).data = self.get_module_ctx_ptr(module); } // ------------- let mut psr: *mut ngx_http_request_t = core::ptr::null_mut(); - let r = unsafe { + if unsafe { ngx_http_subrequest( - (self as *const Request as *mut Request).cast(), + self.as_ptr(), uri_ptr, core::ptr::null_mut(), &mut psr as *mut _, - sub_ptr as *mut _, + post_subreq as *mut _, NGX_HTTP_SUBREQUEST_WAITED as _, - ) - }; + ) != NGX_OK as ngx_int_t + } { + return Status::NGX_ERROR.into(); + } - // previously call of ngx_http_subrequest() would ensure that the pointer is not null - // anymore + // SAFETY: previous call of ngx_http_subrequest() would ensure that the pointer is not null let sr = unsafe { &mut *psr }; /* * allocate fake request body to avoid attempts to read it and to make * sure real body file (if already read) won't be closed by upstream */ - sr.request_body = - self.pool() - .alloc(core::mem::size_of::()) as *mut _; + sr.request_body = self + .pool() + .allocate(core::alloc::Layout::new::())? + .as_ptr() as *mut _; - if sr.request_body.is_null() { - return Status::NGX_ERROR; - } sr.set_header_only(1 as _); - Status(r) + + Ok(NGX_OK as ngx_int_t) } /// Iterate over headers_in diff --git a/src/http/status.rs b/src/http/status.rs index 8b067cb6..3eb5b90f 100644 --- a/src/http/status.rs +++ b/src/http/status.rs @@ -1,6 +1,6 @@ use core::{error, fmt}; -use crate::core::Status; +use crate::core::{NgxResult, Status}; use crate::ffi::*; /// Represents an HTTP status code. @@ -36,6 +36,16 @@ impl From for Status { } } +impl From for NgxResult { + fn from(val: HTTPStatus) -> Self { + if (100..599).contains(&val.0) { + Ok(val.0 as ngx_int_t) + } else { + Err(crate::core::NgxError {}) + } + } +} + impl From for ngx_uint_t { fn from(val: HTTPStatus) -> Self { val.0 @@ -49,7 +59,7 @@ impl fmt::Debug for HTTPStatus { } impl HTTPStatus { - /// Convets a u16 to a status code. + /// Converts a u16 to a status code. #[inline] pub fn from_u16(src: u16) -> Result { if !(100..600).contains(&src) { From 7f6e4e642a880629d29ac5bd788b1ca31668c2ab Mon Sep 17 00:00:00 2001 From: Evgeny Shirykalov Date: Thu, 18 Sep 2025 11:02:44 -0700 Subject: [PATCH 2/4] refactor: overlook pool methods --- examples/httporigdst.rs | 30 ++++------------- examples/shared_dict.rs | 8 ++--- examples/upstream.rs | 49 ++++++++++++---------------- src/core/pool.rs | 72 +++++++++++++++++------------------------ src/http/upstream.rs | 4 +-- 5 files changed, 61 insertions(+), 102 deletions(-) diff --git a/examples/httporigdst.rs b/examples/httporigdst.rs index c9989159..1c5ee4ce 100644 --- a/examples/httporigdst.rs +++ b/examples/httporigdst.rs @@ -19,31 +19,13 @@ struct NgxHttpOrigDstCtx { } impl NgxHttpOrigDstCtx { - pub fn save(&mut self, addr: &str, port: in_port_t, pool: &core::Pool) -> core::Status { - let addr_data = pool.alloc_unaligned(addr.len()); - if addr_data.is_null() { - return core::Status::NGX_ERROR; - } - unsafe { libc::memcpy(addr_data, addr.as_ptr() as *const c_void, addr.len()) }; - self.orig_dst_addr.len = addr.len(); - self.orig_dst_addr.data = addr_data as *mut u8; + pub fn save(&mut self, addr: &str, port: in_port_t, pool: &core::Pool) -> core::NgxResult { + self.orig_dst_addr = unsafe { ngx_str_t::from_str(pool.as_ptr(), addr) }; let port_str = port.to_string(); - let port_data = pool.alloc_unaligned(port_str.len()); - if port_data.is_null() { - return core::Status::NGX_ERROR; - } - unsafe { - libc::memcpy( - port_data, - port_str.as_bytes().as_ptr() as *const c_void, - port_str.len(), - ) - }; - self.orig_dst_port.len = port_str.len(); - self.orig_dst_port.data = port_data as *mut u8; + self.orig_dst_port = unsafe { ngx_str_t::from_str(pool.as_ptr(), &port_str) }; - core::Status::NGX_OK + Ok(core::Status::NGX_OK.into()) } pub unsafe fn bind_addr(&self, v: *mut ngx_variable_value_t) { @@ -226,7 +208,7 @@ http_variable_get!( ip, port, ); - (*new_ctx).save(&ip, port, &request.pool()); + (*new_ctx).save(&ip, port, &request.pool())?; (*new_ctx).bind_addr(v); request .set_module_ctx(new_ctx as *mut c_void, &*addr_of!(ngx_http_orig_dst_module)); @@ -273,7 +255,7 @@ http_variable_get!( ip, port, ); - (*new_ctx).save(&ip, port, &request.pool()); + (*new_ctx).save(&ip, port, &request.pool())?; (*new_ctx).bind_port(v); request .set_module_ctx(new_ctx as *mut c_void, &*addr_of!(ngx_http_orig_dst_module)); diff --git a/examples/shared_dict.rs b/examples/shared_dict.rs index 1981e1aa..190daba7 100644 --- a/examples/shared_dict.rs +++ b/examples/shared_dict.rs @@ -200,10 +200,10 @@ extern "C" fn ngx_http_shared_dict_add_variable( let cf = unsafe { cf.as_mut().unwrap() }; let pool = unsafe { Pool::from_ngx_pool(cf.pool) }; - let key = pool.calloc_type::(); - if key.is_null() { - return NGX_CONF_ERROR; - } + let key = match pool.allocate_type_zeroed::() { + Ok(p) => p.as_ptr(), + Err(_) => return NGX_CONF_ERROR, + }; // SAFETY: // - `cf.args` is guaranteed to be a pointer to an array with 3 elements (NGX_CONF_TAKE2). diff --git a/examples/upstream.rs b/examples/upstream.rs index 5ea2c81b..57bc7b0f 100644 --- a/examples/upstream.rs +++ b/examples/upstream.rs @@ -120,44 +120,34 @@ http_upstream_init_peer_pt!( |request: &mut Request, us: *mut ngx_http_upstream_srv_conf_t| { ngx_log_debug_http!(request, "CUSTOM UPSTREAM request peer init"); - let hcpd = request.pool().alloc_type::(); - if hcpd.is_null() { - return Status::NGX_ERROR; - } + let hcpd = request.pool().allocate_type::()?.as_ptr(); // SAFETY: this function is called with non-NULL uf always let us = unsafe { &mut *us }; - let hccf = match Module::server_conf(us) { - Some(x) => x, - None => return Status::NGX_ERROR, - }; + let hccf = Module::server_conf(us).ok_or(ngx::core::NgxError {})?; let original_init_peer = hccf.original_init_peer.unwrap(); if unsafe { original_init_peer(request.into(), us) != Status::NGX_OK.into() } { - return Status::NGX_ERROR; + return Status::NGX_ERROR.into(); } - let maybe_upstream = request.upstream(); - if maybe_upstream.is_none() { - return Status::NGX_ERROR; - } - let upstream_ptr = maybe_upstream.unwrap(); + let upstream_ptr = request.upstream().ok_or(ngx::core::NgxError {})?; unsafe { (*hcpd).conf = Some(hccf); - (*hcpd).upstream = maybe_upstream; + (*hcpd).upstream = Some(upstream_ptr); (*hcpd).data = (*upstream_ptr).peer.data; (*hcpd).client_connection = Some(request.connection()); (*hcpd).original_get_peer = (*upstream_ptr).peer.get; (*hcpd).original_free_peer = (*upstream_ptr).peer.free; - (*upstream_ptr).peer.data = hcpd as *mut c_void; + (*upstream_ptr).peer.data = hcpd as _; (*upstream_ptr).peer.get = Some(ngx_http_upstream_get_custom_peer); (*upstream_ptr).peer.free = Some(ngx_http_upstream_free_custom_peer); } ngx_log_debug_http!(request, "CUSTOM UPSTREAM end request peer init"); - Status::NGX_OK + Status::NGX_OK.into() } ); @@ -312,24 +302,25 @@ impl HttpModule for Module { unsafe extern "C" fn create_srv_conf(cf: *mut ngx_conf_t) -> *mut c_void { let pool = Pool::from_ngx_pool((*cf).pool); - let conf = pool.alloc_type::(); - if conf.is_null() { + + if let Ok(mut conf) = pool.allocate_type::() { + unsafe { + conf.as_mut().max = NGX_CONF_UNSET as u32; + } + ngx_log_debug_mask!( + DebugMask::Http, + (*cf).log, + "CUSTOM UPSTREAM end create_srv_conf" + ); + conf.as_ptr() as _ + } else { ngx_conf_log_error!( NGX_LOG_EMERG, cf, "CUSTOM UPSTREAM could not allocate memory for config" ); - return std::ptr::null_mut(); + std::ptr::null_mut() } - - (*conf).max = NGX_CONF_UNSET as u32; - - ngx_log_debug_mask!( - DebugMask::Http, - (*cf).log, - "CUSTOM UPSTREAM end create_srv_conf" - ); - conf as *mut c_void } } diff --git a/src/core/pool.rs b/src/core/pool.rs index 1274e367..8d0d809c 100644 --- a/src/core/pool.rs +++ b/src/core/pool.rs @@ -4,7 +4,7 @@ use core::mem; use core::ptr::{self, NonNull}; use nginx_sys::{ - ngx_buf_t, ngx_create_temp_buf, ngx_palloc, ngx_pcalloc, ngx_pfree, ngx_pmemalign, ngx_pnalloc, + ngx_buf_t, ngx_create_temp_buf, ngx_palloc, ngx_pfree, ngx_pmemalign, ngx_pnalloc, ngx_pool_cleanup_add, ngx_pool_t, NGX_ALIGNMENT, }; @@ -183,10 +183,7 @@ impl Pool { /// Returns `Some(MemoryBuffer)` if the buffer is successfully created, or `None` if allocation /// fails. pub fn create_buffer_from_static_str(&self, str: &'static str) -> Option { - let buf = self.calloc_type::(); - if buf.is_null() { - return None; - } + let buf = self.allocate(Layout::new::()).ok()?.as_ptr() as *mut ngx_buf_t; // We cast away const, but buffers with the memory flag are read-only let start = str.as_ptr() as *mut u8; @@ -229,44 +226,6 @@ impl Pool { unsafe { ngx_palloc(self.0.as_ptr(), size) } } - /// Allocates memory for a type from the pool. - /// The resulting pointer is aligned to a platform word size. - /// - /// Returns a typed pointer to the allocated memory. - pub fn alloc_type(&self) -> *mut T { - self.alloc(mem::size_of::()) as *mut T - } - - /// Allocates zeroed memory from the pool of the specified size. - /// The resulting pointer is aligned to a platform word size. - /// - /// Returns a raw pointer to the allocated memory. - pub fn calloc(&self, size: usize) -> *mut c_void { - unsafe { ngx_pcalloc(self.0.as_ptr(), size) } - } - - /// Allocates zeroed memory for a type from the pool. - /// The resulting pointer is aligned to a platform word size. - /// - /// Returns a typed pointer to the allocated memory. - pub fn calloc_type(&self) -> *mut T { - self.calloc(mem::size_of::()) as *mut T - } - - /// Allocates unaligned memory from the pool of the specified size. - /// - /// Returns a raw pointer to the allocated memory. - pub fn alloc_unaligned(&self, size: usize) -> *mut c_void { - unsafe { ngx_pnalloc(self.0.as_ptr(), size) } - } - - /// Allocates unaligned memory for a type from the pool. - /// - /// Returns a typed pointer to the allocated memory. - pub fn alloc_type_unaligned(&self) -> *mut T { - self.alloc_unaligned(mem::size_of::()) as *mut T - } - /// Allocates memory for a value of a specified type and adds a cleanup handler to the memory /// pool. /// @@ -284,6 +243,33 @@ impl Pool { } } + /// Allocates unaligned memory from the pool of the specified size. + /// + /// Returns Result::Ok with a typed pointer to the allocated memory if successful, + /// or Result::Err(AllocError) if allocation or cleanup handler addition fails. + pub fn allocate_unaligned(&self, size: usize) -> Result, AllocError> { + self.allocate(unsafe { Layout::from_size_align_unchecked(size, 1) }) + } + + /// Allocates memory for a type from the pool. + /// The resulting pointer is aligned to a platform word size. + /// + /// Returns Result::Ok with a typed pointer to the allocated memory if successful, + /// or Result::Err(AllocError) if allocation fails. + pub fn allocate_type(&self) -> Result, AllocError> { + self.allocate(Layout::new::()).map(|ptr| ptr.cast()) + } + + /// Allocates zeroed memory for a type from the pool. + /// The resulting pointer is aligned to a platform word size. + /// + /// Returns Result::Ok with a typed pointer to the allocated memory if successful, + /// or Result::Err(AllocError) if allocation fails. + pub fn allocate_type_zeroed(&self) -> Result, AllocError> { + self.allocate_zeroed(Layout::new::()) + .map(|ptr| ptr.cast()) + } + /// Resizes a memory allocation in place if possible. /// /// If resizing is requested for the last allocation in the pool, it may be diff --git a/src/http/upstream.rs b/src/http/upstream.rs index 22a0e7ed..e22a204d 100644 --- a/src/http/upstream.rs +++ b/src/http/upstream.rs @@ -16,11 +16,11 @@ macro_rules! http_upstream_init_peer_pt { r: *mut $crate::ffi::ngx_http_request_t, us: *mut $crate::ffi::ngx_http_upstream_srv_conf_t, ) -> $crate::ffi::ngx_int_t { - let status: $crate::core::Status = $handler( + let res: $crate::core::NgxResult = $handler( unsafe { &mut $crate::http::Request::from_ngx_http_request(r) }, us, ); - status.0 + res.map_or_else(|_| $crate::core::Status::NGX_ERROR.into(), |code| code) } }; } From 28c1fa0396193ea2f021f53adc7a74175d7460bb Mon Sep 17 00:00:00 2001 From: Evgeny Shirykalov Date: Mon, 22 Sep 2025 13:10:06 -0700 Subject: [PATCH 3/4] refactor: use Result for handler return type in variable getters and setters --- examples/httporigdst.rs | 45 +++--- examples/shared_dict.rs | 303 +++++++++++++++++----------------------- src/http/request.rs | 7 +- 3 files changed, 157 insertions(+), 198 deletions(-) diff --git a/examples/httporigdst.rs b/examples/httporigdst.rs index 1c5ee4ce..185849b3 100644 --- a/examples/httporigdst.rs +++ b/examples/httporigdst.rs @@ -1,5 +1,4 @@ -use std::ffi::{c_int, c_void}; -use std::ptr::addr_of; +use std::ffi::c_int; use ngx::core; use ngx::ffi::{ @@ -28,30 +27,30 @@ impl NgxHttpOrigDstCtx { Ok(core::Status::NGX_OK.into()) } - pub unsafe fn bind_addr(&self, v: *mut ngx_variable_value_t) { + pub unsafe fn bind_addr(&self, v: &mut ngx_variable_value_t) { if self.orig_dst_addr.len == 0 { - (*v).set_not_found(1); + v.set_not_found(1); return; } - (*v).set_valid(1); - (*v).set_no_cacheable(0); - (*v).set_not_found(0); - (*v).set_len(self.orig_dst_addr.len as u32); - (*v).data = self.orig_dst_addr.data; + v.set_valid(1); + v.set_no_cacheable(0); + v.set_not_found(0); + v.set_len(self.orig_dst_addr.len as u32); + v.data = self.orig_dst_addr.data; } - pub unsafe fn bind_port(&self, v: *mut ngx_variable_value_t) { + pub unsafe fn bind_port(&self, v: &mut ngx_variable_value_t) { if self.orig_dst_port.len == 0 { - (*v).set_not_found(1); + v.set_not_found(1); return; } - (*v).set_valid(1); - (*v).set_no_cacheable(0); - (*v).set_not_found(0); - (*v).set_len(self.orig_dst_port.len as u32); - (*v).data = self.orig_dst_port.data; + v.set_valid(1); + v.set_no_cacheable(0); + v.set_not_found(0); + v.set_len(self.orig_dst_port.len as u32); + v.data = self.orig_dst_port.data; } } @@ -173,8 +172,8 @@ unsafe fn ngx_get_origdst( http_variable_get!( ngx_http_orig_dst_addr_variable, - |request: &mut http::Request, v: *mut ngx_variable_value_t, _: usize| { - let ctx = request.get_module_ctx::(&*addr_of!(ngx_http_orig_dst_module)); + |request: &mut http::Request, v: &mut ngx_variable_value_t, _: usize| { + let ctx = request.get_module_ctx::(Module::module()); if let Some(obj) = ctx { ngx_log_debug_http!(request, "httporigdst: found context and binding variable",); obj.bind_addr(v); @@ -210,8 +209,7 @@ http_variable_get!( ); (*new_ctx).save(&ip, port, &request.pool())?; (*new_ctx).bind_addr(v); - request - .set_module_ctx(new_ctx as *mut c_void, &*addr_of!(ngx_http_orig_dst_module)); + request.set_module_ctx(new_ctx as _, Module::module()); } } core::Status::NGX_OK.into() @@ -220,8 +218,8 @@ http_variable_get!( http_variable_get!( ngx_http_orig_dst_port_variable, - |request: &mut http::Request, v: *mut ngx_variable_value_t, _: usize| { - let ctx = request.get_module_ctx::(&*addr_of!(ngx_http_orig_dst_module)); + |request: &mut http::Request, v: &mut ngx_variable_value_t, _: usize| { + let ctx = request.get_module_ctx::(Module::module()); if let Some(obj) = ctx { ngx_log_debug_http!(request, "httporigdst: found context and binding variable",); obj.bind_port(v); @@ -257,8 +255,7 @@ http_variable_get!( ); (*new_ctx).save(&ip, port, &request.pool())?; (*new_ctx).bind_port(v); - request - .set_module_ctx(new_ctx as *mut c_void, &*addr_of!(ngx_http_orig_dst_module)); + request.set_module_ctx(new_ctx as _, Module::module()); } } core::Status::NGX_OK.into() diff --git a/examples/shared_dict.rs b/examples/shared_dict.rs index 190daba7..c374402b 100644 --- a/examples/shared_dict.rs +++ b/examples/shared_dict.rs @@ -4,16 +4,15 @@ use ::core::{mem, ptr}; use nginx_sys::{ ngx_command_t, ngx_conf_t, ngx_http_add_variable, ngx_http_compile_complex_value_t, - ngx_http_complex_value, ngx_http_complex_value_t, ngx_http_module_t, ngx_http_request_t, - ngx_http_variable_t, ngx_http_variable_value_t, ngx_int_t, ngx_module_t, ngx_parse_size, - ngx_shared_memory_add, ngx_shm_zone_t, ngx_str_t, ngx_uint_t, NGX_CONF_TAKE2, NGX_HTTP_DELETE, - NGX_HTTP_MAIN_CONF, NGX_HTTP_MAIN_CONF_OFFSET, NGX_HTTP_MODULE, NGX_HTTP_VAR_CHANGEABLE, - NGX_HTTP_VAR_NOCACHEABLE, NGX_LOG_EMERG, + ngx_http_complex_value_t, ngx_http_module_t, ngx_http_variable_t, ngx_int_t, ngx_module_t, + ngx_parse_size, ngx_shared_memory_add, ngx_shm_zone_t, ngx_str_t, ngx_uint_t, + ngx_variable_value_t, NGX_CONF_TAKE2, NGX_HTTP_MAIN_CONF, NGX_HTTP_MAIN_CONF_OFFSET, + NGX_HTTP_MODULE, NGX_HTTP_VAR_CHANGEABLE, NGX_HTTP_VAR_NOCACHEABLE, NGX_LOG_EMERG, }; use ngx::collections::RbTreeMap; -use ngx::core::{NgxStr, NgxString, Pool, SlabPool, Status, NGX_CONF_ERROR, NGX_CONF_OK}; -use ngx::http::{HttpModule, HttpModuleMainConf}; -use ngx::{ngx_conf_log_error, ngx_log_debug, ngx_string}; +use ngx::core::{NgxError, NgxStr, NgxString, Pool, SlabPool, Status, NGX_CONF_ERROR, NGX_CONF_OK}; +use ngx::http::{HttpModule, HttpModuleMainConf, Request}; +use ngx::{http_variable_get, http_variable_set, ngx_conf_log_error, ngx_log_debug, ngx_string}; struct HttpSharedDictModule; @@ -154,19 +153,16 @@ extern "C" fn ngx_http_shared_dict_add_zone( NGX_CONF_OK } -fn ngx_http_shared_dict_get_shared(shm_zone: &mut ngx_shm_zone_t) -> Result<&SharedData, Status> { - let mut alloc = unsafe { SlabPool::from_shm_zone(shm_zone) }.ok_or(Status::NGX_ERROR)?; +fn ngx_http_shared_dict_get_shared(shm_zone: &mut ngx_shm_zone_t) -> Result<&SharedData, NgxError> { + let mut alloc = unsafe { SlabPool::from_shm_zone(shm_zone) }.ok_or(NgxError {})?; if alloc.as_mut().data.is_null() { let shared: RbTreeMap, NgxString, SlabPool> = - RbTreeMap::try_new_in(alloc.clone()).map_err(|_| Status::NGX_ERROR)?; + RbTreeMap::try_new_in(alloc.clone())?; let shared = ngx::sync::RwLock::new(shared); - alloc.as_mut().data = ngx::allocator::allocate(shared, &alloc) - .map_err(|_| Status::NGX_ERROR)? - .as_ptr() - .cast(); + alloc.as_mut().data = ngx::allocator::allocate(shared, &alloc)?.as_ptr().cast(); } unsafe { @@ -175,7 +171,7 @@ fn ngx_http_shared_dict_get_shared(shm_zone: &mut ngx_shm_zone_t) -> Result<&Sha .data .cast::() .as_ref() - .ok_or(Status::NGX_ERROR) + .ok_or(NgxError {}) } } @@ -185,10 +181,9 @@ extern "C" fn ngx_http_shared_dict_zone_init( ) -> ngx_int_t { let shm_zone = unsafe { &mut *shm_zone }; - match ngx_http_shared_dict_get_shared(shm_zone) { - Err(e) => e.into(), - Ok(_) => Status::NGX_OK.into(), - } + ngx_http_shared_dict_get_shared(shm_zone) + .map_or_else(|_| Status::NGX_ERROR, |_| Status::NGX_OK) + .into() } extern "C" fn ngx_http_shared_dict_add_variable( @@ -250,191 +245,157 @@ extern "C" fn ngx_http_shared_dict_add_variable( NGX_CONF_OK } -extern "C" fn ngx_http_shared_dict_get_variable( - r: *mut ngx_http_request_t, - v: *mut ngx_http_variable_value_t, - data: usize, -) -> ngx_int_t { - let r = unsafe { &mut *r }; - let v = unsafe { &mut *v }; - let smcf = HttpSharedDictModule::main_conf_mut(r).expect("shared dict main config"); - - let mut key = ngx_str_t::empty(); - if unsafe { ngx_http_complex_value(r, data as _, &mut key) } != Status::NGX_OK.into() { - return Status::NGX_ERROR.into(); - } - - let key = unsafe { NgxStr::from_ngx_str(key) }; - - let Ok(shared) = ngx_http_shared_dict_get_shared(unsafe { &mut *smcf.shm_zone }) else { - return Status::NGX_ERROR.into(); - }; - - let value = shared - .read() - .get(key) - .and_then(|x| unsafe { ngx_str_t::from_bytes(r.pool, x.as_bytes()) }); - - ngx_log_debug!( - unsafe { (*r.connection).log }, - "shared dict: get \"{}\" -> {:?} w:{} p:{}", - key, - value.as_ref().map(|x| unsafe { NgxStr::from_ngx_str(*x) }), - unsafe { nginx_sys::ngx_worker }, - unsafe { nginx_sys::ngx_pid }, - ); - - let Some(value) = value else { - v.set_not_found(1); - return Status::NGX_ERROR.into(); - }; +http_variable_get!( + ngx_http_shared_dict_get_variable, + |r: &mut Request, v: &mut ngx_variable_value_t, data: usize| { + let smcf = HttpSharedDictModule::main_conf_mut(r).expect("shared dict main config"); - v.data = value.data; - v.set_len(value.len as _); + let key = r + .get_complex_value(&*(data as *mut ngx_http_complex_value_t)) + .ok_or(NgxError {})?; - v.set_valid(1); - v.set_no_cacheable(0); - v.set_not_found(0); + let shared = ngx_http_shared_dict_get_shared(unsafe { &mut *smcf.shm_zone })?; - Status::NGX_OK.into() -} - -extern "C" fn ngx_http_shared_dict_set_variable( - r: *mut ngx_http_request_t, - v: *mut ngx_http_variable_value_t, - data: usize, -) { - let r = unsafe { &mut *r }; - let v = unsafe { &mut *v }; - let smcf = HttpSharedDictModule::main_conf_mut(r).expect("shared dict main config"); - let mut key = ngx_str_t::empty(); - - if unsafe { ngx_http_complex_value(r, data as _, &mut key) } != Status::NGX_OK.into() { - return; - } - - let Ok(shared) = ngx_http_shared_dict_get_shared(unsafe { &mut *smcf.shm_zone }) else { - return; - }; - - if r.method == NGX_HTTP_DELETE as _ { - let key = unsafe { NgxStr::from_ngx_str(key) }; + let value = shared + .read() + .get(key) + .and_then(|x| unsafe { ngx_str_t::from_bytes(r.as_ref().pool, x.as_bytes()) }); ngx_log_debug!( - unsafe { (*r.connection).log }, - "shared dict: delete \"{}\" w:{} p:{}", + unsafe { (*r.connection()).log }, + "shared dict: get \"{}\" -> {:?} w:{} p:{}", key, + value.as_ref().map(|x| unsafe { NgxStr::from_ngx_str(*x) }), unsafe { nginx_sys::ngx_worker }, unsafe { nginx_sys::ngx_pid }, ); - let _ = shared.write().remove(key); - } else { - let alloc = unsafe { SlabPool::from_shm_zone(&*smcf.shm_zone).expect("slab pool") }; - - let Ok(key) = NgxString::try_from_bytes_in(key.as_bytes(), alloc.clone()) else { - return; + let Some(value) = value else { + v.set_not_found(1); + return Status::NGX_ERROR.into(); }; - let Ok(value) = NgxString::try_from_bytes_in(v.as_bytes(), alloc.clone()) else { - return; - }; + v.data = value.data; + v.set_len(value.len as _); - ngx_log_debug!( - unsafe { (*r.connection).log }, - "shared dict: set \"{}\" -> \"{}\" w:{} p:{}", - key, - value, - unsafe { nginx_sys::ngx_worker }, - unsafe { nginx_sys::ngx_pid }, - ); + v.set_valid(1); + v.set_no_cacheable(0); + v.set_not_found(0); - let _ = shared.write().try_insert(key, value); + Status::NGX_OK.into() } -} +); + +http_variable_set!( + ngx_http_shared_dict_set_variable, + |r: &mut Request, v: &mut ngx_variable_value_t, data: usize| { + let smcf = HttpSharedDictModule::main_conf_mut(r).expect("shared dict main config"); + + let key = r + .get_complex_value(&*(data as *mut ngx_http_complex_value_t)) + .ok_or(NgxError {})?; + + let shared = ngx_http_shared_dict_get_shared(unsafe { &mut *smcf.shm_zone })?; + + if r.method() == ngx::http::Method::DELETE { + ngx_log_debug!( + unsafe { (*r.connection()).log }, + "shared dict: delete \"{}\" w:{} p:{}", + key, + unsafe { nginx_sys::ngx_worker }, + unsafe { nginx_sys::ngx_pid }, + ); + + let _ = shared.write().remove(key); + } else { + let alloc = unsafe { SlabPool::from_shm_zone(&*smcf.shm_zone).expect("slab pool") }; + + let key = + NgxString::try_from_bytes_in(key.as_bytes(), alloc.clone()).or(Err(NgxError {}))?; + + let value = + NgxString::try_from_bytes_in(v.as_bytes(), alloc.clone()).or(Err(NgxError {}))?; + + ngx_log_debug!( + unsafe { (*r.connection()).log }, + "shared dict: set \"{}\" -> \"{}\" w:{} p:{}", + key, + value, + unsafe { nginx_sys::ngx_worker }, + unsafe { nginx_sys::ngx_pid }, + ); + + let _ = shared.write().try_insert(key, value); + } + Ok(()) + } +); -extern "C" fn ngx_http_shared_dict_get_entries( - r: *mut ngx_http_request_t, - v: *mut ngx_http_variable_value_t, - _data: usize, -) -> ngx_int_t { - use core::fmt::Write; +http_variable_get!( + ngx_http_shared_dict_get_entries, + |r: &mut Request, v: &mut ngx_variable_value_t, _data: usize| { + use core::fmt::Write; - let r = unsafe { &mut *r }; - let v = unsafe { &mut *v }; - let pool = unsafe { Pool::from_ngx_pool(r.pool) }; - let smcf = HttpSharedDictModule::main_conf_mut(r).expect("shared dict main config"); + let smcf = HttpSharedDictModule::main_conf_mut(r).expect("shared dict main config"); - ngx_log_debug!( - unsafe { (*r.connection).log }, - "shared dict: get all entries" - ); + ngx_log_debug!( + unsafe { (*r.connection()).log }, + "shared dict: get all entries" + ); - let Ok(shared) = ngx_http_shared_dict_get_shared(unsafe { &mut *smcf.shm_zone }) else { - return Status::NGX_ERROR.into(); - }; + let shared = ngx_http_shared_dict_get_shared(unsafe { &mut *smcf.shm_zone })?; - let mut str = NgxString::new_in(pool); - { - let dict = shared.read(); + let mut str = NgxString::new_in(r.pool()); + { + let dict = shared.read(); - let mut len: usize = 0; - let mut values: usize = 0; + let mut len: usize = 0; + let mut values: usize = 0; - for (key, value) in dict.iter() { - len += key.len() + value.len() + b" = ; ".len(); - values += 1; - } + for (key, value) in dict.iter() { + len += key.len() + value.len() + b" = ; ".len(); + values += 1; + } - len += values.checked_ilog10().unwrap_or(0) as usize + b"0; ".len(); + len += values.checked_ilog10().unwrap_or(0) as usize + b"0; ".len(); - if str.try_reserve(len).is_err() { - return Status::NGX_ERROR.into(); - } + str.try_reserve(len).or(Err(NgxError {}))?; - if write!(str, "{values}; ").is_err() { - return Status::NGX_ERROR.into(); - } + write!(str, "{values}; ").or(Err(NgxError {}))?; - for (key, value) in dict.iter() { - if write!(str, "{key} = {value}; ").is_err() { - return Status::NGX_ERROR.into(); + for (key, value) in dict.iter() { + write!(str, "{key} = {value}; ").or(Err(NgxError {}))?; } } - } - // The string is allocated on the `ngx_pool_t` and will be freed with the request. - let (data, len, _, _) = str.into_raw_parts(); + // The string is allocated on the `ngx_pool_t` and will be freed with the request. + let (data, len, _, _) = str.into_raw_parts(); - v.data = data; - v.set_len(len as _); + v.data = data; + v.set_len(len as _); - v.set_valid(1); - v.set_no_cacheable(1); - v.set_not_found(0); + v.set_valid(1); + v.set_no_cacheable(1); + v.set_not_found(0); - Status::NGX_OK.into() -} + Status::NGX_OK.into() + } +); -extern "C" fn ngx_http_shared_dict_set_entries( - r: *mut ngx_http_request_t, - _v: *mut ngx_http_variable_value_t, - _data: usize, -) { - let r = unsafe { &mut *r }; - let smcf = HttpSharedDictModule::main_conf_mut(r).expect("shared dict main config"); +http_variable_set!( + ngx_http_shared_dict_set_entries, + |r: &mut Request, _v: &mut ngx_variable_value_t, _data: usize| { + let smcf = HttpSharedDictModule::main_conf_mut(r).expect("shared dict main config"); - ngx_log_debug!(unsafe { (*r.connection).log }, "shared dict: clear"); + ngx_log_debug!(unsafe { (*r.connection()).log }, "shared dict: clear"); - let Ok(shared) = ngx_http_shared_dict_get_shared(unsafe { &mut *smcf.shm_zone }) else { - return; - }; + let shared = ngx_http_shared_dict_get_shared(unsafe { &mut *smcf.shm_zone })?; - let Ok(tree) = RbTreeMap::try_new_in(shared.read().allocator().clone()) else { - return; - }; + let tree = RbTreeMap::try_new_in(shared.read().allocator().clone())?; - // This would check both .clear() and the drop implementation - *shared.write() = tree; - // shared.write().clear() -} + // This would check both .clear() and the drop implementation + *shared.write() = tree; + // shared.write().clear() + Ok(()) + } +); diff --git a/src/http/request.rs b/src/http/request.rs index 9ff3e2d1..2bb87133 100644 --- a/src/http/request.rs +++ b/src/http/request.rs @@ -55,9 +55,9 @@ macro_rules! http_variable_set { v: *mut $crate::ffi::ngx_variable_value_t, data: usize, ) { - $handler( + let _: $crate::core::NgxResult<()> = $handler( unsafe { &mut $crate::http::Request::from_ngx_http_request(r) }, - v, + ::core::ptr::NonNull::new(v).unwrap().as_mut(), data, ); } @@ -80,7 +80,8 @@ macro_rules! http_variable_get { ) -> $crate::ffi::ngx_int_t { let res: $crate::core::NgxResult = $handler( unsafe { &mut $crate::http::Request::from_ngx_http_request(r) }, - v, + // SAFETY: pointer to variable value is non-NULL and valid + ::core::ptr::NonNull::new(v).unwrap().as_mut(), data, ); res.unwrap_or_else(|_| $crate::core::Status::NGX_ERROR.into()) From 8fe93dbd8baa2b34f4a362e77cd06b55d8f80c9e Mon Sep 17 00:00:00 2001 From: Evgeny Shirykalov Date: Fri, 26 Sep 2025 11:10:23 -0700 Subject: [PATCH 4/4] feat: request handler trait --- examples/curl.rs | 60 +++++++++++++++++++------------------------ nginx-sys/src/http.rs | 27 +++++++++++++++++++ src/http/module.rs | 26 +++++++++++++++++-- src/http/request.rs | 45 ++++++++++++++++++++++++++++++++ 4 files changed, 123 insertions(+), 35 deletions(-) diff --git a/examples/curl.rs b/examples/curl.rs index ec76f4c3..f3fc46da 100644 --- a/examples/curl.rs +++ b/examples/curl.rs @@ -2,13 +2,12 @@ use std::ffi::{c_char, c_void}; use ngx::core; use ngx::ffi::{ - ngx_array_push, ngx_command_t, ngx_conf_t, ngx_http_handler_pt, ngx_http_module_t, - ngx_http_phases_NGX_HTTP_ACCESS_PHASE, ngx_int_t, ngx_module_t, ngx_str_t, ngx_uint_t, + ngx_command_t, ngx_conf_t, ngx_http_module_t, ngx_module_t, ngx_str_t, ngx_uint_t, NGX_CONF_TAKE1, NGX_HTTP_LOC_CONF, NGX_HTTP_LOC_CONF_OFFSET, NGX_HTTP_MODULE, NGX_LOG_EMERG, }; -use ngx::http::{self, HttpModule, MergeConfigError}; -use ngx::http::{HttpModuleLocationConf, HttpModuleMainConf, NgxHttpCoreModule}; -use ngx::{http_request_handler, ngx_conf_log_error, ngx_log_debug_http, ngx_string}; +use ngx::http::HttpModuleLocationConf; +use ngx::http::{self, HttpModule, MergeConfigError, RequestHandler}; +use ngx::{ngx_conf_log_error, ngx_log_debug_http, ngx_string}; struct Module; @@ -17,20 +16,8 @@ impl http::HttpModule for Module { unsafe { &*::core::ptr::addr_of!(ngx_http_curl_module) } } - unsafe extern "C" fn postconfiguration(cf: *mut ngx_conf_t) -> ngx_int_t { - // SAFETY: this function is called with non-NULL cf always - let cf = &mut *cf; - let cmcf = NgxHttpCoreModule::main_conf_mut(cf).expect("http core main conf"); - - let h = ngx_array_push( - &mut cmcf.phases[ngx_http_phases_NGX_HTTP_ACCESS_PHASE as usize].handlers, - ) as *mut ngx_http_handler_pt; - if h.is_null() { - return core::Status::NGX_ERROR.into(); - } - // set an Access phase handler - *h = Some(curl_access_handler); - core::Status::NGX_OK.into() + fn request_handlers() -> impl Iterator> { + ::core::iter::once(CurlRequestHandler) } } @@ -90,25 +77,32 @@ impl http::Merge for ModuleConfig { } } -http_request_handler!(curl_access_handler, |request: &mut http::Request| { - let co = Module::location_conf(request).expect("module config is none"); +struct CurlRequestHandler; + +impl RequestHandler for CurlRequestHandler { + const PHASE: nginx_sys::NgxHttpPhases = nginx_sys::NgxHttpPhases::Access; + type Module = Module; - ngx_log_debug_http!(request, "curl module enabled: {}", co.enable); + fn handler(request: &mut http::Request) -> ngx::core::NgxResult { + let co = Module::location_conf(request).expect("module config is none"); - match co.enable { - true => { - if request - .user_agent() - .is_some_and(|ua| ua.as_bytes().starts_with(b"curl")) - { - http::HTTPStatus::FORBIDDEN.into() - } else { - core::Status::NGX_DECLINED.into() + ngx_log_debug_http!(request, "curl module enabled: {}", co.enable); + + match co.enable { + true => { + if request + .user_agent() + .is_some_and(|ua| ua.as_bytes().starts_with(b"curl")) + { + http::HTTPStatus::FORBIDDEN.into() + } else { + core::Status::NGX_DECLINED.into() + } } + false => core::Status::NGX_DECLINED.into(), } - false => core::Status::NGX_DECLINED.into(), } -}); +} extern "C" fn ngx_http_curl_commands_set_enable( cf: *mut ngx_conf_t, diff --git a/nginx-sys/src/http.rs b/nginx-sys/src/http.rs index 435cc794..cd3c3b08 100644 --- a/nginx-sys/src/http.rs +++ b/nginx-sys/src/http.rs @@ -16,3 +16,30 @@ pub const NGX_HTTP_SRV_CONF_OFFSET: usize = offset_of!(ngx_http_conf_ctx_t, srv_ /// /// This is used to access the location configuration context for an HTTP module. pub const NGX_HTTP_LOC_CONF_OFFSET: usize = offset_of!(ngx_http_conf_ctx_t, loc_conf); + +/// HTTP phases in which a module can register handlers. +#[repr(u32)] +pub enum NgxHttpPhases { + /// Post-read phase + PostRead = crate::ngx_http_phases_NGX_HTTP_POST_READ_PHASE, + /// Server rewrite phase + ServerRewrite = crate::ngx_http_phases_NGX_HTTP_SERVER_REWRITE_PHASE, + /// Find configuration phase + FindConfig = crate::ngx_http_phases_NGX_HTTP_FIND_CONFIG_PHASE, + /// Rewrite phase + Rewrite = crate::ngx_http_phases_NGX_HTTP_REWRITE_PHASE, + /// Post-rewrite phase + PostRewrite = crate::ngx_http_phases_NGX_HTTP_POST_REWRITE_PHASE, + /// Pre-access phase + Preaccess = crate::ngx_http_phases_NGX_HTTP_PREACCESS_PHASE, + /// Access phase + Access = crate::ngx_http_phases_NGX_HTTP_ACCESS_PHASE, + /// Post-access phase + PostAccess = crate::ngx_http_phases_NGX_HTTP_POST_ACCESS_PHASE, + /// Pre-content phase + PreContent = crate::ngx_http_phases_NGX_HTTP_PRECONTENT_PHASE, + /// Content phase + Content = crate::ngx_http_phases_NGX_HTTP_CONTENT_PHASE, + /// Log phase + Log = crate::ngx_http_phases_NGX_HTTP_LOG_PHASE, +} diff --git a/src/http/module.rs b/src/http/module.rs index c5c6222f..11d79fb6 100644 --- a/src/http/module.rs +++ b/src/http/module.rs @@ -6,6 +6,7 @@ use core::ptr; use crate::core::NGX_CONF_ERROR; use crate::core::*; use crate::ffi::*; +use crate::http::RequestHandler; /// MergeConfigError - configuration cannot be merged with levels above. #[derive(Debug)] @@ -52,11 +53,24 @@ pub trait HttpModule { /// Returns reference to a global variable of type [ngx_module_t] created for this module. fn module() -> &'static ngx_module_t; + /// Returns an iterator over request handlers provided by this module. + fn request_handlers() -> impl Iterator> { + // returns empty iterator by default + core::iter::empty::>() + } + /// Register all request handlers provided by this module. + /// /// # Safety /// /// Callers should provide valid non-null `ngx_conf_t` arguments. Implementers must /// guard against null inputs or risk runtime errors. - unsafe extern "C" fn preconfiguration(_cf: *mut ngx_conf_t) -> ngx_int_t { + unsafe fn register_request_handlers(cf: *mut ngx_conf_t) -> ngx_int_t { + let cf = unsafe { &mut *cf }; + for rh in Self::request_handlers() { + if !rh.register(cf) { + return Status::NGX_ERROR.into(); + } + } Status::NGX_OK.into() } @@ -64,10 +78,18 @@ pub trait HttpModule { /// /// Callers should provide valid non-null `ngx_conf_t` arguments. Implementers must /// guard against null inputs or risk runtime errors. - unsafe extern "C" fn postconfiguration(_cf: *mut ngx_conf_t) -> ngx_int_t { + unsafe extern "C" fn preconfiguration(_cf: *mut ngx_conf_t) -> ngx_int_t { Status::NGX_OK.into() } + /// # Safety + /// + /// Callers should provide valid non-null `ngx_conf_t` arguments. Implementers must + /// guard against null inputs or risk runtime errors. + unsafe extern "C" fn postconfiguration(cf: *mut ngx_conf_t) -> ngx_int_t { + Self::register_request_handlers(cf) + } + /// # Safety /// /// Callers should provide valid non-null `ngx_conf_t` arguments. Implementers must diff --git a/src/http/request.rs b/src/http/request.rs index 2bb87133..72786178 100644 --- a/src/http/request.rs +++ b/src/http/request.rs @@ -1,6 +1,7 @@ use core::error; use core::ffi::c_void; use core::fmt; +use core::marker::PhantomData; use core::ptr::NonNull; use core::slice; use core::str::FromStr; @@ -10,6 +11,7 @@ use allocator_api2::alloc::Allocator; use crate::core::*; use crate::ffi::*; use crate::http::status::*; +use crate::http::{HttpModule, HttpModuleMainConf, NgxHttpCoreModule}; /// Define a static request handler. /// @@ -89,6 +91,49 @@ macro_rules! http_variable_get { }; } +/// Trait for static request handler. +pub trait RequestHandler { + /// The phase in which the handler is invoked + const PHASE: NgxHttpPhases; + /// The module to which the handler belongs + type Module: HttpModule + ?Sized; + /// The handler function + fn handler(request: &mut Request) -> NgxResult; + /// Wrapper for the handler function to match the C function signature + /// + /// # Safety + /// The caller must provide a valid non-null pointer to an `ngx_http_request_t` + unsafe extern "C" fn handler_wrapper(r: *mut ngx_http_request_t) -> ngx_int_t { + let res: NgxResult = Self::handler(unsafe { Request::from_ngx_http_request(r) }); + res.unwrap_or_else(|_| Status::NGX_ERROR.into()) + } + /// Register a request handler for a specified phase. + fn register(&self, cf: &mut ngx_conf_t) -> bool { + let cmcf = NgxHttpCoreModule::main_conf_mut(cf).expect("http core main conf"); + let h: *mut ngx_http_handler_pt = + unsafe { ngx_array_push(&mut cmcf.phases[Self::PHASE as usize].handlers) as _ }; + if h.is_null() { + return false; + } + // set an H::PHASE phase handler + unsafe { + *h = Some(Self::handler_wrapper); + } + true + } +} + +pub(crate) struct EmptyHandler(PhantomData); + +impl RequestHandler for EmptyHandler { + const PHASE: NgxHttpPhases = NgxHttpPhases::Access; + type Module = M; + + fn handler(_request: &mut Request) -> NgxResult { + Status::NGX_OK.into() + } +} + /// Wrapper struct for an [`ngx_http_request_t`] pointer, providing methods for working with HTTP /// requests. ///