From 51425739b7b7642e7b6116ecb0f4de451924aade Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Wed, 6 May 2026 08:29:19 +0000 Subject: [PATCH 1/4] Initial plan From 6394aea9259287fab2a305a4bfa85e645758af04 Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Wed, 6 May 2026 08:35:23 +0000 Subject: [PATCH 2/4] Fix segfaults: replace c->data=item with c->read->data mapping for NTLM cache nginx's ngx_http_upstream_handler() assumes r = c->data is ngx_http_request_t*. Storing our cache item in c->data causes segfaults when posted/ready events fire for cached upstream connections. Fix: store the cache item in c->read->data instead. Since we replace c->read->handler with our own handler, only our handler runs for idle connections and retrieves item via ev->data. Restore c->read->data = c in get_ntlm_peer before returning the connection to upstream core. Agent-Logs-Url: https://github.com/Securepoint/nginx-ntlm-module/sessions/2e9fcf2a-dc92-4dda-831c-76cc616351af Co-authored-by: matthias-lay <163420385+matthias-lay@users.noreply.github.com> --- ngx_http_upstream_ntlm_module.c | 80 +++++++++++++++++++++------------ 1 file changed, 52 insertions(+), 28 deletions(-) diff --git a/ngx_http_upstream_ntlm_module.c b/ngx_http_upstream_ntlm_module.c index 9e32f02..ffda663 100644 --- a/ngx_http_upstream_ntlm_module.c +++ b/ngx_http_upstream_ntlm_module.c @@ -262,6 +262,13 @@ static ngx_int_t ngx_http_upstream_get_ntlm_peer(ngx_peer_connection_t *pc, c->idle = 0; c->sent = 0; c->data = NULL; + /* + * Restore c->read->data to the connection pointer. While cached, + * c->read->data held the ntlm cache item (see ngx_http_upstream_free_ntlm_peer). + * nginx upstream core expects ev->data == c (the connection), so we must + * reset this before handing the connection back to upstream. + */ + c->read->data = c; c->log = pc->log; c->read->log = pc->log; c->write->log = pc->log; @@ -391,14 +398,18 @@ static void ngx_http_upstream_free_ntlm_peer(ngx_peer_connection_t *pc, c->read->handler = ngx_http_upstream_ntlm_close_handler; /* - * c->data stores the cache item while the connection is idle in the - * NTLM cache. The close_handler validates in_cache and peer_connection - * before trusting this pointer, so it is safe. NGINX core never reads - * c->data for idle connections (it only does so inside - * ngx_http_upstream_handler, which is not installed as the read/write - * handler for cached connections). + * Store the cache item in c->read->data (the read event's data field) + * rather than in c->data. nginx's ngx_http_upstream_handler() assumes + * c->data is ngx_http_request_t*, so overwriting it with an ntlm cache + * item pointer causes a segfault if any posted or ready event reaches + * that handler while the connection is idle. Since we have replaced + * c->read->handler with our own ngx_http_upstream_ntlm_close_handler, + * only our handler runs on read events for this connection; it retrieves + * the item via ev->data (= c->read->data) without touching c->data at + * all. c->read->data is restored to c before the connection is returned + * to upstream core (see ngx_http_upstream_get_ntlm_peer). */ - c->data = item; + c->read->data = item; c->idle = 1; c->log = ngx_cycle->log; c->read->log = ngx_cycle->log; @@ -476,7 +487,34 @@ static void ngx_http_upstream_ntlm_close_handler(ngx_event_t *ev) { char buf[1]; ngx_connection_t *c; - c = ev->data; + /* + * While a connection is idle in the NTLM cache its read event's data + * field (c->read->data, i.e. ev->data here) holds a pointer to the + * owning ngx_http_upstream_ntlm_cache_t item. We retrieve the item + * directly from ev->data rather than from c->data, because c->data must + * be left alone: nginx's ngx_http_upstream_handler() assumes c->data is + * ngx_http_request_t* and will segfault if it finds our cache item there + * via any posted or ready event. + */ + item = ev->data; + + /* + * item should never be NULL here because we always set c->read->data = + * item before installing this handler, and c->read->data is restored to + * c only after the handler is replaced. The NULL guard is a belt-and- + * suspenders safety check. + */ + if (item == NULL || !item->in_cache) { + /* Already reclaimed by cleanup or eviction; nothing to do. */ + return; + } + + c = item->peer_connection; + + if (c == NULL) { + /* Ownership was cleared; nothing to do. */ + return; + } if (c->close || c->read->timedout) { goto close; @@ -497,27 +535,13 @@ static void ngx_http_upstream_ntlm_close_handler(ngx_event_t *ev) { close: /* - * c->data holds the cache item pointer only while the connection is - * idle in the NTLM cache (item->in_cache == 1 and - * item->peer_connection == c). - * - * Guard against two hazard scenarios: - * - * 1. The cleanup handler already reclaimed this connection - * (set in_cache = 0 and peer_connection = NULL) before this - * handler ran. In that case c->data is still the old item - * pointer, but touching the queue would corrupt state. - * - * 2. The item was evicted and reassigned to a different upstream - * connection (peer_connection != c). Closing or re-queuing - * based on stale c->data would affect a live connection. - * - * Both cases are safely caught by the in_cache + peer_connection - * ownership check below. + * Belt-and-suspenders guard for the close: path. Although the early + * checks above already verified in_cache and peer_connection before any + * blocking/yield point, this second check ensures correctness if, due to + * a hypothetical future code change, the close: label is reached by a + * path that did not pass the early guards. */ - item = c->data; - - if (item == NULL || !item->in_cache || item->peer_connection != c) { + if (!item->in_cache || item->peer_connection != c) { /* Connection is no longer ours; nothing to do. */ return; } From 28cccc6b3d06195c9de21e872c8374f1a2741090 Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Wed, 6 May 2026 09:14:53 +0000 Subject: [PATCH 3/4] fix: issue 1 (c->data=NULL), 2 (ntlm_requests/ntlm_time limits), 3 (notify passthrough), 5 (eviction NULL guard) Agent-Logs-Url: https://github.com/Securepoint/nginx-ntlm-module/sessions/672b9196-95fe-4861-bee8-50b1cc5ff159 Co-authored-by: matthias-lay <163420385+matthias-lay@users.noreply.github.com> --- ngx_http_upstream_ntlm_module.c | 45 ++++++++++++++++++++++++++++++++- 1 file changed, 44 insertions(+), 1 deletion(-) diff --git a/ngx_http_upstream_ntlm_module.c b/ngx_http_upstream_ntlm_module.c index ffda663..3af1a2f 100644 --- a/ngx_http_upstream_ntlm_module.c +++ b/ngx_http_upstream_ntlm_module.c @@ -13,6 +13,8 @@ static void ngx_http_upstream_free_ntlm_peer(ngx_peer_connection_t *pc, static void ngx_http_upstream_ntlm_dummy_handler(ngx_event_t *ev); static void ngx_http_upstream_ntlm_close_handler(ngx_event_t *ev); static void ngx_http_upstream_ntlm_close(ngx_connection_t *c); +static void ngx_http_upstream_ntlm_notify_peer(ngx_peer_connection_t *pc, + void *data, ngx_uint_t type); #if (NGX_HTTP_SSL) static ngx_int_t ngx_http_upstream_ntlm_set_session(ngx_peer_connection_t *pc, @@ -29,6 +31,8 @@ static void ngx_http_upstream_client_conn_cleanup(void *data); typedef struct { ngx_uint_t max_cached; + ngx_uint_t requests; + ngx_msec_t time; ngx_msec_t timeout; ngx_queue_t free; ngx_queue_t cache; @@ -62,6 +66,7 @@ typedef struct { unsigned ntlm_init : 1; ngx_event_get_peer_pt original_get_peer; ngx_event_free_peer_pt original_free_peer; + ngx_event_notify_peer_pt original_notify; #if (NGX_HTTP_SSL) ngx_event_set_peer_session_pt original_set_session; ngx_event_save_peer_session_pt original_save_session; @@ -78,6 +83,14 @@ static ngx_command_t ngx_http_upstream_ntlm_commands[] = { ngx_conf_set_msec_slot, NGX_HTTP_SRV_CONF_OFFSET, offsetof(ngx_http_upstream_ntlm_srv_conf_t, timeout), NULL}, + {ngx_string("ntlm_time"), NGX_HTTP_UPS_CONF | NGX_CONF_TAKE1, + ngx_conf_set_msec_slot, NGX_HTTP_SRV_CONF_OFFSET, + offsetof(ngx_http_upstream_ntlm_srv_conf_t, time), NULL}, + + {ngx_string("ntlm_requests"), NGX_HTTP_UPS_CONF | NGX_CONF_TAKE1, + ngx_conf_set_num_slot, NGX_HTTP_SRV_CONF_OFFSET, + offsetof(ngx_http_upstream_ntlm_srv_conf_t, requests), NULL}, + ngx_null_command /* command termination */ }; @@ -123,6 +136,8 @@ static ngx_int_t ngx_http_upstream_init_ntlm(ngx_conf_t *cf, ngx_conf_init_uint_value(hncf->max_cached, 100); ngx_conf_init_msec_value(hncf->timeout, 60000); + ngx_conf_init_uint_value(hncf->requests, 1000); + ngx_conf_init_msec_value(hncf->time, 3600000); if (hncf->original_init_upstream(cf, us) != NGX_OK) { return NGX_ERROR; @@ -198,6 +213,12 @@ ngx_http_upstream_init_ntlm_peer(ngx_http_request_t *r, r->upstream->peer.get = ngx_http_upstream_get_ntlm_peer; r->upstream->peer.free = ngx_http_upstream_free_ntlm_peer; + hnpd->original_notify = NULL; + if (r->upstream->peer.notify) { + hnpd->original_notify = r->upstream->peer.notify; + r->upstream->peer.notify = ngx_http_upstream_ntlm_notify_peer; + } + #if (NGX_HTTP_SSL) hnpd->original_set_session = r->upstream->peer.set_session; hnpd->original_save_session = r->upstream->peer.save_session; @@ -322,6 +343,14 @@ static void ngx_http_upstream_free_ntlm_peer(ngx_peer_connection_t *pc, goto invalid; } + if (c->requests >= hndp->conf->requests) { + goto invalid; + } + + if (ngx_current_msec - c->start_time > hndp->conf->time) { + goto invalid; + } + if (hndp->ntlm_init == 0 && hndp->cached == 0) { goto invalid; } @@ -343,7 +372,9 @@ static void ngx_http_upstream_free_ntlm_peer(ngx_peer_connection_t *pc, item->in_cache = 0; item->peer_connection = NULL; - ngx_http_upstream_ntlm_close(old_pc); + if (old_pc != NULL) { + ngx_http_upstream_ntlm_close(old_pc); + } } else { q = ngx_queue_head(&hndp->conf->free); ngx_queue_remove(q); @@ -410,6 +441,7 @@ static void ngx_http_upstream_free_ntlm_peer(ngx_peer_connection_t *pc, * to upstream core (see ngx_http_upstream_get_ntlm_peer). */ c->read->data = item; + c->data = NULL; c->idle = 1; c->log = ngx_cycle->log; c->read->log = ngx_cycle->log; @@ -479,6 +511,15 @@ static void ngx_http_upstream_ntlm_dummy_handler(ngx_event_t *ev) { ngx_log_debug0(NGX_LOG_DEBUG_HTTP, ev->log, 0, "ntlm dummy handler"); } +static void ngx_http_upstream_ntlm_notify_peer(ngx_peer_connection_t *pc, + void *data, ngx_uint_t type) { + ngx_http_upstream_ntlm_peer_data_t *hndp = data; + + if (hndp->original_notify) { + hndp->original_notify(pc, hndp->data, type); + } +} + static void ngx_http_upstream_ntlm_close_handler(ngx_event_t *ev) { ngx_http_upstream_ntlm_srv_conf_t *conf; ngx_http_upstream_ntlm_cache_t *item; @@ -614,6 +655,8 @@ static void *ngx_http_upstream_ntlm_create_conf(ngx_conf_t *cf) { } conf->max_cached = NGX_CONF_UNSET_UINT; + conf->requests = NGX_CONF_UNSET_UINT; + conf->time = NGX_CONF_UNSET_MSEC; conf->timeout = NGX_CONF_UNSET_MSEC; return conf; From 5e00278eebf9d030be33eb040e5cfa1f80a9f2e0 Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Wed, 6 May 2026 09:16:36 +0000 Subject: [PATCH 4/4] refactor: reorder c->read->data/c->data restore in get_peer; rename requests->max_requests Agent-Logs-Url: https://github.com/Securepoint/nginx-ntlm-module/sessions/672b9196-95fe-4861-bee8-50b1cc5ff159 Co-authored-by: matthias-lay <163420385+matthias-lay@users.noreply.github.com> --- ngx_http_upstream_ntlm_module.c | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/ngx_http_upstream_ntlm_module.c b/ngx_http_upstream_ntlm_module.c index 3af1a2f..2a106ff 100644 --- a/ngx_http_upstream_ntlm_module.c +++ b/ngx_http_upstream_ntlm_module.c @@ -31,7 +31,7 @@ static void ngx_http_upstream_client_conn_cleanup(void *data); typedef struct { ngx_uint_t max_cached; - ngx_uint_t requests; + ngx_uint_t max_requests; ngx_msec_t time; ngx_msec_t timeout; ngx_queue_t free; @@ -89,7 +89,7 @@ static ngx_command_t ngx_http_upstream_ntlm_commands[] = { {ngx_string("ntlm_requests"), NGX_HTTP_UPS_CONF | NGX_CONF_TAKE1, ngx_conf_set_num_slot, NGX_HTTP_SRV_CONF_OFFSET, - offsetof(ngx_http_upstream_ntlm_srv_conf_t, requests), NULL}, + offsetof(ngx_http_upstream_ntlm_srv_conf_t, max_requests), NULL}, ngx_null_command /* command termination */ }; @@ -136,7 +136,7 @@ static ngx_int_t ngx_http_upstream_init_ntlm(ngx_conf_t *cf, ngx_conf_init_uint_value(hncf->max_cached, 100); ngx_conf_init_msec_value(hncf->timeout, 60000); - ngx_conf_init_uint_value(hncf->requests, 1000); + ngx_conf_init_uint_value(hncf->max_requests, 1000); ngx_conf_init_msec_value(hncf->time, 3600000); if (hncf->original_init_upstream(cf, us) != NGX_OK) { @@ -282,7 +282,6 @@ static ngx_int_t ngx_http_upstream_get_ntlm_peer(ngx_peer_connection_t *pc, c->idle = 0; c->sent = 0; - c->data = NULL; /* * Restore c->read->data to the connection pointer. While cached, * c->read->data held the ntlm cache item (see ngx_http_upstream_free_ntlm_peer). @@ -290,6 +289,7 @@ static ngx_int_t ngx_http_upstream_get_ntlm_peer(ngx_peer_connection_t *pc, * reset this before handing the connection back to upstream. */ c->read->data = c; + c->data = NULL; c->log = pc->log; c->read->log = pc->log; c->write->log = pc->log; @@ -343,7 +343,7 @@ static void ngx_http_upstream_free_ntlm_peer(ngx_peer_connection_t *pc, goto invalid; } - if (c->requests >= hndp->conf->requests) { + if (c->requests >= hndp->conf->max_requests) { goto invalid; } @@ -655,7 +655,7 @@ static void *ngx_http_upstream_ntlm_create_conf(ngx_conf_t *cf) { } conf->max_cached = NGX_CONF_UNSET_UINT; - conf->requests = NGX_CONF_UNSET_UINT; + conf->max_requests = NGX_CONF_UNSET_UINT; conf->time = NGX_CONF_UNSET_MSEC; conf->timeout = NGX_CONF_UNSET_MSEC;