From 0e80c0057a581b2c7773a3538cf802cb68256648 Mon Sep 17 00:00:00 2001 From: "Ryan.K" Date: Wed, 3 Dec 2025 19:14:27 +0800 Subject: [PATCH 1/5] test: add integration tests for x402_ttl configuration segfault issue - Add ttl_config_tests module to detect segfaults - Test TTL configuration doesn't cause worker process crashes - Test TTL configuration merging from parent to child locations - Test multiple requests with TTL configuration - Add x402_ttl 60 to test configuration This addresses the segfault issue reported where worker processes exit with signal 11 (core dumped) after TTL configuration was added. --- tests/docker_integration/mod.rs | 3 + tests/docker_integration/ttl_config_tests.rs | 158 +++++++++++++++++++ tests/nginx.test.conf | 1 + 3 files changed, 162 insertions(+) create mode 100644 tests/docker_integration/ttl_config_tests.rs diff --git a/tests/docker_integration/mod.rs b/tests/docker_integration/mod.rs index 0d247db..fe03ef1 100644 --- a/tests/docker_integration/mod.rs +++ b/tests/docker_integration/mod.rs @@ -58,3 +58,6 @@ pub mod header_passthrough_tests; #[cfg(feature = "integration-test")] pub mod timestamp_tests; + +#[cfg(feature = "integration-test")] +pub mod ttl_config_tests; diff --git a/tests/docker_integration/ttl_config_tests.rs b/tests/docker_integration/ttl_config_tests.rs new file mode 100644 index 0000000..819695b --- /dev/null +++ b/tests/docker_integration/ttl_config_tests.rs @@ -0,0 +1,158 @@ +//! Integration tests for x402_ttl configuration directive +//! +//! These tests verify that x402_ttl configuration works correctly and doesn't cause segfaults. +//! This is critical because TTL configuration was added recently and may have memory safety issues. + +#[cfg(feature = "integration-test")] +mod tests { + use crate::docker_integration::common::*; + use std::process::Command; + use std::thread; + use std::time::Duration; + + /// Test that x402_ttl configuration doesn't cause segfaults + /// + /// This test verifies that: + /// 1. x402_ttl can be configured without causing segfaults + /// 2. Multiple requests with TTL configuration work correctly + /// 3. Worker processes don't crash + #[test] + #[ignore = "requires Docker"] + fn test_ttl_configuration_no_segfault() { + if !ensure_container_running() { + eprintln!("Failed to start container. Skipping test."); + return; + } + + // Make multiple requests to trigger potential segfaults + // The segfault typically occurs during configuration merging or request processing + for i in 0..10 { + let _ = http_request("/api/protected"); + thread::sleep(Duration::from_millis(100)); + + // Check if nginx is still running (if segfault occurred, nginx would restart) + if i % 3 == 0 { + let status = http_request("/health"); + assert!( + status.is_some() && status.unwrap() == "200", + "Nginx should still be running after request {}. If this fails, segfault may have occurred.", + i + ); + } + } + + println!("✓ TTL configuration test completed without segfaults"); + } + + /// Test that x402_ttl with different values works correctly + /// + /// This test verifies that: + /// 1. TTL configuration is parsed correctly + /// 2. Different TTL values don't cause issues + /// 3. Configuration merging works correctly + #[test] + #[ignore = "requires Docker"] + fn test_ttl_configuration_values() { + if !ensure_container_running() { + eprintln!("Failed to start container. Skipping test."); + return; + } + + // Check Docker logs for segfaults or errors + let logs = Command::new("docker") + .args(["logs", "--tail", "50", CONTAINER_NAME]) + .output() + .ok() + .and_then(|output| { + let stdout = String::from_utf8_lossy(&output.stdout).to_string(); + Some(stdout) + }) + .unwrap_or_default(); + + // Make a request that should trigger TTL usage + let _ = http_request("/api/protected"); + + // Wait a bit for any segfaults to occur + thread::sleep(Duration::from_millis(500)); + + // Check for segfault indicators in logs + let recent_logs = Command::new("docker") + .args(["logs", "--tail", "100", CONTAINER_NAME]) + .output() + .ok() + .and_then(|output| { + let stdout = String::from_utf8_lossy(&output.stdout).to_string(); + Some(stdout) + }) + .unwrap_or_default(); + + // Check for segfault indicators + assert!( + !recent_logs.contains("signal 11") && !recent_logs.contains("core dumped"), + "Segfault detected in logs! Logs: {}", + recent_logs.chars().take(1000).collect::() + ); + + // Verify nginx is still running + let status = http_request("/health"); + assert!( + status.is_some() && status.unwrap() == "200", + "Nginx should still be running after TTL configuration test" + ); + + println!("✓ TTL configuration values test completed"); + } + + /// Test that x402_ttl configuration merging works correctly + /// + /// This test verifies that: + /// 1. TTL configuration merges correctly from parent to child locations + /// 2. Configuration merging doesn't cause segfaults + /// 3. Multiple nested locations work correctly + #[test] + #[ignore = "requires Docker"] + fn test_ttl_configuration_merging() { + if !ensure_container_running() { + eprintln!("Failed to start container. Skipping test."); + return; + } + + // Make requests to different endpoints that may have different TTL configurations + // This tests configuration merging from parent to child locations + let endpoints = vec!["/api/protected", "/api/", "/health"]; + + for endpoint in endpoints { + let _ = http_request(endpoint); + thread::sleep(Duration::from_millis(100)); + } + + // Wait a bit for any segfaults to occur + thread::sleep(Duration::from_millis(500)); + + // Check for segfault indicators + let logs = Command::new("docker") + .args(["logs", "--tail", "100", CONTAINER_NAME]) + .output() + .ok() + .and_then(|output| { + let stdout = String::from_utf8_lossy(&output.stdout).to_string(); + Some(stdout) + }) + .unwrap_or_default(); + + assert!( + !logs.contains("signal 11") && !logs.contains("core dumped"), + "Segfault detected during configuration merging! Logs: {}", + logs.chars().take(1000).collect::() + ); + + // Verify nginx is still running + let status = http_request("/health"); + assert!( + status.is_some() && status.unwrap() == "200", + "Nginx should still be running after configuration merging test" + ); + + println!("✓ TTL configuration merging test completed"); + } +} diff --git a/tests/nginx.test.conf b/tests/nginx.test.conf index d6ff645..afd4d81 100644 --- a/tests/nginx.test.conf +++ b/tests/nginx.test.conf @@ -48,6 +48,7 @@ http { x402_network base-sepolia; x402_facilitator_fallback error; x402_description "Test API access payment"; + x402_ttl 60; # TTL configuration for testing segfault issues } # Protected location with x402 payment AND proxy_pass From 88901e955dcee0de7bb76bb79cbfc00a7bf75528 Mon Sep 17 00:00:00 2001 From: "Ryan.K" Date: Wed, 3 Dec 2025 19:17:35 +0800 Subject: [PATCH 2/5] fix: remove unused variable in ttl_config_tests --- tests/docker_integration/ttl_config_tests.rs | 11 ----------- 1 file changed, 11 deletions(-) diff --git a/tests/docker_integration/ttl_config_tests.rs b/tests/docker_integration/ttl_config_tests.rs index 819695b..1bb88fd 100644 --- a/tests/docker_integration/ttl_config_tests.rs +++ b/tests/docker_integration/ttl_config_tests.rs @@ -58,17 +58,6 @@ mod tests { return; } - // Check Docker logs for segfaults or errors - let logs = Command::new("docker") - .args(["logs", "--tail", "50", CONTAINER_NAME]) - .output() - .ok() - .and_then(|output| { - let stdout = String::from_utf8_lossy(&output.stdout).to_string(); - Some(stdout) - }) - .unwrap_or_default(); - // Make a request that should trigger TTL usage let _ = http_request("/api/protected"); From 4c14bb9aacb0fd1a77ee94f19999a095cb08b333 Mon Sep 17 00:00:00 2001 From: "Ryan.K" Date: Wed, 3 Dec 2025 19:20:29 +0800 Subject: [PATCH 3/5] fix: prevent segfault in merge_loc_conf by copying strings to current pool The segfault was caused by directly copying ngx_str_t pointers from prev_conf to conf_mut in merge_loc_conf. When parent and child configurations use different memory pools, copying pointers can lead to dangling pointers that cause segfaults when accessed later. Fix: - Use copy_string_to_pool() to reallocate strings from the current configuration pool instead of copying pointers - Apply this fix to all string fields: amount_str, pay_to_str, facilitator_url_str, description_str, network_str, network_id_str, resource_str, asset_str, asset_decimals_str, timeout_str, facilitator_fallback_str, and ttl_str - Export common module to make copy_string_to_pool accessible This ensures all strings are allocated from the current configuration's memory pool, preventing segfaults when accessing configuration values during request processing. --- src/ngx_module/commands/common.rs | 2 +- src/ngx_module/commands/mod.rs | 2 +- src/ngx_module/module.rs | 80 +++++++++++++++++++++---------- 3 files changed, 56 insertions(+), 28 deletions(-) diff --git a/src/ngx_module/commands/common.rs b/src/ngx_module/commands/common.rs index d350585..66adfcb 100644 --- a/src/ngx_module/commands/common.rs +++ b/src/ngx_module/commands/common.rs @@ -21,7 +21,7 @@ use std::ptr; /// /// * `Some(ngx_str_t)` - Successfully allocated and copied string /// * `None` - Failed to allocate memory -pub(crate) unsafe fn copy_string_to_pool(cf: *mut ngx_conf_t, src: ngx_str_t) -> Option { +pub unsafe fn copy_string_to_pool(cf: *mut ngx_conf_t, src: ngx_str_t) -> Option { if src.len == 0 { return Some(ngx_str_t { len: 0, diff --git a/src/ngx_module/commands/mod.rs b/src/ngx_module/commands/mod.rs index 52351e0..db9f5c5 100644 --- a/src/ngx_module/commands/mod.rs +++ b/src/ngx_module/commands/mod.rs @@ -24,7 +24,7 @@ mod asset; mod basic; -mod common; +pub mod common; mod network; mod other; diff --git a/src/ngx_module/module.rs b/src/ngx_module/module.rs index 9970ed1..2ad52e9 100644 --- a/src/ngx_module/module.rs +++ b/src/ngx_module/module.rs @@ -139,10 +139,12 @@ unsafe extern "C" fn create_loc_conf(cf: *mut ngx::ffi::ngx_conf_t) -> *mut core /// We cannot set handler here because it causes segmentation faults. /// However, we can verify that handler is still set after merging. unsafe extern "C" fn merge_loc_conf( - _cf: *mut ngx::ffi::ngx_conf_t, + cf: *mut ngx::ffi::ngx_conf_t, prev: *mut core::ffi::c_void, conf: *mut core::ffi::c_void, ) -> *mut c_char { + use crate::ngx_module::commands::common::copy_string_to_pool; + let prev = prev.cast::(); let conf = conf.cast::(); @@ -162,42 +164,68 @@ unsafe extern "C" fn merge_loc_conf( // CRITICAL: Do NOT access clcf in merge_loc_conf - it causes segmentation faults // The handler verification must be done at runtime in the handler itself, not during merge - // Merge string fields: use current if non-empty, otherwise use previous - if conf_mut.amount_str.len == 0 { - conf_mut.amount_str = prev_conf.amount_str; + // Merge string fields: use current if non-empty, otherwise copy from previous using current pool + // IMPORTANT: We must copy strings to the current configuration pool instead of copying pointers + // because prev_conf may use a different memory pool that could be freed, causing segfaults + if conf_mut.amount_str.len == 0 && prev_conf.amount_str.len > 0 { + if let Some(copied_str) = copy_string_to_pool(cf, prev_conf.amount_str) { + conf_mut.amount_str = copied_str; + } } - if conf_mut.pay_to_str.len == 0 { - conf_mut.pay_to_str = prev_conf.pay_to_str; + if conf_mut.pay_to_str.len == 0 && prev_conf.pay_to_str.len > 0 { + if let Some(copied_str) = copy_string_to_pool(cf, prev_conf.pay_to_str) { + conf_mut.pay_to_str = copied_str; + } } - if conf_mut.facilitator_url_str.len == 0 { - conf_mut.facilitator_url_str = prev_conf.facilitator_url_str; + if conf_mut.facilitator_url_str.len == 0 && prev_conf.facilitator_url_str.len > 0 { + if let Some(copied_str) = copy_string_to_pool(cf, prev_conf.facilitator_url_str) { + conf_mut.facilitator_url_str = copied_str; + } } - if conf_mut.description_str.len == 0 { - conf_mut.description_str = prev_conf.description_str; + if conf_mut.description_str.len == 0 && prev_conf.description_str.len > 0 { + if let Some(copied_str) = copy_string_to_pool(cf, prev_conf.description_str) { + conf_mut.description_str = copied_str; + } } - if conf_mut.network_str.len == 0 { - conf_mut.network_str = prev_conf.network_str; + if conf_mut.network_str.len == 0 && prev_conf.network_str.len > 0 { + if let Some(copied_str) = copy_string_to_pool(cf, prev_conf.network_str) { + conf_mut.network_str = copied_str; + } } - if conf_mut.network_id_str.len == 0 { - conf_mut.network_id_str = prev_conf.network_id_str; + if conf_mut.network_id_str.len == 0 && prev_conf.network_id_str.len > 0 { + if let Some(copied_str) = copy_string_to_pool(cf, prev_conf.network_id_str) { + conf_mut.network_id_str = copied_str; + } } - if conf_mut.resource_str.len == 0 { - conf_mut.resource_str = prev_conf.resource_str; + if conf_mut.resource_str.len == 0 && prev_conf.resource_str.len > 0 { + if let Some(copied_str) = copy_string_to_pool(cf, prev_conf.resource_str) { + conf_mut.resource_str = copied_str; + } } - if conf_mut.asset_str.len == 0 { - conf_mut.asset_str = prev_conf.asset_str; + if conf_mut.asset_str.len == 0 && prev_conf.asset_str.len > 0 { + if let Some(copied_str) = copy_string_to_pool(cf, prev_conf.asset_str) { + conf_mut.asset_str = copied_str; + } } - if conf_mut.asset_decimals_str.len == 0 { - conf_mut.asset_decimals_str = prev_conf.asset_decimals_str; + if conf_mut.asset_decimals_str.len == 0 && prev_conf.asset_decimals_str.len > 0 { + if let Some(copied_str) = copy_string_to_pool(cf, prev_conf.asset_decimals_str) { + conf_mut.asset_decimals_str = copied_str; + } } - if conf_mut.timeout_str.len == 0 { - conf_mut.timeout_str = prev_conf.timeout_str; + if conf_mut.timeout_str.len == 0 && prev_conf.timeout_str.len > 0 { + if let Some(copied_str) = copy_string_to_pool(cf, prev_conf.timeout_str) { + conf_mut.timeout_str = copied_str; + } } - if conf_mut.facilitator_fallback_str.len == 0 { - conf_mut.facilitator_fallback_str = prev_conf.facilitator_fallback_str; + if conf_mut.facilitator_fallback_str.len == 0 && prev_conf.facilitator_fallback_str.len > 0 { + if let Some(copied_str) = copy_string_to_pool(cf, prev_conf.facilitator_fallback_str) { + conf_mut.facilitator_fallback_str = copied_str; + } } - if conf_mut.ttl_str.len == 0 { - conf_mut.ttl_str = prev_conf.ttl_str; + if conf_mut.ttl_str.len == 0 && prev_conf.ttl_str.len > 0 { + if let Some(copied_str) = copy_string_to_pool(cf, prev_conf.ttl_str) { + conf_mut.ttl_str = copied_str; + } } // Note: Handler is set in ngx_http_x402 command handler when x402 on; is parsed From e5768c8b8bb1f309c583876a62c8208f19923fcd Mon Sep 17 00:00:00 2001 From: "Ryan.K" Date: Wed, 3 Dec 2025 23:05:18 +0800 Subject: [PATCH 4/5] feat: add panic handling for FFI boundaries with detailed logging Add comprehensive panic handling to prevent panics from crossing FFI boundaries, which would cause undefined behavior. This provides detailed logging when panics occur to help with debugging. Changes: - Create panic_handler module with panic protection utilities: - init_panic_hook(): Initialize global panic hook for FFI safety - catch_panic(): Catch panics and log detailed information - catch_panic_or_default(): Catch panics and return default value - catch_panic_with_info(): Catch panics with additional context - Wrap all FFI functions with panic protection: - x402_phase_handler: Protected with catch_panic_or_default - x402_ngx_handler: Protected with catch_panic_or_default - x402_metrics_handler: Protected with catch_panic_or_default - Auto-initialize panic hook in logging module - Add Safety documentation to copy_string_to_pool function Benefits: - Prevents undefined behavior from panics crossing FFI boundaries - Provides detailed panic logs including message, file location, and context - Helps debug issues in production by logging panics to nginx error log - All panics are caught and logged before they can crash nginx worker processes --- src/ngx_module/commands/common.rs | 7 + src/ngx_module/logging.rs | 3 + src/ngx_module/mod.rs | 358 ++++++++++++++++-------------- src/ngx_module/panic_handler.rs | 179 +++++++++++++++ 4 files changed, 382 insertions(+), 165 deletions(-) create mode 100644 src/ngx_module/panic_handler.rs diff --git a/src/ngx_module/commands/common.rs b/src/ngx_module/commands/common.rs index 66adfcb..95954fc 100644 --- a/src/ngx_module/commands/common.rs +++ b/src/ngx_module/commands/common.rs @@ -21,6 +21,13 @@ use std::ptr; /// /// * `Some(ngx_str_t)` - Successfully allocated and copied string /// * `None` - Failed to allocate memory +/// +/// # Safety +/// +/// The caller must ensure that: +/// * `cf` is a valid pointer to a `ngx_conf_t` structure +/// * `src.data` points to valid memory if `src.len > 0` +/// * The source string is valid UTF-8 if it will be converted to a Rust string pub unsafe fn copy_string_to_pool(cf: *mut ngx_conf_t, src: ngx_str_t) -> Option { if src.len == 0 { return Some(ngx_str_t { diff --git a/src/ngx_module/logging.rs b/src/ngx_module/logging.rs index 683bc58..ac3e601 100644 --- a/src/ngx_module/logging.rs +++ b/src/ngx_module/logging.rs @@ -32,6 +32,9 @@ static INIT: Once = Once::new(); /// Initialize the logger pub fn init() { INIT.call_once(|| { + // Initialize panic handler for FFI safety + crate::ngx_module::panic_handler::init_panic_hook(); + log::set_logger(&LOGGER) .map(|()| log::set_max_level(log::LevelFilter::Debug)) .expect("Failed to initialize logger"); diff --git a/src/ngx_module/mod.rs b/src/ngx_module/mod.rs index b3f2145..527dc97 100644 --- a/src/ngx_module/mod.rs +++ b/src/ngx_module/mod.rs @@ -31,6 +31,7 @@ pub mod handler; pub mod logging; pub mod metrics; pub mod module; +pub mod panic_handler; pub mod request; pub mod requirements; pub mod response; @@ -69,18 +70,25 @@ pub use runtime::{ pub unsafe extern "C" fn x402_metrics_handler( r: *mut ngx::ffi::ngx_http_request_t, ) -> ngx::ffi::ngx_int_t { + use crate::ngx_module::panic_handler::catch_panic_or_default; + if r.is_null() { return ngx::ffi::NGX_ERROR as ngx::ffi::ngx_int_t; } - let req_mut = ngx::http::Request::from_ngx_http_request(r); - - match x402_metrics_handler_impl(req_mut) { - ngx::core::Status::NGX_OK => ngx::ffi::NGX_OK as ngx::ffi::ngx_int_t, - ngx::core::Status::NGX_ERROR => ngx::ffi::NGX_ERROR as ngx::ffi::ngx_int_t, - ngx::core::Status::NGX_DECLINED => ngx::ffi::NGX_DECLINED as ngx::ffi::ngx_int_t, - _ => ngx::ffi::NGX_ERROR as ngx::ffi::ngx_int_t, - } + catch_panic_or_default( + || { + let req_mut = ngx::http::Request::from_ngx_http_request(r); + match x402_metrics_handler_impl(req_mut) { + ngx::core::Status::NGX_OK => ngx::ffi::NGX_OK as ngx::ffi::ngx_int_t, + ngx::core::Status::NGX_ERROR => ngx::ffi::NGX_ERROR as ngx::ffi::ngx_int_t, + ngx::core::Status::NGX_DECLINED => ngx::ffi::NGX_DECLINED as ngx::ffi::ngx_int_t, + _ => ngx::ffi::NGX_ERROR as ngx::ffi::ngx_int_t, + } + }, + "x402_metrics_handler", + ngx::ffi::NGX_ERROR as ngx::ffi::ngx_int_t, + ) } /// Clear x402 content handler if it's set @@ -147,180 +155,193 @@ unsafe fn clear_x402_content_handler(r: *mut ngx::ffi::ngx_http_request_t, reaso pub unsafe extern "C" fn x402_phase_handler( r: *mut ngx::ffi::ngx_http_request_t, ) -> ngx::ffi::ngx_int_t { + use crate::ngx_module::panic_handler::catch_panic_or_default; + // Validate pointer if r.is_null() { return ngx::ffi::NGX_ERROR as ngx::ffi::ngx_int_t; } - // 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 - 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, - }; + // Wrap the entire handler in panic protection + catch_panic_or_default( + || { + // 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 + let req_mut = ngx::http::Request::from_ngx_http_request(r); - // Skip payment verification for special request types - // These requests should bypass payment verification: - // 1. Certain HTTP methods (OPTIONS, HEAD, TRACE) - used for protocol-level operations - // - OPTIONS: CORS preflight requests sent by browsers before cross-origin requests - // - HEAD: Used to check resource existence without retrieving body - // - TRACE: Used for diagnostic and debugging purposes - // 2. WebSocket upgrades - long-lived connections that use special HTTP Upgrade mechanism. - // Payment verification would interfere with WebSocket handshake, and subsequent - // WebSocket frames are not HTTP requests, so payment verification is not applicable. - // Payment should be handled at application layer for WebSocket connections. - // 3. Subrequests (auth_request, etc.) - detected via raw request pointer - // 4. Internal redirects - detected via raw request pointer + 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, + }; - // 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) }; + // Skip payment verification for special request types + // These requests should bypass payment verification: + // 1. Certain HTTP methods (OPTIONS, HEAD, TRACE) - used for protocol-level operations + // - OPTIONS: CORS preflight requests sent by browsers before cross-origin requests + // - HEAD: Used to check resource existence without retrieving body + // - TRACE: Used for diagnostic and debugging purposes + // 2. WebSocket upgrades - long-lived connections that use special HTTP Upgrade mechanism. + // Payment verification would interfere with WebSocket handshake, and subsequent + // WebSocket frames are not HTTP requests, so payment verification is not applicable. + // Payment should be handled at application layer for WebSocket connections. + // 3. Subrequests (auth_request, etc.) - detected via raw request pointer + // 4. Internal redirects - detected via raw request pointer - log_debug( - Some(req_mut), - &format!( - "[x402] Phase handler: method_id=0x{:08x}, detected_method={:?}", - method_id, detected_method - ), - ); + // 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) }; - if unsafe { should_skip_payment_for_method(r) } { - let method = detected_method.unwrap_or("UNKNOWN"); - log_debug( - Some(req_mut), - &format!( - "[x402] Phase handler: {} request detected (method_id=0x{:08x}), skipping payment verification", - method, method_id - ), - ); - // 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), + log_debug( + Some(req_mut), + &format!( + "[x402] Phase handler: method_id=0x{:08x}, detected_method={:?}", + method_id, detected_method + ), ); - } - return ngx::ffi::NGX_DECLINED as ngx::ffi::ngx_int_t; - } - // Check for WebSocket upgrade (can be detected via headers) - if is_websocket_request(req_mut) { - log_debug( - Some(req_mut), - "[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"); - } - return ngx::ffi::NGX_DECLINED as ngx::ffi::ngx_int_t; - } + if unsafe { should_skip_payment_for_method(r) } { + let method = detected_method.unwrap_or("UNKNOWN"); + log_debug( + Some(req_mut), + &format!( + "[x402] Phase handler: {} request detected (method_id=0x{:08x}), skipping payment verification", + method, method_id + ), + ); + // 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), + ); + } + 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() { + // Check for WebSocket upgrade (can be detected via headers) + if is_websocket_request(req_mut) { log_debug( Some(req_mut), - "[x402] Phase handler: Subrequest detected (parent != NULL), skipping payment verification", + "[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", + ); + } 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( + // 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: Internal redirect detected (named location @), skipping payment verification", + "[x402] Phase handler: Subrequest detected (parent != NULL), skipping payment verification", ); - return ngx::ffi::NGX_DECLINED as ngx::ffi::ngx_int_t; + return ngx::ffi::NGX_DECLINED as ngx::ffi::ngx_int_t; + } } } - } - } - // Check if module is enabled for this location - let conf = match get_module_config(req_mut) { - Ok(c) => c, - Err(_) => { - // Module not configured for this location, decline to let other handlers process - 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 if module is enabled - if conf.enabled == 0 { - return ngx::ffi::NGX_DECLINED as ngx::ffi::ngx_int_t; - } + // Check if module is enabled for this location + let conf = match get_module_config(req_mut) { + Ok(c) => c, + Err(_) => { + // Module not configured for this location, decline to let other handlers process + return ngx::ffi::NGX_DECLINED as ngx::ffi::ngx_int_t; + } + }; - // Module is enabled - perform payment verification - // This will verify payment and send 402 if needed, or allow request to proceed - use crate::ngx_module::handler::HandlerResult; - let (status, result) = x402_ngx_handler_impl(req_mut); - match (status, result) { - (ngx::core::Status::NGX_OK, HandlerResult::PaymentValid) => { - // Payment verified - allow request to continue - // 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", - ); + // Check if module is enabled + if conf.enabled == 0 { + return ngx::ffi::NGX_DECLINED as ngx::ffi::ngx_int_t; } - // This will proceed to CONTENT_PHASE where proxy_pass handler will run (if set) - ngx::ffi::NGX_OK as ngx::ffi::ngx_int_t - } - (ngx::core::Status::NGX_DECLINED, HandlerResult::ResponseSent) => { - // Response was sent (402 or error) - stop processing - // Return OK to indicate we handled the request and prevent further processing - // This prevents proxy_pass from executing - // Note: In nginx, when a response is sent in ACCESS_PHASE, returning NGX_OK - // tells nginx that we've handled the request and it should not proceed to CONTENT_PHASE - ngx::ffi::NGX_OK as ngx::ffi::ngx_int_t - } - (ngx::core::Status::NGX_ERROR, _) => { - // Error occurred during payment verification - // The handler should have sent an appropriate response (402 or 500) - // Return OK to indicate we handled the request and prevent further processing - ngx::ffi::NGX_OK as ngx::ffi::ngx_int_t - } - _ => { - // Unexpected status - return OK to prevent further processing - ngx::ffi::NGX_OK as ngx::ffi::ngx_int_t - } - } + + // Module is enabled - perform payment verification + // This will verify payment and send 402 if needed, or allow request to proceed + use crate::ngx_module::handler::HandlerResult; + let (status, result) = x402_ngx_handler_impl(req_mut); + match (status, result) { + (ngx::core::Status::NGX_OK, HandlerResult::PaymentValid) => { + // Payment verified - allow request to continue + // 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", + ); + } + // This will proceed to CONTENT_PHASE where proxy_pass handler will run (if set) + ngx::ffi::NGX_OK as ngx::ffi::ngx_int_t + } + (ngx::core::Status::NGX_DECLINED, HandlerResult::ResponseSent) => { + // Response was sent (402 or error) - stop processing + // Return OK to indicate we handled the request and prevent further processing + // This prevents proxy_pass from executing + // Note: In nginx, when a response is sent in ACCESS_PHASE, returning NGX_OK + // tells nginx that we've handled the request and it should not proceed to CONTENT_PHASE + ngx::ffi::NGX_OK as ngx::ffi::ngx_int_t + } + (ngx::core::Status::NGX_ERROR, _) => { + // Error occurred during payment verification + // The handler should have sent an appropriate response (402 or 500) + // Return OK to indicate we handled the request and prevent further processing + ngx::ffi::NGX_OK as ngx::ffi::ngx_int_t + } + _ => { + // Unexpected status - return OK to prevent further processing + ngx::ffi::NGX_OK as ngx::ffi::ngx_int_t + } + } + }, + "x402_phase_handler", + ngx::ffi::NGX_ERROR as ngx::ffi::ngx_int_t, + ) } /// Main content handler C export @@ -344,17 +365,24 @@ pub unsafe extern "C" fn x402_phase_handler( pub unsafe extern "C" fn x402_ngx_handler( r: *mut ngx::ffi::ngx_http_request_t, ) -> ngx::ffi::ngx_int_t { + use crate::ngx_module::panic_handler::catch_panic_or_default; + if r.is_null() { return ngx::ffi::NGX_ERROR as ngx::ffi::ngx_int_t; } - let req_mut = ngx::http::Request::from_ngx_http_request(r); - - let (status, _result) = x402_ngx_handler_impl(req_mut); - match status { - ngx::core::Status::NGX_OK => ngx::ffi::NGX_OK as ngx::ffi::ngx_int_t, - ngx::core::Status::NGX_ERROR => ngx::ffi::NGX_ERROR as ngx::ffi::ngx_int_t, - ngx::core::Status::NGX_DECLINED => ngx::ffi::NGX_DECLINED as ngx::ffi::ngx_int_t, - _ => ngx::ffi::NGX_ERROR as ngx::ffi::ngx_int_t, - } + catch_panic_or_default( + || { + let req_mut = ngx::http::Request::from_ngx_http_request(r); + let (status, _result) = x402_ngx_handler_impl(req_mut); + match status { + ngx::core::Status::NGX_OK => ngx::ffi::NGX_OK as ngx::ffi::ngx_int_t, + ngx::core::Status::NGX_ERROR => ngx::ffi::NGX_ERROR as ngx::ffi::ngx_int_t, + ngx::core::Status::NGX_DECLINED => ngx::ffi::NGX_DECLINED as ngx::ffi::ngx_int_t, + _ => ngx::ffi::NGX_ERROR as ngx::ffi::ngx_int_t, + } + }, + "x402_ngx_handler", + ngx::ffi::NGX_ERROR as ngx::ffi::ngx_int_t, + ) } diff --git a/src/ngx_module/panic_handler.rs b/src/ngx_module/panic_handler.rs new file mode 100644 index 0000000..6517652 --- /dev/null +++ b/src/ngx_module/panic_handler.rs @@ -0,0 +1,179 @@ +//! Panic handling for FFI boundaries +//! +//! This module provides panic handling utilities to prevent panics from crossing +//! FFI boundaries, which would cause undefined behavior. It also provides detailed +//! logging when panics occur to help with debugging. + +use crate::ngx_module::logging::log_error; +use std::panic::{catch_unwind, AssertUnwindSafe}; +use std::sync::Once; + +static PANIC_HOOK_INIT: Once = Once::new(); + +/// Initialize panic hook for FFI safety +/// +/// This sets up a custom panic hook that logs panics to nginx error log +/// instead of stderr. This is important because panics in FFI code can cause +/// undefined behavior if they cross the FFI boundary. +pub fn init_panic_hook() { + PANIC_HOOK_INIT.call_once(|| { + std::panic::set_hook(Box::new(|panic_info| { + log_panic(panic_info); + })); + }); +} + +/// Log panic information to nginx error log +fn log_panic(panic_info: &std::panic::PanicHookInfo) { + let message = if let Some(s) = panic_info.payload().downcast_ref::<&str>() { + format!("Panic: {}", s) + } else if let Some(s) = panic_info.payload().downcast_ref::() { + format!("Panic: {}", s) + } else { + "Panic: unknown panic payload".to_string() + }; + + let location = if let Some(location) = panic_info.location() { + format!( + "{}:{}:{}", + location.file(), + location.line(), + location.column() + ) + } else { + "unknown location".to_string() + }; + + log_error( + None, + &format!("[x402] FFI PANIC DETECTED: {} at {}", message, location), + ); + + // Try to get backtrace if available (requires RUST_BACKTRACE=1) + if std::env::var("RUST_BACKTRACE").is_ok() { + // Backtrace is automatically captured by panic hook when RUST_BACKTRACE is set + // We can't easily access it here, but it will be printed to stderr + log_error( + None, + "[x402] Panic occurred. Set RUST_BACKTRACE=1 for detailed backtrace.", + ); + } +} + +/// Execute a function with panic protection +/// +/// This wrapper catches any panics and converts them to error logs, +/// preventing panics from crossing FFI boundaries. +/// +/// # Arguments +/// +/// * `f` - The function to execute +/// * `context` - Context string for logging (e.g., function name) +/// +/// # Returns +/// +/// * `Some(T)` - Function result if successful +/// * `None` - If panic occurred +pub fn catch_panic(f: F, context: &str) -> Option +where + F: FnOnce() -> T, +{ + match catch_unwind(AssertUnwindSafe(f)) { + Ok(result) => Some(result), + Err(e) => { + let message = if let Some(s) = e.downcast_ref::<&str>() { + format!("Panic in {}: {}", context, s) + } else if let Some(s) = e.downcast_ref::() { + format!("Panic in {}: {}", context, s) + } else { + format!("Panic in {}: unknown panic payload", context) + }; + + log_error(None, &format!("[x402] {}", message)); + + // Log backtrace hint if RUST_BACKTRACE is set + if std::env::var("RUST_BACKTRACE").is_ok() { + log_error( + None, + &format!( + "[x402] Panic in {}. Backtrace available in stderr (RUST_BACKTRACE=1).", + context + ), + ); + } + + None + } + } +} + +/// Execute a function with panic protection and return a default value on panic +/// +/// This is useful for FFI functions that need to return a specific value +/// (like NGX_ERROR) when a panic occurs. +/// +/// # Arguments +/// +/// * `f` - The function to execute +/// * `context` - Context string for logging +/// * `default` - Default value to return if panic occurs +/// +/// # Returns +/// +/// * Function result if successful, or `default` if panic occurred +pub fn catch_panic_or_default(f: F, context: &str, default: T) -> T +where + F: FnOnce() -> T, +{ + catch_panic(f, context).unwrap_or(default) +} + +/// Execute a function with panic protection and detailed error logging +/// +/// This version includes additional context information in the error log. +/// +/// # Arguments +/// +/// * `f` - The function to execute +/// * `context` - Context string for logging +/// * `additional_info` - Additional information to include in logs +/// +/// # Returns +/// +/// * `Some(T)` - Function result if successful +/// * `None` - If panic occurred +pub fn catch_panic_with_info(f: F, context: &str, additional_info: &str) -> Option +where + F: FnOnce() -> T, +{ + match catch_unwind(AssertUnwindSafe(f)) { + Ok(result) => Some(result), + Err(e) => { + let message = if let Some(s) = e.downcast_ref::<&str>() { + format!("Panic in {}: {} | Context: {}", context, s, additional_info) + } else if let Some(s) = e.downcast_ref::() { + format!("Panic in {}: {} | Context: {}", context, s, additional_info) + } else { + format!( + "Panic in {}: unknown panic payload | Context: {}", + context, additional_info + ) + }; + + log_error(None, &format!("[x402] {}", message)); + + // Log backtrace hint if RUST_BACKTRACE is set + if std::env::var("RUST_BACKTRACE").is_ok() { + log_error( + None, + &format!( + "[x402] Panic in {} ({}). Backtrace available in stderr (RUST_BACKTRACE=1).", + context, additional_info + ), + ); + } + + None + } + } +} From e1101a060bfa2670a32b8648f6a07b1239535266 Mon Sep 17 00:00:00 2001 From: "Ryan.K" Date: Wed, 3 Dec 2025 23:41:40 +0800 Subject: [PATCH 5/5] refactor: use macro to eliminate duplicate code in merge_loc_conf Apply DRY principle to reduce code duplication in configuration merging. Changes: - Create merge_string_field! macro to handle string field merging - Replace 60 lines of repetitive code with 12 macro calls - Net reduction: 28 lines of code - Improve maintainability: changes to merge logic only need to be made once Benefits: - Follows DRY (Don't Repeat Yourself) principle - Easier to maintain: single point of change for merge logic - Easier to extend: adding new fields requires only one macro call - More readable: clear intent with concise syntax --- src/ngx_module/module.rs | 92 ++++++++++++++-------------------------- 1 file changed, 32 insertions(+), 60 deletions(-) diff --git a/src/ngx_module/module.rs b/src/ngx_module/module.rs index 2ad52e9..a176739 100644 --- a/src/ngx_module/module.rs +++ b/src/ngx_module/module.rs @@ -8,6 +8,26 @@ use ngx::http::Request; use std::ffi::c_char; use std::ptr; +/// Macro to merge a string field from previous config to current config +/// +/// This macro safely copies a string field from `prev_conf` to `conf_mut` using +/// the current configuration pool, preventing segfaults from dangling pointers. +/// +/// # Usage +/// +/// ```rust,ignore +/// merge_string_field!(cf, conf_mut, prev_conf, field_name); +/// ``` +macro_rules! merge_string_field { + ($cf:expr, $conf_mut:expr, $prev_conf:expr, $field:ident) => { + if $conf_mut.$field.len == 0 && $prev_conf.$field.len > 0 { + if let Some(copied_str) = copy_string_to_pool($cf, $prev_conf.$field) { + $conf_mut.$field = copied_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`) @@ -167,66 +187,18 @@ unsafe extern "C" fn merge_loc_conf( // Merge string fields: use current if non-empty, otherwise copy from previous using current pool // IMPORTANT: We must copy strings to the current configuration pool instead of copying pointers // because prev_conf may use a different memory pool that could be freed, causing segfaults - if conf_mut.amount_str.len == 0 && prev_conf.amount_str.len > 0 { - if let Some(copied_str) = copy_string_to_pool(cf, prev_conf.amount_str) { - conf_mut.amount_str = copied_str; - } - } - if conf_mut.pay_to_str.len == 0 && prev_conf.pay_to_str.len > 0 { - if let Some(copied_str) = copy_string_to_pool(cf, prev_conf.pay_to_str) { - conf_mut.pay_to_str = copied_str; - } - } - if conf_mut.facilitator_url_str.len == 0 && prev_conf.facilitator_url_str.len > 0 { - if let Some(copied_str) = copy_string_to_pool(cf, prev_conf.facilitator_url_str) { - conf_mut.facilitator_url_str = copied_str; - } - } - if conf_mut.description_str.len == 0 && prev_conf.description_str.len > 0 { - if let Some(copied_str) = copy_string_to_pool(cf, prev_conf.description_str) { - conf_mut.description_str = copied_str; - } - } - if conf_mut.network_str.len == 0 && prev_conf.network_str.len > 0 { - if let Some(copied_str) = copy_string_to_pool(cf, prev_conf.network_str) { - conf_mut.network_str = copied_str; - } - } - if conf_mut.network_id_str.len == 0 && prev_conf.network_id_str.len > 0 { - if let Some(copied_str) = copy_string_to_pool(cf, prev_conf.network_id_str) { - conf_mut.network_id_str = copied_str; - } - } - if conf_mut.resource_str.len == 0 && prev_conf.resource_str.len > 0 { - if let Some(copied_str) = copy_string_to_pool(cf, prev_conf.resource_str) { - conf_mut.resource_str = copied_str; - } - } - if conf_mut.asset_str.len == 0 && prev_conf.asset_str.len > 0 { - if let Some(copied_str) = copy_string_to_pool(cf, prev_conf.asset_str) { - conf_mut.asset_str = copied_str; - } - } - if conf_mut.asset_decimals_str.len == 0 && prev_conf.asset_decimals_str.len > 0 { - if let Some(copied_str) = copy_string_to_pool(cf, prev_conf.asset_decimals_str) { - conf_mut.asset_decimals_str = copied_str; - } - } - if conf_mut.timeout_str.len == 0 && prev_conf.timeout_str.len > 0 { - if let Some(copied_str) = copy_string_to_pool(cf, prev_conf.timeout_str) { - conf_mut.timeout_str = copied_str; - } - } - if conf_mut.facilitator_fallback_str.len == 0 && prev_conf.facilitator_fallback_str.len > 0 { - if let Some(copied_str) = copy_string_to_pool(cf, prev_conf.facilitator_fallback_str) { - conf_mut.facilitator_fallback_str = copied_str; - } - } - if conf_mut.ttl_str.len == 0 && prev_conf.ttl_str.len > 0 { - if let Some(copied_str) = copy_string_to_pool(cf, prev_conf.ttl_str) { - conf_mut.ttl_str = copied_str; - } - } + merge_string_field!(cf, conf_mut, prev_conf, amount_str); + merge_string_field!(cf, conf_mut, prev_conf, pay_to_str); + merge_string_field!(cf, conf_mut, prev_conf, facilitator_url_str); + merge_string_field!(cf, conf_mut, prev_conf, description_str); + merge_string_field!(cf, conf_mut, prev_conf, network_str); + merge_string_field!(cf, conf_mut, prev_conf, network_id_str); + merge_string_field!(cf, conf_mut, prev_conf, resource_str); + merge_string_field!(cf, conf_mut, prev_conf, asset_str); + merge_string_field!(cf, conf_mut, prev_conf, asset_decimals_str); + merge_string_field!(cf, conf_mut, prev_conf, timeout_str); + merge_string_field!(cf, conf_mut, prev_conf, facilitator_fallback_str); + merge_string_field!(cf, conf_mut, prev_conf, ttl_str); // Note: Handler is set in ngx_http_x402 command handler when x402 on; is parsed // We cannot set handler here in merge_loc_conf because accessing clcf during merging