diff --git a/src/ngx_module/config.rs b/src/ngx_module/config.rs index bed04b9..72f7ed2 100644 --- a/src/ngx_module/config.rs +++ b/src/ngx_module/config.rs @@ -8,6 +8,15 @@ use std::str::FromStr; use std::time::Duration; /// Module configuration (raw strings from Nginx config) +/// +/// # Memory Layout +/// +/// This struct must use C-compatible memory layout (`#[repr(C)]`) because: +/// 1. It's allocated by nginx using `ngx_pcalloc` (C memory allocator) +/// 2. It's accessed via raw pointers from nginx's configuration system +/// 3. Field order and padding must match what nginx expects +/// 4. Adding new fields (like `ttl_str`) must not break existing memory layout +#[repr(C)] #[derive(Clone, Default)] pub struct X402Config { pub enabled: ngx::ffi::ngx_flag_t, diff --git a/src/ngx_module/mod.rs b/src/ngx_module/mod.rs index 527dc97..4647eb4 100644 --- a/src/ngx_module/mod.rs +++ b/src/ngx_module/mod.rs @@ -101,18 +101,21 @@ pub unsafe extern "C" fn x402_metrics_handler( /// # Safety /// /// The caller must ensure that `r` is a valid pointer to a `ngx_http_request_t`. -unsafe fn clear_x402_content_handler(r: *mut ngx::ffi::ngx_http_request_t, reason: &str) { - use ngx::ffi::ngx_http_request_t; - let r_raw = r.cast::(); - if r_raw.is_null() { - return; - } +/// Clear x402 content handler if it's set +/// +/// Uses ngx-rust's safe `Request` API instead of raw pointers. +fn clear_x402_content_handler(req: &mut ngx::http::Request, reason: &str) { + use crate::ngx_module::logging::log_debug; extern "C" { fn x402_ngx_handler(r: *mut ngx::ffi::ngx_http_request_t) -> ngx::ffi::ngx_int_t; } + + // Use Request's as_mut() to access the underlying structure + // Safe: Request is a zero-cost wrapper, as_mut() returns a valid mutable reference + let r_raw = req.as_mut(); let x402_handler_fn: ngx::ffi::ngx_http_handler_pt = Some(x402_ngx_handler); - let current_handler = (*r_raw).content_handler; + let current_handler = r_raw.content_handler; // Check if content handler is x402_ngx_handler let is_x402_handler = if let (Some(current), Some(x402)) = (current_handler, x402_handler_fn) { @@ -123,8 +126,7 @@ unsafe fn clear_x402_content_handler(r: *mut ngx::ffi::ngx_http_request_t, reaso if is_x402_handler { // Clear content handler to prevent payment verification in CONTENT_PHASE - (*r_raw).content_handler = None; - use crate::ngx_module::logging::log_debug; + r_raw.content_handler = None; log_debug( None, &format!( @@ -167,13 +169,12 @@ pub unsafe extern "C" fn x402_phase_handler( || { // Create Request object safely using ngx-rust's API // Request is a zero-cost wrapper (repr(transparent)), so we can safely create it from the raw pointer + // Request implements DerefMut, so we can pass req_mut directly to functions expecting &mut Request let req_mut = ngx::http::Request::from_ngx_http_request(r); use crate::ngx_module::logging::log_debug; use crate::ngx_module::module::get_module_config; - use crate::ngx_module::request::{ - get_http_method, is_websocket_request, should_skip_payment_for_method, - }; + use crate::ngx_module::request::is_websocket_request; // Skip payment verification for special request types // These requests should bypass payment verification: @@ -190,9 +191,9 @@ pub unsafe extern "C" fn x402_phase_handler( // Check if HTTP method should skip payment verification // This check happens early to avoid unnecessary processing - let request_struct = unsafe { &*r }; - let method_id = request_struct.method; - let detected_method = unsafe { get_http_method(r) }; + // Use safe Request API instead of raw pointer access + let method_id = crate::ngx_module::request::get_http_method_id(req_mut); + let detected_method = crate::ngx_module::request::get_http_method(req_mut); log_debug( Some(req_mut), @@ -202,7 +203,7 @@ pub unsafe extern "C" fn x402_phase_handler( ), ); - if unsafe { should_skip_payment_for_method(r) } { + if crate::ngx_module::request::should_skip_payment_for_method(req_mut) { let method = detected_method.unwrap_or("UNKNOWN"); log_debug( Some(req_mut), @@ -214,12 +215,10 @@ pub unsafe extern "C" fn x402_phase_handler( // Clear content handler if it's x402_ngx_handler to prevent payment verification in CONTENT_PHASE // When returning NGX_DECLINED, nginx will still proceed to CONTENT_PHASE, so we need to clear // the content handler to prevent duplicate payment verification - unsafe { - clear_x402_content_handler( - r, - &format!("for {} request to prevent payment verification", method), - ); - } + clear_x402_content_handler( + req_mut, + &format!("for {} request to prevent payment verification", method), + ); return ngx::ffi::NGX_DECLINED as ngx::ffi::ngx_int_t; } @@ -230,63 +229,57 @@ pub unsafe extern "C" fn x402_phase_handler( "[x402] Phase handler: WebSocket upgrade detected, skipping payment verification", ); // Clear content handler if it's x402_ngx_handler to prevent payment verification in CONTENT_PHASE - unsafe { - clear_x402_content_handler( - r, - "for WebSocket request to prevent payment verification", - ); - } + clear_x402_content_handler( + req_mut, + "for WebSocket request to prevent payment verification", + ); return ngx::ffi::NGX_DECLINED as ngx::ffi::ngx_int_t; } // Check for subrequest using raw request pointer // Subrequests have r->parent != NULL - unsafe { - let r_raw = r.cast_const(); - if !r_raw.is_null() { - let parent = (*r_raw).parent; - if !parent.is_null() { - log_debug( - Some(req_mut), - "[x402] Phase handler: Subrequest detected (parent != NULL), skipping payment verification", - ); - return ngx::ffi::NGX_DECLINED as ngx::ffi::ngx_int_t; + // Use panic protection to catch any invalid memory access + use crate::ngx_module::panic_handler::catch_panic; + let is_subrequest = catch_panic( + || { + unsafe { + let r_raw = r.cast_const(); + if r_raw.is_null() { + return false; + } + // Access parent field - this may cause segfault if memory is invalid + let parent = (*r_raw).parent; + !parent.is_null() } - } + }, + "check subrequest (parent field)", + ) + .unwrap_or(false); + + if is_subrequest { + log_debug( + Some(req_mut), + "[x402] Phase handler: Subrequest detected (parent != NULL), skipping payment verification", + ); + return ngx::ffi::NGX_DECLINED as ngx::ffi::ngx_int_t; } - // Check for internal redirect using raw request pointer - // Internal redirects have r->internal = 1 (unsigned flag) - // In nginx C code, internal is a field in ngx_http_request_t structure - unsafe { - let r_raw = r.cast_const(); - if !r_raw.is_null() { - // Access internal field directly from C structure - // internal is an unsigned integer field in ngx_http_request_t - // We need to access it via pointer dereference - // Note: This is accessing the raw C structure, so we need to be careful - let request_struct = &*r_raw; - // Try to access internal field - it should be a field, not a method - // If this doesn't compile, we may need to use offset_of! macro or other approach - // For now, we'll use a workaround: check if uri.data starts with @ (named locations) - // Named locations (like @fallback) are always internal redirects - let uri = request_struct.uri; - if !uri.data.is_null() && uri.len > 0 { - // Check if URI starts with '@' which indicates named location (always internal) - let uri_slice = - std::slice::from_raw_parts(uri.data.cast_const(), uri.len.min(1)); - if uri_slice[0] == b'@' { - log_debug( - Some(req_mut), - "[x402] Phase handler: Internal redirect detected (named location @), skipping payment verification", - ); - return ngx::ffi::NGX_DECLINED as ngx::ffi::ngx_int_t; - } - } + // Check for internal redirect using safe Request API + // Internal redirects in nginx are typically named locations (starting with @) + // We can detect this by checking the request path using Request::path() + // This is safer than accessing raw C structure fields + if let Ok(path) = req_mut.path().to_str() { + if path.starts_with('@') { + log_debug( + Some(req_mut), + "[x402] Phase handler: Internal redirect detected (named location @), skipping payment verification", + ); + return ngx::ffi::NGX_DECLINED as ngx::ffi::ngx_int_t; } } // Check if module is enabled for this location + #[allow(clippy::question_mark)] // Need to return NGX_DECLINED on error, can't use ? let conf = match get_module_config(req_mut) { Ok(c) => c, Err(_) => { @@ -310,12 +303,10 @@ pub unsafe extern "C" fn x402_phase_handler( // If content handler is x402_ngx_handler (no proxy_pass), clear it to prevent // duplicate payment verification in CONTENT_PHASE. If content handler is something // else (like proxy_pass), keep it so it runs in CONTENT_PHASE. - unsafe { - clear_x402_content_handler( - r, - "after payment verification to prevent duplicate verification", - ); - } + clear_x402_content_handler( + req_mut, + "after payment verification to prevent duplicate verification", + ); // This will proceed to CONTENT_PHASE where proxy_pass handler will run (if set) ngx::ffi::NGX_OK as ngx::ffi::ngx_int_t } diff --git a/src/ngx_module/module.rs b/src/ngx_module/module.rs index a176739..e85f679 100644 --- a/src/ngx_module/module.rs +++ b/src/ngx_module/module.rs @@ -3,7 +3,9 @@ use crate::ngx_module::commands::ngx_http_x402_commands; use crate::ngx_module::config::X402Config; use crate::ngx_module::error::{ConfigError, Result}; -use ngx::ffi::ngx_http_core_main_conf_t; +use crate::ngx_module::panic_handler::catch_panic; +use ngx::core::NgxStr; +use ngx::ffi::{ngx_http_core_main_conf_t, ngx_str_t}; use ngx::http::Request; use std::ffi::c_char; use std::ptr; @@ -28,9 +30,145 @@ macro_rules! merge_string_field { }; } +/// Helper function to copy a string from config to request pool +/// +/// This function safely copies a string field from configuration to the request's +/// memory pool, ensuring the string lives as long as the request. +/// +/// Uses ngx-rust's safe `Request::pool()` method instead of accessing raw pointers. +/// +/// # Memory Safety +/// +/// This function uses panic protection to handle cases where `src.data` points to +/// invalid memory (e.g., if the configuration's memory pool was freed). If accessing +/// the string causes a panic, it's caught and returns None instead of crashing. +fn copy_string_to_request_pool(req: &Request, src: ngx_str_t) -> Option { + if src.len == 0 { + return Some(ngx_str_t { + len: 0, + data: ptr::null_mut(), + }); + } + + // Validate that src.data is not null and points to valid memory + if src.data.is_null() { + return None; + } + + // Use panic protection to handle invalid memory access + // If the source memory was freed, accessing it will cause a panic + // We catch it here and return None instead of crashing + use crate::ngx_module::panic_handler::catch_panic; + + catch_panic( + || { + // Use ngx-rust's safe pool() method instead of accessing raw pointer + let pool = req.pool(); + + // Try to create NgxStr - this may panic if memory is invalid + // Safe: NgxStr::from_ngx_str validates the string data + let ngx_str = unsafe { NgxStr::from_ngx_str(src) }; + + match ngx_str.to_str() { + Ok(s) => { + let len = s.len(); + let data = pool.alloc(len).cast::(); + if data.is_null() { + return None; + } + // Safe: We've validated data is not null and len is correct + unsafe { + ptr::copy_nonoverlapping(s.as_ptr(), data, len); + } + Some(ngx_str_t { len, data }) + } + Err(_) => None, + } + }, + "copy_string_to_request_pool", + ) + .flatten() +} + +/// Safely clone configuration, copying all strings to request pool +/// +/// This function creates a new configuration with all string fields copied +/// to the request's memory pool, preventing segfaults from dangling pointers. +/// +/// Uses ngx-rust's safe `Request` API instead of raw pointers. +/// +/// # Memory Safety +/// +/// This function handles cases where the source configuration's memory pool may have +/// been freed. If accessing any string field causes a panic (due to invalid memory), +/// the function returns an error instead of crashing. This prevents segfaults when +/// configuration memory pools are freed during request processing. +fn clone_config_to_request_pool(req: &Request, src: &X402Config) -> Result { + // CRITICAL: Accessing src fields may cause segfault if src's memory pool was freed. + // We use panic protection to catch any segfaults during field access. + // If a panic occurs, it means the memory is invalid and we return an error. + + // First, try to read the enabled field to validate memory is accessible + // This is a simple integer field that's less likely to cause issues + let enabled = match catch_panic(|| src.enabled, "read enabled field") { + Some(val) => val, + None => { + return Err(ConfigError::from( + "Configuration memory is invalid (cannot access enabled field)", + )); + } + }; + + // Use a helper macro to safely copy each field with panic protection + macro_rules! safe_copy_field { + ($field:ident) => { + match catch_panic( + || copy_string_to_request_pool(req, src.$field), + &format!("copy {}", stringify!($field)), + ) { + Some(Some(val)) => val, + Some(None) => { + return Err(ConfigError::from(format!( + "Failed to copy {} to request pool", + stringify!($field) + ))); + } + None => { + return Err(ConfigError::from(format!( + "Failed to access {} field (memory may be invalid)", + stringify!($field) + ))); + } + } + }; + } + + Ok(X402Config { + enabled, + amount_str: safe_copy_field!(amount_str), + pay_to_str: safe_copy_field!(pay_to_str), + facilitator_url_str: safe_copy_field!(facilitator_url_str), + description_str: safe_copy_field!(description_str), + network_str: safe_copy_field!(network_str), + network_id_str: safe_copy_field!(network_id_str), + resource_str: safe_copy_field!(resource_str), + asset_str: safe_copy_field!(asset_str), + asset_decimals_str: safe_copy_field!(asset_decimals_str), + timeout_str: safe_copy_field!(timeout_str), + facilitator_fallback_str: safe_copy_field!(facilitator_fallback_str), + ttl_str: safe_copy_field!(ttl_str), + }) +} + /// Helper function to get `ngx_http_core_main_conf_t` from `ngx_conf_t` /// /// This is equivalent to `ngx_http_conf_get_module_main_conf(cf`, `ngx_http_core_module`) +/// +/// # Safety +/// +/// This function uses panic protection to catch any invalid memory access. +/// If accessing the configuration structure causes a panic (e.g., due to invalid pointers), +/// the function returns None instead of crashing. unsafe fn get_core_main_conf( cf: *mut ngx::ffi::ngx_conf_t, ) -> Option<*mut ngx_http_core_main_conf_t> { @@ -38,35 +176,51 @@ unsafe fn get_core_main_conf( return None; } - let ctx = (*cf).ctx.cast::(); - if ctx.is_null() { - return None; - } + // Use panic protection to catch invalid memory access + use crate::ngx_module::panic_handler::catch_panic; + catch_panic( + || { + let ctx = (*cf).ctx.cast::(); + if ctx.is_null() { + return None; + } - let main_conf = (*ctx).main_conf; - if main_conf.is_null() { - return None; - } + let main_conf = (*ctx).main_conf; + if main_conf.is_null() { + return None; + } - // Get core module's main config using ctx_index - // main_conf is *mut *mut c_void (pointer to array of pointers) - // Use main_conf.add() directly, not (*main_conf).add() - let core_ctx_index = ngx::ffi::ngx_http_core_module.ctx_index; - let ptr_to_ptr = main_conf.add(core_ctx_index); - if ptr_to_ptr.is_null() { - return None; - } + // Get core module's main config using ctx_index + // main_conf is *mut *mut c_void (pointer to array of pointers) + // Use main_conf.add() directly, not (*main_conf).add() + let core_ctx_index = ngx::ffi::ngx_http_core_module.ctx_index; - // Read the pointer value from the array - let cmcf_void: *mut core::ffi::c_void = ptr::read(ptr_to_ptr.cast_const()); - if cmcf_void.is_null() { - return None; - } + // Validate ctx_index is reasonable (prevent out-of-bounds access) + // Typical nginx setups have < 100 modules + const MAX_REASONABLE_CTX_INDEX: usize = 256; + if core_ctx_index >= MAX_REASONABLE_CTX_INDEX { + return None; + } + + let ptr_to_ptr = main_conf.add(core_ctx_index); + if ptr_to_ptr.is_null() { + return None; + } - Some(core::mem::transmute::< - *mut core::ffi::c_void, - *mut ngx_http_core_main_conf_t, - >(cmcf_void)) + // Read the pointer value from the array + // Use read_volatile to prevent compiler optimizations that might skip invalid reads + let cmcf_void: *mut core::ffi::c_void = ptr::read_volatile(ptr_to_ptr.cast_const()); + if cmcf_void.is_null() { + return None; + } + + // Use cast() instead of transmute for better type safety + // cast() is slightly safer than transmute as it's more explicit + Some(cmcf_void.cast::()) + }, + "get_core_main_conf", + ) + .flatten() } /// Postconfiguration hook @@ -377,10 +531,42 @@ pub fn get_module_config(req: &Request) -> Result { // This is a basic sanity check - if the pointer is invalid, this might fail // Note: We can't easily validate the structure without knowing its layout, // but we can at least ensure the pointer is aligned and accessible - let _ = std::ptr::read_volatile(&raw const (*conf_ptr).enabled); + // Use panic protection to catch any invalid memory access + use crate::ngx_module::panic_handler::catch_panic; + // We're already inside an unsafe block, so read_volatile doesn't need its own unsafe + let _enabled_check = catch_panic( + || std::ptr::read_volatile(&raw const (*conf_ptr).enabled), + "read_volatile enabled field", + ); + if _enabled_check.is_none() { + return Err(ConfigError::from( + "Configuration pointer is invalid (cannot read enabled field). This may indicate memory corruption or invalid pointer." + )); + } - // Clone the configuration - // Safety: We've validated that conf_ptr is non-null and points to valid memory - Ok((*conf_ptr).clone()) + // CRITICAL: We cannot safely clone the configuration because accessing its fields + // may cause segfaults if the configuration's memory pool was freed. + // Instead, we'll parse the configuration immediately while we have a valid reference, + // converting all strings to Rust Strings before the memory pool could be freed. + // + // However, we still need to return X402Config for the API. The safest approach is to: + // 1. Try to clone with panic protection + // 2. If cloning fails (due to invalid memory), return an error + // 3. This allows the handler to gracefully handle the error instead of crashing + // + // Use ngx-rust's safe Request API instead of raw pointer + match catch_panic( + || clone_config_to_request_pool(req, &*conf_ptr), + "clone_config_to_request_pool", + ) { + Some(Ok(config)) => Ok(config), + Some(Err(e)) => Err(e), + None => { + // Panic occurred - memory is likely invalid + Err(ConfigError::from( + "Configuration memory is invalid (may have been freed). This can occur during config reload." + )) + } + } } } diff --git a/src/ngx_module/request.rs b/src/ngx_module/request.rs index 2d1069a..e26acd9 100644 --- a/src/ngx_module/request.rs +++ b/src/ngx_module/request.rs @@ -181,23 +181,19 @@ pub fn is_websocket_request(r: &Request) -> bool { /// Uses the `method` field (integer ID) for reliable detection, falling back to /// `method_name` (string) if the method ID is not recognized. /// -/// # Safety -/// This function accesses the raw request pointer to check the HTTP method. -/// The caller must ensure the request pointer is valid. +/// Uses ngx-rust's safe `Request` API instead of raw pointers. /// /// # Arguments -/// - `r`: Raw nginx request pointer +/// - `r`: Nginx request object /// /// # Returns /// - `Some(&str)` with the HTTP method name (uppercase) if available -/// - `None` if request pointer is null or method cannot be determined +/// - `None` if method cannot be determined #[must_use] -pub unsafe fn get_http_method(r: *const ngx::ffi::ngx_http_request_t) -> Option<&'static str> { - if r.is_null() { - return None; - } - - let request_struct = &*r; +pub fn get_http_method(r: &Request) -> Option<&'static str> { + // Use Request's as_ref() to access the underlying structure + // Safe: Request is a zero-cost wrapper, as_ref() returns a valid reference + let request_struct = r.as_ref(); // Use method ID (integer) for reliable detection // Nginx method IDs: @@ -230,7 +226,30 @@ pub unsafe fn get_http_method(r: *const ngx::ffi::ngx_http_request_t) -> Option< return None; } - let method_slice = std::slice::from_raw_parts(method_name.data, method_name.len); + // Safe: We've validated method_name.data is not null and len > 0 + // Add additional bounds checking to prevent buffer overflows + // Use panic protection to catch any invalid memory access + use crate::ngx_module::panic_handler::catch_panic; + let method_slice = catch_panic( + || { + // Validate length is reasonable (prevent DoS) + const MAX_METHOD_LENGTH: usize = 32; // HTTP methods are typically short + if method_name.len > MAX_METHOD_LENGTH { + return None; + } + // Safe: We've validated data is not null and len is within bounds + unsafe { + Some(std::slice::from_raw_parts( + method_name.data, + method_name.len, + )) + } + }, + "create method_name slice", + ) + .flatten(); + + let method_slice = method_slice?; match method_slice { b"GET" | b"get" => Some("GET"), b"POST" | b"post" => Some("POST"), @@ -247,6 +266,23 @@ pub unsafe fn get_http_method(r: *const ngx::ffi::ngx_http_request_t) -> Option< } } +/// Get HTTP method ID from request +/// +/// Returns the HTTP method as an integer ID (nginx's internal representation). +/// +/// Uses ngx-rust's safe `Request` API instead of raw pointers. +/// +/// # Arguments +/// - `r`: Nginx request object +/// +/// # Returns +/// - Method ID as usize (e.g., 0x00000002 for GET) +#[must_use] +pub fn get_http_method_id(r: &Request) -> usize { + let request_struct = r.as_ref(); + request_struct.method +} + /// Build full URL from request /// /// Constructs a complete URL from the request's scheme, host, and URI. @@ -374,18 +410,16 @@ pub fn infer_mime_type(r: &Request) -> String { /// These methods are typically used for infrastructure/checking purposes rather than /// actual resource access, so payment verification should be skipped. /// -/// # Safety -/// This function accesses the raw request pointer to check the HTTP method. -/// The caller must ensure the request pointer is valid. +/// Uses ngx-rust's safe `Request` API instead of raw pointers. /// /// # Arguments -/// - `r`: Raw nginx request pointer +/// - `r`: Nginx request object /// /// # Returns /// - `true` if the HTTP method should skip payment verification /// - `false` otherwise #[must_use] -pub unsafe fn should_skip_payment_for_method(r: *const ngx::ffi::ngx_http_request_t) -> bool { +pub fn should_skip_payment_for_method(r: &Request) -> bool { matches!( get_http_method(r), Some("OPTIONS") | Some("HEAD") | Some("TRACE") diff --git a/src/ngx_module/response.rs b/src/ngx_module/response.rs index 53159ab..85f11d4 100644 --- a/src/ngx_module/response.rs +++ b/src/ngx_module/response.rs @@ -100,13 +100,53 @@ pub fn send_response_body(r: &mut Request, body: &[u8]) -> Result<()> { return Err(ConfigError::from("Failed to allocate buffer")); } - // Copy body data to buffer + // Copy body data to buffer with bounds checking unsafe { - let buf_slice = core::slice::from_raw_parts_mut((*buf).pos, body_len); + // Validate buffer pointer and fields + let buf_ptr = &mut *buf; + + // Verify pos is not null + if buf_ptr.pos.is_null() { + return Err(ConfigError::from("Buffer pos pointer is null")); + } + + // Verify buffer capacity: end - pos should be >= body_len + // ngx_create_temp_buf allocates exactly body_len bytes, so end - pos == body_len + // But we check anyway to be safe + let pos_ptr = buf_ptr.pos as usize; + let end_ptr = buf_ptr.end as usize; + + if end_ptr < pos_ptr { + return Err(ConfigError::from("Buffer end is before buffer pos")); + } + + let buf_capacity = end_ptr - pos_ptr; + if buf_capacity < body_len { + return Err(ConfigError::from(format!( + "Buffer capacity ({}) is less than body length ({})", + buf_capacity, body_len + ))); + } + + // Use checked_add to prevent integer overflow + let new_last_ptr = pos_ptr + .checked_add(body_len) + .ok_or_else(|| ConfigError::from("Buffer pointer addition overflow"))?; + + // Verify new_last doesn't exceed buffer end + if new_last_ptr > end_ptr { + return Err(ConfigError::from("Buffer pointer would exceed buffer end")); + } + + // Safe to create slice: we've validated pos is not null and body_len is within bounds + let buf_slice = core::slice::from_raw_parts_mut(buf_ptr.pos, body_len); buf_slice.copy_from_slice(body); - (*buf).last = (*buf).pos.add(body_len); - (*buf).set_last_buf(1); - (*buf).set_last_in_chain(1); + + // Safe to set last: we've validated the addition doesn't overflow + // last is a *mut u8, so we can directly assign the pointer + buf_ptr.last = new_last_ptr as *mut u8; + buf_ptr.set_last_buf(1); + buf_ptr.set_last_in_chain(1); } // Allocate chain link @@ -149,7 +189,35 @@ pub fn send_response_body(r: &mut Request, body: &[u8]) -> Result<()> { } // Send body using output filter - let chain_mut = unsafe { &mut *chain }; + // Double-check chain pointer before dereferencing (defense in depth) + if chain.is_null() { + return Err(ConfigError::from("Chain pointer became null before use")); + } + + // Use panic protection to catch any invalid memory access + // This protects against cases where the chain was freed between allocation and use + use crate::ngx_module::panic_handler::catch_panic; + let chain_mut = catch_panic( + || { + // Safe: We've validated chain is not null above + // However, the memory may have been freed, so we use panic protection + unsafe { + // Validate that buf field is accessible before dereferencing + // This is a basic sanity check + let chain_ptr = &mut *chain; + // Verify buf is not null (it should point to our allocated buffer) + if chain_ptr.buf.is_null() { + return Err(ConfigError::from("Chain buf pointer is null")); + } + Ok(chain_ptr) + } + }, + "dereference chain pointer", + ) + .ok_or_else(|| { + ConfigError::from("Chain pointer points to invalid memory (may have been freed)") + })??; + let status = r.output_filter(chain_mut); if status == Status::NGX_OK { } else { diff --git a/src/ngx_module/runtime.rs b/src/ngx_module/runtime.rs index 1223c53..47404b0 100644 --- a/src/ngx_module/runtime.rs +++ b/src/ngx_module/runtime.rs @@ -4,7 +4,7 @@ use crate::ngx_module::error::{ConfigError, Result}; use rust_x402::facilitator::FacilitatorClient; use rust_x402::types::FacilitatorConfig; use std::collections::HashMap; -use std::sync::{Mutex, OnceLock}; +use std::sync::{Arc, Mutex, OnceLock}; use std::time::Duration; use tokio::time::timeout; @@ -15,7 +15,10 @@ pub static RUNTIME: OnceLock = OnceLock::new(); /// /// Stores facilitator clients keyed by URL to enable reuse across requests. /// Each URL gets its own client instance with connection pooling. -pub static FACILITATOR_CLIENTS: OnceLock>> = +/// +/// Uses `Arc` to avoid unsafe pointer conversions. +/// This allows safe sharing of clients across requests without unsafe code. +pub static FACILITATOR_CLIENTS: OnceLock>>> = OnceLock::new(); /// Default timeout for facilitator requests (10 seconds) @@ -47,13 +50,15 @@ pub fn get_runtime() -> Result<&'static tokio::runtime::Runtime> { /// Uses a global pool to reuse clients across requests, improving performance /// by avoiding repeated client creation and connection setup. /// +/// Uses `Arc` to safely share clients without unsafe pointer conversions. +/// /// # Arguments /// - `url`: Facilitator service URL /// /// # Returns -/// - `Ok(&'static FacilitatorClient)` if client is available +/// - `Ok(Arc)` if client is available /// - `Err` if client cannot be created or retrieved -pub fn get_facilitator_client(url: &str) -> Result<&'static FacilitatorClient> { +pub fn get_facilitator_client(url: &str) -> Result> { let clients = FACILITATOR_CLIENTS.get_or_init(|| Mutex::new(HashMap::new())); // Check if client already exists @@ -62,11 +67,9 @@ pub fn get_facilitator_client(url: &str) -> Result<&'static FacilitatorClient> { .lock() .map_err(|_| ConfigError::from("Lock poisoned"))?; if let Some(client) = guard.get(url) { - // Return a reference to the existing client - // SAFETY: The client is stored in a static OnceLock, so it lives for 'static - // We need to use unsafe to convert the reference, but the client is guaranteed - // to live as long as the program runs. - return unsafe { Ok(&*std::ptr::from_ref::(client)) }; + // Return a clone of the Arc - this is safe and doesn't require unsafe + // Arc::clone only increments the reference count, it doesn't clone the data + return Ok(Arc::clone(client)); } } @@ -75,22 +78,19 @@ pub fn get_facilitator_client(url: &str) -> Result<&'static FacilitatorClient> { let client = FacilitatorClient::new(config) .map_err(|e| ConfigError::from(format!("Failed to create facilitator client: {e}")))?; + // Wrap in Arc for safe sharing + let client_arc = Arc::new(client); + // Store in pool { let mut guard = clients .lock() .map_err(|_| ConfigError::from("Lock poisoned"))?; - guard.insert(url.to_string(), client); + guard.insert(url.to_string(), Arc::clone(&client_arc)); } - // Retrieve the stored client - let guard = clients - .lock() - .map_err(|_| ConfigError::from("Lock poisoned"))?; - guard - .get(url) - .map(|client| unsafe { &*std::ptr::from_ref::(client) }) - .ok_or_else(|| ConfigError::from("Failed to retrieve facilitator client")) + // Return the Arc - no unsafe needed! + Ok(client_arc) } /// Verify payment with facilitator service diff --git a/test_struct_size b/test_struct_size new file mode 100755 index 0000000..c3959a1 Binary files /dev/null and b/test_struct_size differ diff --git a/tests/docker_integration/common.rs b/tests/docker_integration/common.rs index 9fbf8d9..d6be587 100644 --- a/tests/docker_integration/common.rs +++ b/tests/docker_integration/common.rs @@ -339,3 +339,35 @@ pub fn ensure_container_running() -> bool { cleanup_container(); build_docker_image() && start_container() && wait_for_nginx(Duration::from_secs(10)) } + +/// Get recent Docker container logs +/// +/// # Arguments +/// +/// * `lines` - Number of recent log lines to retrieve +/// +/// # Returns +/// +/// Returns `Some(logs)` if logs were retrieved successfully, `None` otherwise. +pub fn get_recent_docker_logs(lines: usize) -> Option { + // Docker logs command outputs all logs (both stdout and stderr) to stdout by default + let output = Command::new("docker") + .args(["logs", "--tail", &lines.to_string(), CONTAINER_NAME]) + .output() + .ok()?; + + // Docker logs outputs everything to stdout (including stderr from container) + let stdout = String::from_utf8_lossy(&output.stdout).to_string(); + let stderr = String::from_utf8_lossy(&output.stderr).to_string(); + + // Combine both outputs (docker logs command output goes to stdout, errors to stderr) + if stdout.is_empty() && stderr.is_empty() { + None + } else if stdout.is_empty() { + Some(stderr) + } else if stderr.is_empty() { + Some(stdout) + } else { + Some(format!("{}\n{}", stdout, stderr)) + } +} diff --git a/tests/docker_integration/mod.rs b/tests/docker_integration/mod.rs index fe03ef1..97241bd 100644 --- a/tests/docker_integration/mod.rs +++ b/tests/docker_integration/mod.rs @@ -56,6 +56,9 @@ pub mod config_tests; #[cfg(feature = "integration-test")] pub mod header_passthrough_tests; +#[cfg(feature = "integration-test")] +pub mod segfault_reproduction_tests; + #[cfg(feature = "integration-test")] pub mod timestamp_tests; diff --git a/tests/docker_integration/segfault_reproduction_tests.rs b/tests/docker_integration/segfault_reproduction_tests.rs new file mode 100644 index 0000000..b7ed44f --- /dev/null +++ b/tests/docker_integration/segfault_reproduction_tests.rs @@ -0,0 +1,185 @@ +//! Tests to reproduce segfault issues in production-like scenarios +//! +//! These tests attempt to reproduce the segfault issues that occur in production +//! but not in simple integration tests. The key differences are: +//! +//! 1. **Configuration Merging**: Production uses server-level config that merges into location-level +//! 2. **Memory Pool Lifecycle**: Production may reload config or have complex memory pool patterns +//! 3. **Concurrency**: Production has multiple worker processes handling concurrent requests +//! 4. **Timing**: Segfaults may be timing-dependent, requiring specific memory allocation patterns + +#[cfg(feature = "integration-test")] +mod tests { + use crate::docker_integration::common::*; + use std::thread; + use std::time::Duration; + + /// Test configuration merging scenario that may trigger segfaults + /// + /// This test simulates production scenario where: + /// - Server-level configuration provides default values + /// - Location-level configuration inherits and overrides + /// - Configuration merging happens during request processing + /// + /// The segfault occurs when accessing strings from merged configuration + /// if the parent config's memory pool was freed. + #[test] + #[ignore = "requires Docker and may not reproduce segfault"] + fn test_config_merging_segfault_scenario() { + if !ensure_container_running() { + eprintln!("Failed to start container. Skipping test."); + return; + } + + // Make requests that trigger configuration merging + // In production, this happens when server-level config merges into location-level + for i in 0..50 { + // Alternate between different endpoints to trigger different config paths + let endpoint = if i % 2 == 0 { + "/api/protected" + } else { + "/api/" + }; + + let _ = http_request(endpoint); + + // Small delay to allow memory pool operations + thread::sleep(Duration::from_millis(50)); + + // Check for segfaults periodically + if i % 10 == 0 { + let logs = get_recent_docker_logs(50).unwrap_or_default(); + if logs.contains("signal 11") || logs.contains("core dumped") { + panic!("Segfault detected at request {}! Logs: {}", i, logs); + } + } + } + + println!("✓ Configuration merging test completed"); + } + + /// Test concurrent requests that may trigger race conditions + /// + /// Production environment has multiple worker processes handling concurrent requests. + /// This test simulates that scenario to trigger potential race conditions. + #[test] + #[ignore = "requires Docker and may not reproduce segfault"] + fn test_concurrent_request_segfault_scenario() { + if !ensure_container_running() { + eprintln!("Failed to start container. Skipping test."); + return; + } + + let num_threads = 4; + let requests_per_thread = 20; + let mut handles = vec![]; + + for _ in 0..num_threads { + let handle = thread::spawn(move || { + for _ in 0..requests_per_thread { + let _ = http_request("/api/protected"); + thread::sleep(Duration::from_millis(10)); + } + }); + handles.push(handle); + } + + // Wait for all threads to complete + for handle in handles { + handle.join().unwrap(); + } + + // Wait a bit for any segfaults to be logged + thread::sleep(Duration::from_millis(1000)); + + // Check for segfaults + let logs = get_recent_docker_logs(100).unwrap_or_default(); + assert!( + !logs.contains("signal 11") && !logs.contains("core dumped"), + "Segfault detected in concurrent request test! Logs: {}", + logs.chars().take(2000).collect::() + ); + + println!("✓ Concurrent request test completed"); + } + + /// Test rapid request pattern that may trigger memory pool issues + /// + /// This test sends many rapid requests to trigger memory pool allocation + /// and deallocation patterns that may expose the segfault. + #[test] + #[ignore = "requires Docker and may not reproduce segfault"] + fn test_rapid_requests_segfault_scenario() { + if !ensure_container_running() { + eprintln!("Failed to start container. Skipping test."); + return; + } + + // Send rapid requests without delays + for i in 0..100 { + let _ = http_request("/api/protected"); + + // Check for segfaults every 20 requests + if i % 20 == 0 && i > 0 { + thread::sleep(Duration::from_millis(100)); + let logs = get_recent_docker_logs(50).unwrap_or_default(); + if logs.contains("signal 11") || logs.contains("core dumped") { + panic!("Segfault detected at request {}! Logs: {}", i, logs); + } + } + } + + // Final check + thread::sleep(Duration::from_millis(500)); + let logs = get_recent_docker_logs(100).unwrap_or_default(); + assert!( + !logs.contains("signal 11") && !logs.contains("core dumped"), + "Segfault detected in rapid request test! Logs: {}", + logs.chars().take(2000).collect::() + ); + + println!("✓ Rapid request test completed"); + } + + /// Test with configuration that requires merging from parent + /// + /// This test uses a test configuration file that has server-level + /// x402 configuration that merges into location-level config. + /// This is the scenario that triggers the segfault in production. + #[test] + #[ignore = "requires Docker and special test config"] + fn test_parent_config_merging_segfault() { + if !ensure_container_running() { + eprintln!("Failed to start container. Skipping test."); + return; + } + + // This test would require a special nginx config with server-level x402 config + // that merges into location-level config. The current test config doesn't have this. + // In production, the config might look like: + // + // server { + // x402_ttl 60; # Server-level default + // location /api/ { + // x402 on; # Inherits ttl from server level + // } + // } + + // Make requests that would trigger merging + for _ in 0..30 { + let _ = http_request("/api/protected"); + thread::sleep(Duration::from_millis(50)); + } + + // Check for segfaults + thread::sleep(Duration::from_millis(500)); + let logs = get_recent_docker_logs(100).unwrap_or_default(); + assert!( + !logs.contains("signal 11") && !logs.contains("core dumped"), + "Segfault detected in parent config merging test! Logs: {}", + logs.chars().take(2000).collect::() + ); + + println!("✓ Parent config merging test completed"); + } +} diff --git a/tests/docker_integration/timestamp_tests.rs b/tests/docker_integration/timestamp_tests.rs index f806272..149fdbc 100644 --- a/tests/docker_integration/timestamp_tests.rs +++ b/tests/docker_integration/timestamp_tests.rs @@ -67,31 +67,6 @@ mod tests { /// /// * `lines` - Number of lines to retrieve /// - /// # Returns - /// - /// Returns `Some(logs)` if logs were retrieved successfully, `None` otherwise. - fn get_recent_docker_logs(lines: usize) -> Option { - // Docker logs command outputs all logs (both stdout and stderr) to stdout by default - let output = Command::new("docker") - .args(["logs", "--tail", &lines.to_string(), CONTAINER_NAME]) - .output() - .ok()?; - - // Docker logs outputs everything to stdout (including stderr from container) - let stdout = String::from_utf8_lossy(&output.stdout).to_string(); - let stderr = String::from_utf8_lossy(&output.stderr).to_string(); - - // Combine both outputs (docker logs command output goes to stdout, errors to stderr) - if stdout.is_empty() && stderr.is_empty() { - None - } else if stdout.is_empty() { - Some(stderr) - } else if stderr.is_empty() { - Some(stdout) - } else { - Some(format!("{}\n{}", stdout, stderr)) - } - } #[test] #[ignore = "requires Docker"]