feat: support OpenResty 1.29.2.4 patches#113
Merged
Merged
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAggregates OpenResty 1.29.2.4 changes: APISIX integration gates in nginx core, Lua/FFI keepalive pooling and TLS handshake/session APIs, shared-dict unification to ngx_meta_lua, extensive ngx_lua and ngx_stream_lua socket/TLS/xRPC refactors, privileged-agent listener support, and build/install wiring. ChangesHTTP Keepalive Pooling and Balancer Peer Selection
SSL/TLS Enhancements and Certificate Management
Core TLS Handshake Library and Socket Methods
APISIX Gateway Integration Points
Stream Lua Socket Extensions and xRPC Support
Shared Dictionary Consolidation to ngx_meta_lua
Coroutine Request Phase Exclusions & Filters
Process Architecture and Listening Socket Management
Nginx Core Utilities and Docs
Install/Script Update
🎯 4 (Complex) | ⏱️ ~60 minutes 🚥 Pre-merge checks | ✅ 6✅ Passed checks (6 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
Pull request overview
Adds a new patch set for OpenResty 1.29.2.4 and updates the patch application script so this repo can apply APISIX/OpenResty-specific deltas onto the bundled components (nginx 1.29.2, lua-resty-core 0.1.34rc2, ngx_lua 0.10.31rc2, ngx_stream_lua 0.0.19rc3).
Changes:
- Extend
patch/patch.shto detectopenresty-1.29.2.4and apply the matching patch directory. - Introduce the
patch/1.29.2.4/patch set covering nginx, ngx_lua, ngx_stream_lua, and lua-resty-core updates (TLS handshake FFI, shdict changes, keepalive/balancer changes, APISIX hooks, etc.). - Add/adjust APISIX-specific integration points in upstream nginx/lua modules (e.g., gzip control, mirror on-demand, upstream mTLS toggles, trailer passing, privileged agent listeners, original dst var, client_max_body_size delay logic).
Reviewed changes
Copilot reviewed 35 out of 36 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| patch/patch.sh | Add OpenResty 1.29.2.4 detection and apply the new patch set to the correct bundled component versions. |
| patch/1.29.2.4/ngx_stream_lua-xrpc.patch | Stream cosocket/xrpc enhancements (read-within-bytes, new FFI read/send helpers, new FT_TRUNCATED flag). |
| patch/1.29.2.4/ngx_stream_lua-tlshandshake.patch | Stream TLS handshake moved to FFI API and internal handler refactors for session/error reporting. |
| patch/1.29.2.4/ngx_stream_lua-ssl_session_hostname.patch | Expose SSL session hostname via stream SSL certby FFI. |
| patch/1.29.2.4/ngx_stream_lua-shared_shdict.patch | Switch stream shdict plumbing to meta-lua helpers and update init handler typing/injection. |
| patch/1.29.2.4/ngx_stream_lua-reject-in-handshake.patch | Add reject_in_handshake behavior to stream client cert verification FFI. |
| patch/1.29.2.4/ngx_stream_lua-expose_request_struct.patch | Conditionally expose stream request struct headers for APISIX builds. |
| patch/1.29.2.4/ngx_lua-tlshandshake.patch | HTTP cosocket TLS handshake rename/refactor and session free API rename. |
| patch/1.29.2.4/ngx_lua-ssl_session_hostname.patch | Expose SSL session hostname via HTTP SSL certby FFI. |
| patch/1.29.2.4/ngx_lua-skip_filter.patch | Allow APISIX to skip header/body filters by lua when requested. |
| patch/1.29.2.4/ngx_lua-request_header_set.patch | Mark request-header-set operations for APISIX tracking. |
| patch/1.29.2.4/ngx_lua-reject-in-handshake.patch | Add reject_in_handshake flag to HTTP client cert verification FFI. |
| patch/1.29.2.4/ngx_lua-init_worker_coroutine.patch | Allow coroutine wrappers to run for init_worker context (adds ctx 0x100). |
| patch/1.29.2.4/ngx_lua-enable_keepalive.patch | Rework balancer keepalive pooling (per-pool userdata table keyed by crc32, pool sizing, lifecycle). |
| patch/1.29.2.4/nginx-upstream_pass_trailers.patch | Gate trailer forwarding behind an APISIX runtime check. |
| patch/1.29.2.4/nginx-upstream_mtls.patch | Let APISIX override upstream ssl verify behavior and store upstream SSL connection. |
| patch/1.29.2.4/nginx-tcp_over_tls.patch | Let APISIX enable stream proxy SSL dynamically; adjust ssl setup path. |
| patch/1.29.2.4/nginx-real_ip.patch | Export realip setter helper for APISIX and add a realip header dependency. |
| patch/1.29.2.4/nginx-proxy_request_buffering.patch | Let APISIX override proxy request buffering behavior dynamically. |
| patch/1.29.2.4/nginx-privileged_agent_process_thread.patch | Ensure thread-pool worker exit also runs for privileged agent when enabled. |
| patch/1.29.2.4/nginx-mirror.patch | Add APISIX “mirror on demand” directive and runtime enablement check. |
| patch/1.29.2.4/nginx-listen-in-privileged-agent.patch | Allow binding/listening sockets specifically in privileged agent process. |
| patch/1.29.2.4/nginx-gzip.patch | Let APISIX override gzip enablement/level/buffer sizing and gzip http version gating. |
| patch/1.29.2.4/nginx-grpc_set_header_authority.patch | Treat explicit :authority header as host_set under APISIX build. |
| patch/1.29.2.4/nginx-get_last_reopen_time.patch | Track last log reopen timestamp (ms) for APISIX and expose accessor. |
| patch/1.29.2.4/nginx-error_page_contains_apisix.patch | Replace 50x page content and add “Powered by APISIX” tail to special responses. |
| patch/1.29.2.4/nginx-enable_ntls.patch | Enable NTLS in handshake when APISIX and TongSuO runtime setting is enabled. |
| patch/1.29.2.4/nginx-connection-original-dst.patch | Add Linux netfilter feature detection and $connection_original_dst variable. |
| patch/1.29.2.4/nginx-client_max_body_size.patch | Allow APISIX to delay/max-body-size enforcement and apply it during body read/V2 filtering. |
| patch/1.29.2.4/lua-resty-core-tlshandshake.patch | Add resty.core.socket.tcp to implement FFI-based tlshandshake and re-add sslhandshake wrapper. |
| patch/1.29.2.4/lua-resty-core-ssl_session_hostname.patch | Add ngx.ssl.session_hostname() binding via new FFI entrypoints. |
| patch/1.29.2.4/lua-resty-core-shared_shdict.patch | Switch shdict FFI bindings to meta-lua symbols and simplify subsystem branching (incl. macOS path). |
| patch/1.29.2.4/lua-resty-core-reject-in-handshake.patch | Change ngx.ssl.verify_client signature to accept reject_in_handshake (default true) and pass it to FFI. |
| patch/1.29.2.4/lua-resty-core-init_worker_coroutine.patch | Mirror coroutine init_worker context allowance (adds ctx 0x100). |
| patch/1.29.2.4/lua-resty-core-enable_keepalive.patch | Update balancer API to accept an opts table (pool/pool_size) and pass pool info to FFI in HTTP subsystem. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
Note
Due to the large number of review comments, Critical severity comments were prioritized as inline comments.
🟠 Major comments (18)
patch/1.29.2.4/nginx-listen-in-privileged-agent.patch-9-11 (1)
9-11:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy liftKeep privileged-agent binds in the startup validation path.
These lines make
ngx_open_listening_sockets()skip everyprivileged_agentlistener until the privileged-agent process is already running. That means startup/config-test no longer probes whether those sockets can actually bind, so a port conflict is discovered only later inngx_privileged_agent_process_cycle()as a late helper exit instead of an early configuration failure.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@patch/1.29.2.4/nginx-listen-in-privileged-agent.patch` around lines 9 - 11, The patch causes ngx_open_listening_sockets to skip listeners marked privileged_agent by checking ngx_is_privileged_agent != ls[i].privileged_agent, which prevents startup/config-test from validating those binds; to fix, revert that conditional so privileged_agent listeners are included during startup validation (remove or adjust the ngx_is_privileged_agent != ls[i].privileged_agent check in ngx_open_listening_sockets) so binding errors are caught early rather than later in ngx_privileged_agent_process_cycle.patch/1.29.2.4/nginx-real_ip.patch-40-52 (1)
40-52:⚠️ Potential issue | 🟠 Major | ⚡ Quick winGuard the function declaration with the same conditional as the implementation.
The function declaration at line 49 is unconditional, but the implementation in
ngx_http_realip_module.c(lines 22-28) is only compiled whenNGX_HTTP_APISIXis defined. This mismatch will cause linkage errors if code attempts to call this function whenNGX_HTTP_APISIXis not enabled.🔧 Proposed fix
`#include` <ngx_config.h> `#include` <ngx_core.h> `#include` <ngx_http.h> +#if (NGX_HTTP_APISIX) ngx_int_t ngx_http_realip_set_real_addr(ngx_http_request_t *r, ngx_addr_t *addr); +#endif `#endif` /* _NGX_HTTP_REALIP_H_INCLUDED_ */🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@patch/1.29.2.4/nginx-real_ip.patch` around lines 40 - 52, The header currently declares ngx_http_realip_set_real_addr unconditionally; wrap that declaration in the same `#if/`#endif conditional used around its implementation (i.e., the NGX_HTTP_APISIX guard) so the prototype only exists when NGX_HTTP_APISIX is defined; locate the declaration of ngx_http_realip_set_real_addr in the header and surround it with the same `#if` defined(NGX_HTTP_APISIX) ... `#endif` used in the implementation (keeping the existing include guards intact).patch/1.29.2.4/nginx-get_last_reopen_time.patch-29-32 (1)
29-32:⚠️ Potential issue | 🟠 Major | ⚡ Quick winPotential integer overflow in millisecond calculation.
At line 31,
tp->sec * 1000 + tp->msecis assigned tongx_last_reopen_msec(angx_uint_t, typically 32-bit unsigned). Multiplying seconds by 1000 will overflow after approximately 49 days of uptime (2^32 / 1000 / 86400 ≈ 49.7 days).Consider using
ngx_msec_tinstead ofngx_uint_t, or document the overflow behavior if wraparound is acceptable.🔧 Proposed fix
Change the global variable type from
ngx_uint_ttongx_msec_t:sig_atomic_t ngx_reopen; `#if` (NGX_HTTP_APISIX) -ngx_uint_t ngx_last_reopen_msec; +ngx_msec_t ngx_last_reopen_msec; `#endif`And update the accessor return type:
`#if` (NGX_HTTP_APISIX) -ngx_uint_t +ngx_msec_t ngx_worker_process_get_last_reopen_ms()🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@patch/1.29.2.4/nginx-get_last_reopen_time.patch` around lines 29 - 32, The computation tp->sec * 1000 + tp->msec used to set ngx_last_reopen_msec can overflow because ngx_last_reopen_msec is currently a ngx_uint_t; change the storage and any accessors to a millisecond type (use ngx_msec_t) so the multiplication cannot overflow for long uptimes, update the global declaration of ngx_last_reopen_msec from ngx_uint_t to ngx_msec_t and adjust the accessor function return type and any callers that assume ngx_uint_t, keeping the assignment using ngx_timeofday()'s tp->sec and tp->msec unchanged.patch/1.29.2.4/nginx-connection-original-dst.patch-47-50 (1)
47-50:⚠️ Potential issue | 🟠 Major | ⚡ Quick winMark the variable as non-cacheable.
The
connection_original_dstvariable is connection-specific and should not be cached. The registration should include theNGX_HTTP_VAR_NOCACHEABLEflag, similar toconnection_timeat line 44.🔧 Proposed fix
`#if` (NGX_HAVE_NETFILTER_IPV4) { ngx_string("connection_original_dst"), NULL, - ngx_http_variable_connection_dst, 0, 0, 0 }, + ngx_http_variable_connection_dst, 0, NGX_HTTP_VAR_NOCACHEABLE, 0 }, `#endif`🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@patch/1.29.2.4/nginx-connection-original-dst.patch` around lines 47 - 50, The variable registration for connection_original_dst must be marked non-cacheable: update the ngx_http_variable_t entry that registers "connection_original_dst" (the line using ngx_http_variable_connection_dst) to include the NGX_HTTP_VAR_NOCACHEABLE flag (same way connection_time is declared) so the variable is treated as connection-specific and not cached by NGINX.patch/1.29.2.4/nginx-connection-original-dst.patch-59-93 (1)
59-93:⚠️ Potential issue | 🟠 Major | ⚡ Quick winFix cacheability flag in variable handler.
Line 87 sets
v->no_cacheable = 0, marking the value as cacheable. Since this is connection-specific data, it should bev->no_cacheable = 1to prevent incorrect values from being served.🔧 Proposed fix
v->len = ngx_sock_ntop((struct sockaddr *) &dst, socklen, p, NGX_SOCKADDR_STRLEN, dst.sin_port); v->valid = 1; - v->no_cacheable = 0; + v->no_cacheable = 1; v->not_found = 0; v->data = p;🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@patch/1.29.2.4/nginx-connection-original-dst.patch` around lines 59 - 93, The variable handler ngx_http_variable_connection_dst incorrectly marks the result as cacheable by setting v->no_cacheable = 0; change this to v->no_cacheable = 1 so the connection-specific original destination string is not cached; update the assignment in ngx_http_variable_connection_dst and keep the rest of the initialization (v->len, v->valid, v->not_found, v->data) unchanged.patch/1.29.2.4/ngx_lua-enable_keepalive.patch-493-520 (1)
493-520:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy liftPool lookup needs a collision-free, config-scoped key.
These pools are indexed only by
cpool_crc32, while the old per-lscf/sockaddr matching has been removed. That means two different pool names with the same CRC32 — or the same hash reused from another upstream config — can hand out the same idle socket with no secondary check. Please scope the key with stable upstream identity and keep the original pool name (or equivalent) for collision verification before reusing a cached connection.Also applies to: 541-563
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@patch/1.29.2.4/ngx_lua-enable_keepalive.patch` around lines 493 - 520, The pool lookup currently keys pools only by cpool_crc32 (see balancer_keepalive_pools_table_key, lua_rawget/lua_rawseti usage and upool), which risks CRC collisions across different upstream configs; change the key to a collision-free, config-scoped composite (for example combine a stable upstream identity like lscf pointer or upstream name and the pool name/hash) when pushing/reading from the registry (where lua_pushlightuserdata and lua_rawget/lua_rawseti are used), store the original pool name/identity inside upool (add a pool_name or upstream_id field) and on pool reuse verify the stored name/identity matches the requested one before handing out a cached connection; apply the same fix to the other block referenced (lines ~541-563) that does similar lookup/insert.patch/1.29.2.4/lua-resty-core-enable_keepalive.patch-45-79 (1)
45-79:⚠️ Potential issue | 🟠 Major | ⚡ Quick winPreserve the existing
host/SNI override path.This makes
set_current_peer(addr, port, host)a hard error, and the companion C change removes the only place that copied a replacement host into the peer state. Existing balancer code that selects an IP peer but still needs a different TLS hostname/SNI will now either break immediately or silently lose that override. Please keep the legacy string form working, or add anopts.hostequivalent before changing the public API.Also applies to: 114-124
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@patch/1.29.2.4/lua-resty-core-enable_keepalive.patch` around lines 45 - 79, The change to _M.set_current_peer removed the previous third-argument host/SNI override and thus breaks callers using set_current_peer(addr, port, host); restore backwards-compatibility by accepting either the legacy string third argument or the new opts table with an opts.host field: if the third parameter is a string treat it as host (and set the peer host/SNI replacement as before), otherwise if a table read opts.host in addition to opts.pool and opts.pool_size; ensure you still compute pool_crc32 with ngx_crc32_long(pool) and preserve the logic that copies the replacement host into the peer state so existing balancer code keeps its host/SNI override.patch/1.29.2.4/lua-resty-core-reject-in-handshake.patch-36-45 (1)
36-45:⚠️ Potential issue | 🟠 Major | ⚡ Quick winKeep
verify_client()backward compatible by default.Defaulting
reject_in_handshaketotruesilently changes every existing 3-argumentverify_client()call from post-handshake handling to handshake-time rejection. That is a behavior break across both subsystems; the new mode should be explicit, or the default should stayfalse.Proposed fix
- if reject_in_handshake == nil then - -- reject by default so we can migrate to the new behavior - -- without modifying Lua code - reject_in_handshake = true - end + if reject_in_handshake == nil then + reject_in_handshake = false + end🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@patch/1.29.2.4/lua-resty-core-reject-in-handshake.patch` around lines 36 - 45, The patch currently defaults reject_in_handshake to true, changing the default behavior of verify_client(); revert this to preserve backward compatibility by default: in the verify_client implementation, set reject_in_handshake to false when nil, keep the conversion to reject_in_handshake_int and pass that to ngx_lua_ffi_ssl_verify_client (symbols to edit: function verify_client, variable reject_in_handshake, reject_in_handshake_int, and the call to ngx_lua_ffi_ssl_verify_client).patch/1.29.2.4/ngx_stream_lua-ssl_session_hostname.patch-35-39 (1)
35-39:⚠️ Potential issue | 🟠 Major | ⚡ Quick winGuard the
SSL_get0_session()result before passing it to OpenSSL.If
SSL_get0_session(ssl_conn)returnsNULL(which can occur if the handshake is incomplete, the session has been removed, or session resumption is not active), callingSSL_SESSION_get0_hostname()on it will cause undefined behavior since the function does not acceptNULLper OpenSSL documentation.Proposed fix
- *name = (char *) SSL_SESSION_get0_hostname(SSL_get0_session(ssl_conn)); + if (SSL_get0_session(ssl_conn) == NULL) { + *name = ""; + *namelen = 0; + return NGX_OK; + } + + *name = (char *) SSL_SESSION_get0_hostname(SSL_get0_session(ssl_conn));🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@patch/1.29.2.4/ngx_stream_lua-ssl_session_hostname.patch` around lines 35 - 39, Guard the SSL_get0_session() result before calling SSL_SESSION_get0_hostname: call SSL_get0_session(ssl_conn) into a local variable (e.g., sess), check if sess is NULL and if so set *name = NULL, *namelen = 0 and return an appropriate NGX status (e.g., NGX_DECLINED or NGX_ERROR) instead of calling SSL_SESSION_get0_hostname on NULL; only call SSL_SESSION_get0_hostname(sess) and compute ngx_strlen(*name) when sess is non-NULL, keeping the existing use of ssl_conn, name, namelen and returning NGX_OK on success.patch/1.29.2.4/ngx_lua-ssl_session_hostname.patch-35-39 (1)
35-39:⚠️ Potential issue | 🟠 Major | ⚡ Quick winGuard
SSL_get0_session()before passing toSSL_SESSION_get0_hostname().
SSL_get0_session(ssl_conn)can legitimately return NULL before the handshake completes, after session invalidation, or if no session was established. Per OpenSSL's API contract,SSL_SESSION_get0_hostname()does not accept NULL pointers. The nested call here creates undefined behavior when no session exists, instead of falling through to the safe empty-string fallback already present on lines 43–45.Proposed fix
+ if (SSL_get0_session(ssl_conn) == NULL) { + *name = ""; + *namelen = 0; + return NGX_OK; + } + *name = (char *) SSL_SESSION_get0_hostname(SSL_get0_session(ssl_conn));🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@patch/1.29.2.4/ngx_lua-ssl_session_hostname.patch` around lines 35 - 39, The nested call SSL_SESSION_get0_hostname(SSL_get0_session(ssl_conn)) can dereference a NULL session; store SSL_get0_session(ssl_conn) into a local SSL_SESSION* variable, check that it is non-NULL before calling SSL_SESSION_get0_hostname, and only set *name and *namelen and return NGX_OK when both the session and hostname are non-NULL; otherwise fall through to the existing empty-string fallback handling for *name/*namelen.patch/1.29.2.4/ngx_stream_lua-tlshandshake.patch-421-429 (1)
421-429:⚠️ Potential issue | 🟠 Major | ⚡ Quick winReturn an error when the post-handshake checks fail synchronously.
After
ngx_stream_lua_ssl_handshake_handler(c)runs, the real outcome may already be inu->error_reteven whenngx_ssl_handshake()returnedNGX_OK(for example, cert verify or hostname mismatch). ReturningNGX_OKhere breaks the new Lua fast path forreused_session == false, which treatsFFI_OKas success and never callsget_tlshandshake_result().Suggested fix
ngx_stream_lua_ssl_handshake_handler(c); - if (rc == NGX_ERROR) { + if (rc == NGX_ERROR || u->error_ret != NULL) { *errmsg = u->error_ret; return NGX_ERROR; } return NGX_OK;🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@patch/1.29.2.4/ngx_stream_lua-tlshandshake.patch` around lines 421 - 429, After calling ngx_stream_lua_ssl_handshake_handler(c), ensure you check the handshake outcome stored in rc and the connection userdata error (u->error_ret) and propagate it: if rc == NGX_ERROR set *errmsg = u->error_ret and return NGX_ERROR so synchronous post-handshake failures (cert/hostname verification) are reported; otherwise return NGX_OK — this prevents treating FFI_OK as success in the reused_session fast path and ensures get_tlshandshake_result() sees the real error.patch/1.29.2.4/ngx_stream_lua-tlshandshake.patch-315-329 (1)
315-329:⚠️ Potential issue | 🟠 Major | ⚡ Quick winRestore the OCSP setup failure check.
The new code removes the error check for
SSL_set_tlsext_status_type(), which returns 1 on success and 0 on failure. Without this check,ocsp_status_req=truecan silently fail to enable OCSP stapling and fall back to a plain TLS handshake without raising an error.The suggested fix correctly restores the check while adapting to the new error handling pattern (
*errmsgandNGX_ERRORinstead ofluaL_error).🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@patch/1.29.2.4/ngx_stream_lua-tlshandshake.patch` around lines 315 - 329, The OCSP setup removed the return-value check for SSL_set_tlsext_status_type causing silent failures; restore the check in the ocsp_status_req branch by calling SSL_set_tlsext_status_type(c->ssl->connection, TLSEXT_STATUSTYPE_ocsp) and if it does not return 1 set *errmsg = "failed to enable OCSP stapling" and return NGX_ERROR (use the same error-reporting pattern as the surrounding code that assigns to errmsg and returns NGX_ERROR); reference ocsp_status_req, SSL_set_tlsext_status_type, TLSEXT_STATUSTYPE_ocsp, *errmsg and NGX_ERROR to locate and fix the code.patch/1.29.2.4/ngx_stream_lua-xrpc.patch-111-124 (1)
111-124:⚠️ Potential issue | 🟠 Major | ⚡ Quick winReturn immediately from the busy-write guard.
This macro only populates
errbufand then falls through.ngx_stream_lua_ffi_socket_tcp_send_from_socket()will still continue on a socket that is already writing, which can overwrite the in-flight write state.Suggested fix
`#define` ngx_stream_lua_ffi_socket_check_busy_writing(r, u, errbuf, \ errbuf_size) \ if ((u)->write_waiting) { \ *errbuf_size = ngx_snprintf((errbuf), *(errbuf_size), \ "socket busy writing") \ - (errbuf); \ + return NGX_ERROR; \ } \ if ((u)->raw_downstream \ && ((r)->connection->buffered)) \ { \ *errbuf_size = ngx_snprintf((errbuf), *(errbuf_size), \ "socket busy writing") \ - (errbuf); \ + return NGX_ERROR; \ }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@patch/1.29.2.4/ngx_stream_lua-xrpc.patch` around lines 111 - 124, The macro ngx_stream_lua_ffi_socket_check_busy_writing currently only fills errbuf/errbuf_size and falls through, so callers like ngx_stream_lua_ffi_socket_tcp_send_from_socket continue and can corrupt in-flight writes; change the macro so that after populating errbuf and updating *errbuf_size it returns immediately (e.g., return NGX_AGAIN) from the calling function to stop further processing when (u)->write_waiting or (u)->raw_downstream && (r)->connection->buffered is true.patch/1.29.2.4/ngx_stream_lua-xrpc.patch-176-184 (1)
176-184:⚠️ Potential issue | 🟠 Major | ⚡ Quick winSplice the whole prefix into
free_recv_bufs, not just the head link.This only rewires
u->bufs_in->nextbefore switchingu->bufs_intou->buf_in. For a chain likeA -> B -> Cwithu->buf_in == C,Bbecomes unreachable. That leaks receive buffers and breaks later reuse.Suggested fix
- if (cl->next) { - ll = &cl->next; - } - - if (ll) { - *ll = ctx->free_recv_bufs; - ctx->free_recv_bufs = u->bufs_in; - u->bufs_in = u->buf_in; - } + if (u->bufs_in != u->buf_in) { + for (cl = u->bufs_in; cl->next && cl->next != u->buf_in; cl = cl->next) { + /* find the node right before u->buf_in */ + } + + if (cl->next == u->buf_in) { + cl->next = ctx->free_recv_bufs; + ctx->free_recv_bufs = u->bufs_in; + u->bufs_in = u->buf_in; + } + }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@patch/1.29.2.4/ngx_stream_lua-xrpc.patch` around lines 176 - 184, The code only rewires cl->next (via ll) which detaches the middle of the u->bufs_in chain; instead find the tail of the prefix (the node whose next == u->buf_in), set that tail->next = ctx->free_recv_bufs so the entire prefix chain is spliced, then set ctx->free_recv_bufs = u->bufs_in and finally u->bufs_in = u->buf_in; use the existing cl/ll traversal to locate that tail and update its next pointer (not just a single link) so nodes between u->bufs_in and u->buf_in aren't lost.patch/1.29.2.4/lua-resty-core-shared_shdict.patch-203-213 (1)
203-213:⚠️ Potential issue | 🟠 Major | ⚡ Quick winSeparate required FFI bindings from optional ones.
Lines 203–213 wrap both required and optional FFI bindings in a single
pcall. WhenC.ngx_meta_lua_ffi_shdict_free_spaceis absent, thepcallexits beforengx_lua_ffi_shdict_udata_to_zoneis assigned at line 212, even though it is a required symbol available on all supported builds. This breaks zone resolution.Extract
ngx_lua_ffi_shdict_udata_to_zone(line 212) and the required bindings (lines 204–210) outside thepcall, leaving onlyngx_lua_ffi_shdict_free_space(line 211) inside it.Suggested fix
-pcall(function () - ngx_lua_ffi_shdict_get = C.ngx_meta_lua_ffi_shdict_get - ngx_lua_ffi_shdict_incr = C.ngx_meta_lua_ffi_shdict_incr - ngx_lua_ffi_shdict_store = C.ngx_meta_lua_ffi_shdict_store - ngx_lua_ffi_shdict_flush_all = C.ngx_meta_lua_ffi_shdict_flush_all - ngx_lua_ffi_shdict_get_ttl = C.ngx_meta_lua_ffi_shdict_get_ttl - ngx_lua_ffi_shdict_set_expire = C.ngx_meta_lua_ffi_shdict_set_expire - ngx_lua_ffi_shdict_capacity = C.ngx_meta_lua_ffi_shdict_capacity - ngx_lua_ffi_shdict_free_space = C.ngx_meta_lua_ffi_shdict_free_space - ngx_lua_ffi_shdict_udata_to_zone = C.ngx_meta_lua_ffi_shdict_udata_to_zone -end) +ngx_lua_ffi_shdict_get = C.ngx_meta_lua_ffi_shdict_get +ngx_lua_ffi_shdict_incr = C.ngx_meta_lua_ffi_shdict_incr +ngx_lua_ffi_shdict_store = C.ngx_meta_lua_ffi_shdict_store +ngx_lua_ffi_shdict_flush_all = C.ngx_meta_lua_ffi_shdict_flush_all +ngx_lua_ffi_shdict_get_ttl = C.ngx_meta_lua_ffi_shdict_get_ttl +ngx_lua_ffi_shdict_set_expire = C.ngx_meta_lua_ffi_shdict_set_expire +ngx_lua_ffi_shdict_capacity = C.ngx_meta_lua_ffi_shdict_capacity +ngx_lua_ffi_shdict_udata_to_zone = C.ngx_meta_lua_ffi_shdict_udata_to_zone + +pcall(function () + ngx_lua_ffi_shdict_free_space = C.ngx_meta_lua_ffi_shdict_free_space +end)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@patch/1.29.2.4/lua-resty-core-shared_shdict.patch` around lines 203 - 213, The pcall currently wraps both optional and required FFI bindings so if an optional symbol (e.g. C.ngx_meta_lua_ffi_shdict_free_space) is missing it prevents assignment of required symbols like ngx_lua_ffi_shdict_udata_to_zone and the other required bindings; fix by assigning the required bindings (ngx_lua_ffi_shdict_get, ngx_lua_ffi_shdict_incr, ngx_lua_ffi_shdict_store, ngx_lua_ffi_shdict_flush_all, ngx_lua_ffi_shdict_get_ttl, ngx_lua_ffi_shdict_set_expire, ngx_lua_ffi_shdict_capacity, and ngx_lua_ffi_shdict_udata_to_zone) directly (outside any pcall) and keep only the truly optional binding (C.ngx_meta_lua_ffi_shdict_free_space -> ngx_lua_ffi_shdict_free_space) inside a pcall so missing optional symbols don’t prevent required ones from being set.patch/1.29.2.4/nginx-gzip.patch-132-135 (1)
132-135:⚠️ Potential issue | 🟠 Major | ⚡ Quick winDon't bypass
gzip_http_versionwhen APISIX enables gzip.Lines 132-135 make the per-request gzip override ignore the configured protocol floor entirely, so an HTTP/1.0 request can now be compressed even when
gzip_http_versionis still1.1. If APISIX needs to control that too, it should be a separate per-request hook rather than skipping the core guard.Suggested fix
- if (!ngx_http_apisix_is_gzip_set(r) - && r->http_version < clcf->gzip_http_version) { + if (r->http_version < clcf->gzip_http_version) { return NGX_DECLINED; }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@patch/1.29.2.4/nginx-gzip.patch` around lines 132 - 135, The new conditional bypasses the core gzip_http_version guard when ngx_http_apisix enables per-request gzip, allowing HTTP/1.0 to be compressed even if clcf->gzip_http_version is > 1.0; change the logic so the protocol-version check always runs first (if r->http_version < clcf->gzip_http_version return NGX_DECLINED) and do not short-circuit that check based on ngx_http_apisix_is_gzip_set(r); if APISIX needs to override protocol policy, implement that as a separate per-request hook instead of skipping the core guard.patch/1.29.2.4/nginx-gzip.patch-30-35 (1)
30-35:⚠️ Potential issue | 🟠 Major | ⚡ Quick winKeep
gzip_min_lengthandgzip_typesenforcement outside the APISIX enable check.Lines 30-35 currently let any APISIX-enabled request skip both static filters, but this patch does not add per-request replacements for either knob. That broadens gzip beyond the delegated state/level/buffer behavior and will start compressing tiny or otherwise excluded responses.
Suggested fix
- if (!ngx_http_apisix_is_gzip_set(r) - && (!conf->enable - || (r->headers_out.content_length_n != -1 - && r->headers_out.content_length_n < conf->min_length) - || ngx_http_test_content_type(r, &conf->types) == NULL)) + if ((!ngx_http_apisix_is_gzip_set(r) && !conf->enable) + || (r->headers_out.content_length_n != -1 + && r->headers_out.content_length_n < conf->min_length) + || ngx_http_test_content_type(r, &conf->types) == NULL) { return ngx_http_next_header_filter(r); }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@patch/1.29.2.4/nginx-gzip.patch` around lines 30 - 35, The gzip enablement check currently short-circuits both the gzip_min_length and gzip_types checks; update the conditional in the gzip decision logic so ngx_http_apisix_is_gzip_set(r) (or conf->enable) governs only the APISIX-specific enablement, while the static filters—checking r->headers_out.content_length_n against conf->min_length (gzip_min_length) and ngx_http_test_content_type(r, &conf->types) (gzip_types)—remain enforced for every request; in practice, reorder/split the condition around ngx_http_apisix_is_gzip_set(r) so that conf->min_length and ngx_http_test_content_type are evaluated even when APISIX is enabled.patch/1.29.2.4/nginx-client_max_body_size.patch-113-118 (1)
113-118:⚠️ Potential issue | 🟠 Major | ⚡ Quick winRemove the unused cast on
clcfin the APISIX branch—it evaluates an uninitialized variable.In the APISIX conditional block (line 116),
(void) clcf;attempts to reference a variable that is never assigned in this branch. In C, accessing an uninitialized automatic variable triggers undefined behavior, even when casting to void—the cast only discards the result; it does not prevent the evaluation itself. Either remove the cast entirely or hoist thengx_http_get_module_loc_conf()assignment above the#ifdirective.Suggested fix
`#if` (NGX_HTTP_APISIX) off_t max_body_size = ngx_http_apisix_client_max_body_size(r); - (void) clcf; /* unused */ - if (max_body_size && rb->received > max_body_size) `#else` clcf = ngx_http_get_module_loc_conf(r, ngx_http_core_module);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@patch/1.29.2.4/nginx-client_max_body_size.patch` around lines 113 - 118, The APISIX branch is casting the uninitialized local clcf via "(void) clcf;" which evaluates an uninitialized variable; remove that cast (or alternatively move the ngx_http_get_module_loc_conf(...) assignment for clcf above the `#if` to ensure clcf is initialized before any use). Locate the APISIX block around ngx_http_apisix_client_max_body_size(r) and either delete the "(void) clcf;" line or hoist the ngx_http_get_module_loc_conf(...) call so clcf is properly set before the conditional branch.
🟡 Minor comments (3)
patch/1.29.2.4/ngx_lua-request_header_set.patch-19-24 (1)
19-24:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winAvoid side effects before bad-context rejection for fake requests.
Line 20 marks header mutation before the fake-request guard on Line 23. This can mark state even when the function returns
NGX_HTTP_LUA_FFI_BAD_CONTEXT. Move the mark call after the fake-request check.Proposed fix
`#if` (NGX_HTTP_APISIX) - ngx_http_apisix_mark_request_header_set(r); -#endif - if (r->connection->fd == (ngx_socket_t) -1) { /* fake request */ return NGX_HTTP_LUA_FFI_BAD_CONTEXT; } + +#if (NGX_HTTP_APISIX) + ngx_http_apisix_mark_request_header_set(r); +#endif🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@patch/1.29.2.4/ngx_lua-request_header_set.patch` around lines 19 - 24, The call to ngx_http_apisix_mark_request_header_set currently happens before the fake-request guard (checking r->connection->fd against (ngx_socket_t)-1) and can create side effects even when the function returns NGX_HTTP_LUA_FFI_BAD_CONTEXT; move the ngx_http_apisix_mark_request_header_set(r) invocation to immediately after the fake-request check (i.e., after the early return for r->connection->fd == (ngx_socket_t)-1) so no header mutation is marked for fake requests.patch/1.29.2.4/nginx-get_last_reopen_time.patch-61-67 (1)
61-67:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winFunction declaration should use
(void)parameter list.Line 63 declares the function with an empty parameter list
(). In C, this means the function can accept any number of arguments. Use(void)to explicitly indicate no parameters.🔧 Proposed fix
`#if` (NGX_HTTP_APISIX) ngx_uint_t -ngx_worker_process_get_last_reopen_ms() +ngx_worker_process_get_last_reopen_ms(void) { return ngx_last_reopen_msec; }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@patch/1.29.2.4/nginx-get_last_reopen_time.patch` around lines 61 - 67, Change the function signature of ngx_worker_process_get_last_reopen_ms to explicitly take no parameters by replacing the empty parameter list "()" with "(void)"; update the definition (ngx_worker_process_get_last_reopen_ms) accordingly so it reads "ngx_uint_t ngx_worker_process_get_last_reopen_ms(void)" (keep the body and `#if` NGX_HTTP_APISIX wrapper unchanged).patch/1.29.2.4/lua-resty-core-tlshandshake.patch-302-306 (1)
302-306:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winReject extra positional args in
sslhandshake().Line 303 only errors when
n > 1, so a 6-argument call is accepted and the last argument is silently ignored. This wrapper advertises the legacy 1–5 argument shape, so any non-empty...should fail fast.Suggested fix
- if not self or n > 1 then + if not self or n > 0 then error("ngx.socket sslhandshake: expecting 1 ~ 5 arguments " .. "(including the object), but seen " .. (self and 5 + n or 0)) end🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@patch/1.29.2.4/lua-resty-core-tlshandshake.patch` around lines 302 - 306, The guard in sslhandshake currently allows one extra positional arg (uses n > 1), so change the check to reject any non-empty varargs by using n > 0 (i.e. if not self or n > 0 then error), and update the "seen" count expression from (self and 5 + n or 0) to (self and 1 + n or 0) so the error message reports the actual total arguments seen; adjust only the condition and the seen-count expression in the sslhandshake wrapper (variables: n, self, sslhandshake).
🧹 Nitpick comments (3)
patch/1.29.2.4/ngx_lua-init_worker_coroutine.patch (1)
8-8: ⚡ Quick winConsider updating the comment to reflect all excluded phases.
The comment states
/* ignore header and body filters */, which accurately describes the original two exclusions (0x020 and 0x040), but now a third context value (0x100) is being excluded without corresponding documentation.📝 Suggested comment update
- /* ignore header and body filters */ + /* ignore header, body filters, and 0x100 phase */Or, if 0x100's meaning is known:
- /* ignore header and body filters */ + /* ignore header, body, and [phase_name] filters */🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@patch/1.29.2.4/ngx_lua-init_worker_coroutine.patch` at line 8, The inline comment "/* ignore header and body filters */" no longer matches the excluded context values — update that comment where the context mask excludes 0x020, 0x040 and the newly excluded 0x100 to accurately describe all excluded phases (either expand to "/* ignore header, body and <phase-name-for-0x100> filters */" or to a generic "/* ignore header, body and additional filter (0x100) */" if the phase name is unknown), so the comment matches the mask values 0x020, 0x040 and 0x100.patch/1.29.2.4/nginx-enable_ntls.patch (1)
19-22: ⚡ Quick winAdd error handling for SSL_enable_ntls return value.
Line 21 calls
SSL_enable_ntls(c->ssl->connection)without checking the return value. While the exact API contract for this function is not publicly documented, Tongsuo's NTLS state-machine functions typically return success/failure status. If SSL_enable_ntls follows this pattern, the return value should be checked and handshake initialization should fail gracefully if NTLS enabling fails, rather than proceeding silently.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@patch/1.29.2.4/nginx-enable_ntls.patch` around lines 19 - 22, The call to SSL_enable_ntls(c->ssl->connection) must check its return value and fail the handshake if enabling NTLS fails; update the block guarded by ngx_http_apisix_is_ntls_enabled(hc->conf_ctx) to capture the result of SSL_enable_ntls, log or propagate an error when it returns a failure code, and return an appropriate error/terminate the connection initialization path instead of continuing silently so that handshake initialization does not proceed on failure.patch/1.29.2.4/nginx-connection-original-dst.patch (1)
64-72: 🏗️ Heavy liftImplementation is limited to IPv4 via Linux netfilter.
The code uses
struct sockaddr_inandSOL_IPwithSO_ORIGINAL_DST, restricting support to IPv4. This is by design—the underlying Linux netfilter API (netfilter_ipv4) is IPv4-specific, and IPv6 would require a different kernel interface entirely.The feature is properly guarded with
#if (NGX_HAVE_NETFILTER_IPV4), so the limitation is explicit and compile-time scoped. IPv6 support is not currently a stated requirement in the project.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@patch/1.29.2.4/nginx-connection-original-dst.patch` around lines 64 - 72, The code uses IPv4-specific netfilter calls (struct sockaddr_in, SOL_IP and SO_ORIGINAL_DST via getsockopt) which restricts it to IPv4; ensure this is explicitly compile-time guarded by the NGX_HAVE_NETFILTER_IPV4 macro and add a short comment near the block (referencing getsockopt, SO_ORIGINAL_DST, SOL_IP and struct sockaddr_in) stating the IPv4-only limitation and that IPv6 requires a different kernel interface so readers aren’t misled; if the surrounding code already has the `#if` (NGX_HAVE_NETFILTER_IPV4) guard, verify it encloses this exact block and no IPv6 symbols are introduced outside that guard.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@patch/1.29.2.4/lua-resty-core-shared_shdict.patch`:
- Around line 220-247: The FFI function declarations still reference the old
typedef names (ngx_http_lua_shdict_get_params_t,
ngx_http_lua_shdict_store_params_t, ngx_http_lua_shdict_incr_params_t) but the
typedefs were renamed to ngx_meta_lua_shdict_get_params_t /
ngx_meta_lua_shdict_store_params_t / ngx_meta_lua_shdict_incr_params_t; update
the parameter types in the function prototypes
ngx_meta_lua_ffi_shdict_get_macos, ngx_meta_lua_ffi_shdict_store_macos, and
ngx_meta_lua_ffi_shdict_incr_macos to use the new ngx_meta_lua_shdict_*_params_t
names so the cdef types match the typedefs and allocated structs.
In `@patch/1.29.2.4/nginx-listen-in-privileged-agent.patch`:
- Around line 35-51: The privileged-agent filtering (checks involving
ngx_is_privileged_agent and ls[i].privileged_agent) is currently inside the
NGX_HAVE_REUSEPORT block and thus only compiled for reuseport builds; move the
logic out so it applies unconditionally in ngx_event_process_init: remove the
ngx_is_privileged_agent && !ls[i].privileged_agent and the
!ngx_is_privileged_agent && ls[i].privileged_agent branches from inside the
reuseport-only `#if` and place a single check (using ngx_is_privileged_agent and
ls[i].privileged_agent) in the loop before reuseport-specific handling so
privileged-agent listeners are always skipped/closed correctly regardless of
NGX_HAVE_REUSEPORT.
In `@patch/1.29.2.4/ngx_stream_lua-xrpc.patch`:
- Around line 228-238: The CR/LF trimming logic can read before the buffer when
a line is empty; after computing len = u->length - u->rest and setting *buf =
u->buffer.pos - len, guard both decrements by checking len > 0 before accessing
(*buf)[len - 1]; only decrement len (and test for '\r' after '\n') when len is
positive, then assign *actual_len = len. Reference the variables/expressions
actual_len, len, *buf, u->length, u->rest, and u->buffer.pos to locate the code
block to modify.
- Around line 713-725: The single-buffer fast path currently aliases
src->bufs_in->buf by assigning cl->buf->pos/last to src->bufs_in->buf->pos/last
and then immediately calls ngx_stream_lua_ffi_socket_reset_buf(ctx, src), which
makes the receive buffer reusable and can corrupt the queued send data; instead,
when src->bufs_in has a single buffer, copy its bytes into cl->buf (use ngx_copy
to append into cl->buf->last and update cl->buf->last accordingly) rather than
pointing pos/last at src->bufs_in->buf, then call
ngx_stream_lua_ffi_socket_reset_buf(ctx, src) as before (i.e., make the
single-buffer branch mirror the multi-buffer loop behavior so data is not
aliased to src->bufs_in->buf).
In `@patch/patch.sh`:
- Around line 81-86: The patch script uses incorrect bundled component versions
for OpenResty 1.29.2.4; update the apply_patch calls in the elif block that
checks "$root" == *openresty-1.29.2.4* (where patch_dir is set and apply_patch
is invoked) to match the official bundled versions: change the lua-resty-core
call from "0.1.34rc2" to "0.1.33rc2", change the ngx_lua call from "0.10.31rc2"
to "0.10.30rc2", and change the ngx_stream_lua call from "0.0.19rc3" to "0.0.18"
so the apply_patch invocations reference the exact packaged versions.
---
Major comments:
In `@patch/1.29.2.4/lua-resty-core-enable_keepalive.patch`:
- Around line 45-79: The change to _M.set_current_peer removed the previous
third-argument host/SNI override and thus breaks callers using
set_current_peer(addr, port, host); restore backwards-compatibility by accepting
either the legacy string third argument or the new opts table with an opts.host
field: if the third parameter is a string treat it as host (and set the peer
host/SNI replacement as before), otherwise if a table read opts.host in addition
to opts.pool and opts.pool_size; ensure you still compute pool_crc32 with
ngx_crc32_long(pool) and preserve the logic that copies the replacement host
into the peer state so existing balancer code keeps its host/SNI override.
In `@patch/1.29.2.4/lua-resty-core-reject-in-handshake.patch`:
- Around line 36-45: The patch currently defaults reject_in_handshake to true,
changing the default behavior of verify_client(); revert this to preserve
backward compatibility by default: in the verify_client implementation, set
reject_in_handshake to false when nil, keep the conversion to
reject_in_handshake_int and pass that to ngx_lua_ffi_ssl_verify_client (symbols
to edit: function verify_client, variable reject_in_handshake,
reject_in_handshake_int, and the call to ngx_lua_ffi_ssl_verify_client).
In `@patch/1.29.2.4/lua-resty-core-shared_shdict.patch`:
- Around line 203-213: The pcall currently wraps both optional and required FFI
bindings so if an optional symbol (e.g. C.ngx_meta_lua_ffi_shdict_free_space) is
missing it prevents assignment of required symbols like
ngx_lua_ffi_shdict_udata_to_zone and the other required bindings; fix by
assigning the required bindings (ngx_lua_ffi_shdict_get,
ngx_lua_ffi_shdict_incr, ngx_lua_ffi_shdict_store, ngx_lua_ffi_shdict_flush_all,
ngx_lua_ffi_shdict_get_ttl, ngx_lua_ffi_shdict_set_expire,
ngx_lua_ffi_shdict_capacity, and ngx_lua_ffi_shdict_udata_to_zone) directly
(outside any pcall) and keep only the truly optional binding
(C.ngx_meta_lua_ffi_shdict_free_space -> ngx_lua_ffi_shdict_free_space) inside a
pcall so missing optional symbols don’t prevent required ones from being set.
In `@patch/1.29.2.4/nginx-client_max_body_size.patch`:
- Around line 113-118: The APISIX branch is casting the uninitialized local clcf
via "(void) clcf;" which evaluates an uninitialized variable; remove that cast
(or alternatively move the ngx_http_get_module_loc_conf(...) assignment for clcf
above the `#if` to ensure clcf is initialized before any use). Locate the APISIX
block around ngx_http_apisix_client_max_body_size(r) and either delete the
"(void) clcf;" line or hoist the ngx_http_get_module_loc_conf(...) call so clcf
is properly set before the conditional branch.
In `@patch/1.29.2.4/nginx-connection-original-dst.patch`:
- Around line 47-50: The variable registration for connection_original_dst must
be marked non-cacheable: update the ngx_http_variable_t entry that registers
"connection_original_dst" (the line using ngx_http_variable_connection_dst) to
include the NGX_HTTP_VAR_NOCACHEABLE flag (same way connection_time is declared)
so the variable is treated as connection-specific and not cached by NGINX.
- Around line 59-93: The variable handler ngx_http_variable_connection_dst
incorrectly marks the result as cacheable by setting v->no_cacheable = 0; change
this to v->no_cacheable = 1 so the connection-specific original destination
string is not cached; update the assignment in ngx_http_variable_connection_dst
and keep the rest of the initialization (v->len, v->valid, v->not_found,
v->data) unchanged.
In `@patch/1.29.2.4/nginx-get_last_reopen_time.patch`:
- Around line 29-32: The computation tp->sec * 1000 + tp->msec used to set
ngx_last_reopen_msec can overflow because ngx_last_reopen_msec is currently a
ngx_uint_t; change the storage and any accessors to a millisecond type (use
ngx_msec_t) so the multiplication cannot overflow for long uptimes, update the
global declaration of ngx_last_reopen_msec from ngx_uint_t to ngx_msec_t and
adjust the accessor function return type and any callers that assume ngx_uint_t,
keeping the assignment using ngx_timeofday()'s tp->sec and tp->msec unchanged.
In `@patch/1.29.2.4/nginx-gzip.patch`:
- Around line 132-135: The new conditional bypasses the core gzip_http_version
guard when ngx_http_apisix enables per-request gzip, allowing HTTP/1.0 to be
compressed even if clcf->gzip_http_version is > 1.0; change the logic so the
protocol-version check always runs first (if r->http_version <
clcf->gzip_http_version return NGX_DECLINED) and do not short-circuit that check
based on ngx_http_apisix_is_gzip_set(r); if APISIX needs to override protocol
policy, implement that as a separate per-request hook instead of skipping the
core guard.
- Around line 30-35: The gzip enablement check currently short-circuits both the
gzip_min_length and gzip_types checks; update the conditional in the gzip
decision logic so ngx_http_apisix_is_gzip_set(r) (or conf->enable) governs only
the APISIX-specific enablement, while the static filters—checking
r->headers_out.content_length_n against conf->min_length (gzip_min_length) and
ngx_http_test_content_type(r, &conf->types) (gzip_types)—remain enforced for
every request; in practice, reorder/split the condition around
ngx_http_apisix_is_gzip_set(r) so that conf->min_length and
ngx_http_test_content_type are evaluated even when APISIX is enabled.
In `@patch/1.29.2.4/nginx-listen-in-privileged-agent.patch`:
- Around line 9-11: The patch causes ngx_open_listening_sockets to skip
listeners marked privileged_agent by checking ngx_is_privileged_agent !=
ls[i].privileged_agent, which prevents startup/config-test from validating those
binds; to fix, revert that conditional so privileged_agent listeners are
included during startup validation (remove or adjust the ngx_is_privileged_agent
!= ls[i].privileged_agent check in ngx_open_listening_sockets) so binding errors
are caught early rather than later in ngx_privileged_agent_process_cycle.
In `@patch/1.29.2.4/nginx-real_ip.patch`:
- Around line 40-52: The header currently declares ngx_http_realip_set_real_addr
unconditionally; wrap that declaration in the same `#if/`#endif conditional used
around its implementation (i.e., the NGX_HTTP_APISIX guard) so the prototype
only exists when NGX_HTTP_APISIX is defined; locate the declaration of
ngx_http_realip_set_real_addr in the header and surround it with the same `#if`
defined(NGX_HTTP_APISIX) ... `#endif` used in the implementation (keeping the
existing include guards intact).
In `@patch/1.29.2.4/ngx_lua-enable_keepalive.patch`:
- Around line 493-520: The pool lookup currently keys pools only by cpool_crc32
(see balancer_keepalive_pools_table_key, lua_rawget/lua_rawseti usage and
upool), which risks CRC collisions across different upstream configs; change the
key to a collision-free, config-scoped composite (for example combine a stable
upstream identity like lscf pointer or upstream name and the pool name/hash)
when pushing/reading from the registry (where lua_pushlightuserdata and
lua_rawget/lua_rawseti are used), store the original pool name/identity inside
upool (add a pool_name or upstream_id field) and on pool reuse verify the stored
name/identity matches the requested one before handing out a cached connection;
apply the same fix to the other block referenced (lines ~541-563) that does
similar lookup/insert.
In `@patch/1.29.2.4/ngx_lua-ssl_session_hostname.patch`:
- Around line 35-39: The nested call
SSL_SESSION_get0_hostname(SSL_get0_session(ssl_conn)) can dereference a NULL
session; store SSL_get0_session(ssl_conn) into a local SSL_SESSION* variable,
check that it is non-NULL before calling SSL_SESSION_get0_hostname, and only set
*name and *namelen and return NGX_OK when both the session and hostname are
non-NULL; otherwise fall through to the existing empty-string fallback handling
for *name/*namelen.
In `@patch/1.29.2.4/ngx_stream_lua-ssl_session_hostname.patch`:
- Around line 35-39: Guard the SSL_get0_session() result before calling
SSL_SESSION_get0_hostname: call SSL_get0_session(ssl_conn) into a local variable
(e.g., sess), check if sess is NULL and if so set *name = NULL, *namelen = 0 and
return an appropriate NGX status (e.g., NGX_DECLINED or NGX_ERROR) instead of
calling SSL_SESSION_get0_hostname on NULL; only call
SSL_SESSION_get0_hostname(sess) and compute ngx_strlen(*name) when sess is
non-NULL, keeping the existing use of ssl_conn, name, namelen and returning
NGX_OK on success.
In `@patch/1.29.2.4/ngx_stream_lua-tlshandshake.patch`:
- Around line 421-429: After calling ngx_stream_lua_ssl_handshake_handler(c),
ensure you check the handshake outcome stored in rc and the connection userdata
error (u->error_ret) and propagate it: if rc == NGX_ERROR set *errmsg =
u->error_ret and return NGX_ERROR so synchronous post-handshake failures
(cert/hostname verification) are reported; otherwise return NGX_OK — this
prevents treating FFI_OK as success in the reused_session fast path and ensures
get_tlshandshake_result() sees the real error.
- Around line 315-329: The OCSP setup removed the return-value check for
SSL_set_tlsext_status_type causing silent failures; restore the check in the
ocsp_status_req branch by calling SSL_set_tlsext_status_type(c->ssl->connection,
TLSEXT_STATUSTYPE_ocsp) and if it does not return 1 set *errmsg = "failed to
enable OCSP stapling" and return NGX_ERROR (use the same error-reporting pattern
as the surrounding code that assigns to errmsg and returns NGX_ERROR); reference
ocsp_status_req, SSL_set_tlsext_status_type, TLSEXT_STATUSTYPE_ocsp, *errmsg and
NGX_ERROR to locate and fix the code.
In `@patch/1.29.2.4/ngx_stream_lua-xrpc.patch`:
- Around line 111-124: The macro ngx_stream_lua_ffi_socket_check_busy_writing
currently only fills errbuf/errbuf_size and falls through, so callers like
ngx_stream_lua_ffi_socket_tcp_send_from_socket continue and can corrupt
in-flight writes; change the macro so that after populating errbuf and updating
*errbuf_size it returns immediately (e.g., return NGX_AGAIN) from the calling
function to stop further processing when (u)->write_waiting or
(u)->raw_downstream && (r)->connection->buffered is true.
- Around line 176-184: The code only rewires cl->next (via ll) which detaches
the middle of the u->bufs_in chain; instead find the tail of the prefix (the
node whose next == u->buf_in), set that tail->next = ctx->free_recv_bufs so the
entire prefix chain is spliced, then set ctx->free_recv_bufs = u->bufs_in and
finally u->bufs_in = u->buf_in; use the existing cl/ll traversal to locate that
tail and update its next pointer (not just a single link) so nodes between
u->bufs_in and u->buf_in aren't lost.
---
Minor comments:
In `@patch/1.29.2.4/lua-resty-core-tlshandshake.patch`:
- Around line 302-306: The guard in sslhandshake currently allows one extra
positional arg (uses n > 1), so change the check to reject any non-empty varargs
by using n > 0 (i.e. if not self or n > 0 then error), and update the "seen"
count expression from (self and 5 + n or 0) to (self and 1 + n or 0) so the
error message reports the actual total arguments seen; adjust only the condition
and the seen-count expression in the sslhandshake wrapper (variables: n, self,
sslhandshake).
In `@patch/1.29.2.4/nginx-get_last_reopen_time.patch`:
- Around line 61-67: Change the function signature of
ngx_worker_process_get_last_reopen_ms to explicitly take no parameters by
replacing the empty parameter list "()" with "(void)"; update the definition
(ngx_worker_process_get_last_reopen_ms) accordingly so it reads "ngx_uint_t
ngx_worker_process_get_last_reopen_ms(void)" (keep the body and `#if`
NGX_HTTP_APISIX wrapper unchanged).
In `@patch/1.29.2.4/ngx_lua-request_header_set.patch`:
- Around line 19-24: The call to ngx_http_apisix_mark_request_header_set
currently happens before the fake-request guard (checking r->connection->fd
against (ngx_socket_t)-1) and can create side effects even when the function
returns NGX_HTTP_LUA_FFI_BAD_CONTEXT; move the
ngx_http_apisix_mark_request_header_set(r) invocation to immediately after the
fake-request check (i.e., after the early return for r->connection->fd ==
(ngx_socket_t)-1) so no header mutation is marked for fake requests.
---
Nitpick comments:
In `@patch/1.29.2.4/nginx-connection-original-dst.patch`:
- Around line 64-72: The code uses IPv4-specific netfilter calls (struct
sockaddr_in, SOL_IP and SO_ORIGINAL_DST via getsockopt) which restricts it to
IPv4; ensure this is explicitly compile-time guarded by the
NGX_HAVE_NETFILTER_IPV4 macro and add a short comment near the block
(referencing getsockopt, SO_ORIGINAL_DST, SOL_IP and struct sockaddr_in) stating
the IPv4-only limitation and that IPv6 requires a different kernel interface so
readers aren’t misled; if the surrounding code already has the `#if`
(NGX_HAVE_NETFILTER_IPV4) guard, verify it encloses this exact block and no IPv6
symbols are introduced outside that guard.
In `@patch/1.29.2.4/nginx-enable_ntls.patch`:
- Around line 19-22: The call to SSL_enable_ntls(c->ssl->connection) must check
its return value and fail the handshake if enabling NTLS fails; update the block
guarded by ngx_http_apisix_is_ntls_enabled(hc->conf_ctx) to capture the result
of SSL_enable_ntls, log or propagate an error when it returns a failure code,
and return an appropriate error/terminate the connection initialization path
instead of continuing silently so that handshake initialization does not proceed
on failure.
In `@patch/1.29.2.4/ngx_lua-init_worker_coroutine.patch`:
- Line 8: The inline comment "/* ignore header and body filters */" no longer
matches the excluded context values — update that comment where the context mask
excludes 0x020, 0x040 and the newly excluded 0x100 to accurately describe all
excluded phases (either expand to "/* ignore header, body and
<phase-name-for-0x100> filters */" or to a generic "/* ignore header, body and
additional filter (0x100) */" if the phase name is unknown), so the comment
matches the mask values 0x020, 0x040 and 0x100.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 1835e6ff-1ae5-471b-b1a3-f6ee3b32e0dc
📒 Files selected for processing (36)
patch/1.29.2.4/lua-resty-core-enable_keepalive.patchpatch/1.29.2.4/lua-resty-core-init_worker_coroutine.patchpatch/1.29.2.4/lua-resty-core-reject-in-handshake.patchpatch/1.29.2.4/lua-resty-core-shared_shdict.patchpatch/1.29.2.4/lua-resty-core-ssl_session_hostname.patchpatch/1.29.2.4/lua-resty-core-tlshandshake.patchpatch/1.29.2.4/nginx-client_max_body_size.patchpatch/1.29.2.4/nginx-connection-original-dst.patchpatch/1.29.2.4/nginx-enable_ntls.patchpatch/1.29.2.4/nginx-error_page_contains_apisix.patchpatch/1.29.2.4/nginx-get_last_reopen_time.patchpatch/1.29.2.4/nginx-grpc_set_header_authority.patchpatch/1.29.2.4/nginx-gzip.patchpatch/1.29.2.4/nginx-listen-in-privileged-agent.patchpatch/1.29.2.4/nginx-mirror.patchpatch/1.29.2.4/nginx-privileged_agent_process_thread.patchpatch/1.29.2.4/nginx-proxy_request_buffering.patchpatch/1.29.2.4/nginx-real_ip.patchpatch/1.29.2.4/nginx-tcp_over_tls.patchpatch/1.29.2.4/nginx-upstream_mtls.patchpatch/1.29.2.4/nginx-upstream_pass_trailers.patchpatch/1.29.2.4/ngx_lua-enable_keepalive.patchpatch/1.29.2.4/ngx_lua-init_worker_coroutine.patchpatch/1.29.2.4/ngx_lua-reject-in-handshake.patchpatch/1.29.2.4/ngx_lua-request_header_set.patchpatch/1.29.2.4/ngx_lua-shared_shdict.patchpatch/1.29.2.4/ngx_lua-skip_filter.patchpatch/1.29.2.4/ngx_lua-ssl_session_hostname.patchpatch/1.29.2.4/ngx_lua-tlshandshake.patchpatch/1.29.2.4/ngx_stream_lua-expose_request_struct.patchpatch/1.29.2.4/ngx_stream_lua-reject-in-handshake.patchpatch/1.29.2.4/ngx_stream_lua-shared_shdict.patchpatch/1.29.2.4/ngx_stream_lua-ssl_session_hostname.patchpatch/1.29.2.4/ngx_stream_lua-tlshandshake.patchpatch/1.29.2.4/ngx_stream_lua-xrpc.patchpatch/patch.sh
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 35 out of 36 changed files in this pull request and generated 3 comments.
Comments suppressed due to low confidence (2)
patch/1.29.2.4/ngx_stream_lua-xrpc.patch:458
- The debug log message says "allocate new new buf..." (duplicated word). Consider changing it to "allocate new buf..." to keep logs clean/searchable.
patch/1.29.2.4/ngx_stream_lua-xrpc.patch:486 - The debug log message says "allocate new new buf..." (duplicated word). Consider changing it to "allocate new buf..." to keep logs clean/searchable.
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
patch/1.29.2.4/lua-resty-core-tlshandshake.patch (2)
299-318:⚠️ Potential issue | 🟠 Major | ⚡ Quick winPotential race condition with shared
cached_optionstable across coroutines.The
sslhandshakefunction populates the module-levelcached_optionstable, then callstlshandshakewhich may yield (viaco_yield()). If another coroutine callssslhandshakewhile this one is suspended, it will overwrite the shared table, corrupting the first coroutine's options when it resumes.Consider using a per-call options table instead:
local function sslhandshake(self, reused_session, server_name, ssl_verify, send_status_req, ...) local n = select("#", ...) if not self or n > 1 then error("ngx.socket sslhandshake: expecting 1 ~ 5 arguments " .. "(including the object), but seen " .. (self and 5 + n or 0)) end - cached_options.reused_session = reused_session - cached_options.server_name = server_name - cached_options.verify = ssl_verify - cached_options.ocsp_status_req = send_status_req + local options = { + reused_session = reused_session, + server_name = server_name, + verify = ssl_verify, + ocsp_status_req = send_status_req, + } - local res, err = tlshandshake(self, cached_options) - - clear_tab(cached_options) + local res, err = tlshandshake(self, options) return res, err end🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@patch/1.29.2.4/lua-resty-core-tlshandshake.patch` around lines 299 - 318, In sslhandshake replace use of the shared module-level cached_options with a per-call local options table: create a local table (e.g., local opts = {}) and set opts.reused_session, opts.server_name, opts.verify, and opts.ocsp_status_req from the parameters, then call tlshandshake(self, opts); remove reliance on clear_tab(cached_options) and the module-level cached_options mutation to avoid races; reference sslhandshake, cached_options, tlshandshake and clear_tab when making the change.
128-141:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winFile handle not closed on read error.
If
f:read("*a")fails, the function returns without closing the file handle.local function read_file(path) local f, err = io_open(path) if not f then return nil, err end local txt, err = f:read("*a") f:close() - if not txt then - return nil, err - end - return txt + return txt, err end🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@patch/1.29.2.4/lua-resty-core-tlshandshake.patch` around lines 128 - 141, The read_file function leaks the file handle when f:read("*a") fails; update read_file (which calls io_open and uses f:read and f:close) to always close f before returning on error (e.g., after local txt, err = f:read("*a") do if not txt then f:close(); return nil, err end), ensuring the file handle is closed on both success and failure paths.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@patch/1.29.2.4/lua-resty-core-tlshandshake.patch`:
- Around line 299-318: In sslhandshake replace use of the shared module-level
cached_options with a per-call local options table: create a local table (e.g.,
local opts = {}) and set opts.reused_session, opts.server_name, opts.verify, and
opts.ocsp_status_req from the parameters, then call tlshandshake(self, opts);
remove reliance on clear_tab(cached_options) and the module-level cached_options
mutation to avoid races; reference sslhandshake, cached_options, tlshandshake
and clear_tab when making the change.
- Around line 128-141: The read_file function leaks the file handle when
f:read("*a") fails; update read_file (which calls io_open and uses f:read and
f:close) to always close f before returning on error (e.g., after local txt, err
= f:read("*a") do if not txt then f:close(); return nil, err end), ensuring the
file handle is closed on both success and failure paths.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 08df9099-b35f-4796-9c9c-f8b6ae87b36d
📒 Files selected for processing (7)
patch/1.29.2.4/lua-resty-core-shared_shdict.patchpatch/1.29.2.4/lua-resty-core-tlshandshake.patchpatch/1.29.2.4/nginx-client_max_body_size.patchpatch/1.29.2.4/nginx-error_page_contains_apisix.patchpatch/1.29.2.4/nginx-listen-in-privileged-agent.patchpatch/1.29.2.4/ngx_lua-enable_keepalive.patchpatch/1.29.2.4/ngx_stream_lua-xrpc.patch
✅ Files skipped from review due to trivial changes (1)
- patch/1.29.2.4/nginx-error_page_contains_apisix.patch
There was a problem hiding this comment.
🧹 Nitpick comments (1)
patch/1.29.2.4/ngx_stream_lua-tlshandshake.patch (1)
458-458: 💤 Low valueUnused variable
dc.The variable
dc(downstream connection) is assigned fromr->connectionbut never used in this function. This appears to be dead code from the refactoring.🧹 Remove unused variable
ngx_int_t rc; - ngx_connection_t *dc; /* downstream connection */ ngx_stream_lua_request_t *r; ngx_stream_lua_ctx_t *ctx; ngx_stream_lua_loc_conf_t *llcf;And remove the assignment:
waiting = u->conn_waiting; - dc = r->connection; - if (c->read->timedout) {🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@patch/1.29.2.4/ngx_stream_lua-tlshandshake.patch` at line 458, Remove the unused downstream connection variable by deleting the declaration/assignment of dc (the line assigning dc = r->connection) in the function where dc is declared; ensure no other code references dc and no behavior changes result from removing that dead variable.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@patch/1.29.2.4/ngx_stream_lua-tlshandshake.patch`:
- Line 458: Remove the unused downstream connection variable by deleting the
declaration/assignment of dc (the line assigning dc = r->connection) in the
function where dc is declared; ensure no other code references dc and no
behavior changes result from removing that dead variable.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: e6684b3b-99b7-41f7-aa18-6fe791a4c881
📒 Files selected for processing (1)
patch/1.29.2.4/ngx_stream_lua-tlshandshake.patch
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
patch/1.29.2.4/lua-resty-core-tlshandshake.patch (1)
125-126:⚠️ Potential issue | 🟠 Major | ⚡ Quick winModule-level
cached_optionstable may cause race conditions in concurrent requests.The
cached_optionstable at line 125 is shared across all requests. Insslhandshake(lines 308-316), this table is populated,tlshandshakeis called, and then cleared. If two concurrent requests callsslhandshakesimultaneously, one request could overwrite the other's options beforetlshandshakereads them, orclear_tabcould wipe options mid-use.Consider creating a new options table per call in
sslhandshakeinstead of reusing a module-level cached table.Suggested fix
local function sslhandshake(self, reused_session, server_name, ssl_verify, send_status_req, ...) local n = select("#", ...) if not self or n > 0 then error("ngx.socket sslhandshake: expecting 1 ~ 5 arguments " .. "(including the object), but seen " .. (self and 5 + n or 0)) end - cached_options.reused_session = reused_session - cached_options.server_name = server_name - cached_options.verify = ssl_verify - cached_options.ocsp_status_req = send_status_req - - local res, err = tlshandshake(self, cached_options) - - clear_tab(cached_options) + local options = { + reused_session = reused_session, + server_name = server_name, + verify = ssl_verify, + ocsp_status_req = send_status_req, + } + + local res, err = tlshandshake(self, options) return res, err endAlso applies to: 154-157, 308-316
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@patch/1.29.2.4/lua-resty-core-tlshandshake.patch` around lines 125 - 126, The module-level cached_options table is shared across requests and causes race conditions; fix by making options a new local table inside sslhandshake (instead of using module-level cached_options/new_tab) and pass that local table into tlshandshake so each call has its own options; remove usages of the global cached_options and any clear_tab(cached_options) calls (or replace them with clearing the local table only if needed) and update references in sslhandshake, tlshandshake and any helper code that expects the global to accept the options table as an argument.patch/1.29.2.4/ngx_stream_lua-xrpc.patch (1)
767-767:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winVariable
bmay be uninitialized in the probe call.The variable
bis only assigned inside theif (!src->bufs_in->next)orelsebranches (lines 716-724), but both branches assignbfromsrc->bufs_in->buforin_cl->buf. However, when the send data is from a single buffer,bis assigned assrc->bufs_in->bufat line 716. When there are multiple buffers,bis the lastin_cl->buffrom the loop. In both casesbis assigned, but the probe at line 767 usesb->poswhich should becl->buf->possince we're sending fromcl, not the source buffers. After copying,bpoints to source buffer positions, not the destination.Suggested fix
- ngx_stream_lua_probe_socket_tcp_send_start(r, u, b->pos, len); + ngx_stream_lua_probe_socket_tcp_send_start(r, u, cl->buf->pos, len);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@patch/1.29.2.4/ngx_stream_lua-xrpc.patch` at line 767, The probe call ngx_stream_lua_probe_socket_tcp_send_start currently uses b->pos which may point at the source buffer rather than the actual buffer being sent; change the probe to use cl->buf->pos (the destination/outgoing chain buffer for the current cl) or ensure a local variable (e.g., outb) is set to cl->buf before the copy and pass outb->pos to ngx_stream_lua_probe_socket_tcp_send_start; update references around the send loop where cl and b are manipulated (look for variables named cl, b, src->bufs_in and the probe call ngx_stream_lua_probe_socket_tcp_send_start) so the probe always receives the actual send buffer position and not an uninitialized/source buffer pointer.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@patch/1.29.2.4/lua-resty-core-enable_keepalive.patch`:
- Around line 72-75: The patch incorrectly treats pool_crc32 == 0 as an
invalid/missing pool in set_current_peer; remove the guard that errors on
pool_crc32 being 0 and instead detect presence of the pool by checking the
original option (e.g., opts.pool == nil) or by adding an explicit boolean flag
(e.g., has_pool) when computing pool_crc32; update the logic in the
set_current_peer block that uses pool_crc32 so it only rejects missing pools via
opts.pool (or has_pool) and not by the numeric CRC value.
---
Outside diff comments:
In `@patch/1.29.2.4/lua-resty-core-tlshandshake.patch`:
- Around line 125-126: The module-level cached_options table is shared across
requests and causes race conditions; fix by making options a new local table
inside sslhandshake (instead of using module-level cached_options/new_tab) and
pass that local table into tlshandshake so each call has its own options; remove
usages of the global cached_options and any clear_tab(cached_options) calls (or
replace them with clearing the local table only if needed) and update references
in sslhandshake, tlshandshake and any helper code that expects the global to
accept the options table as an argument.
In `@patch/1.29.2.4/ngx_stream_lua-xrpc.patch`:
- Line 767: The probe call ngx_stream_lua_probe_socket_tcp_send_start currently
uses b->pos which may point at the source buffer rather than the actual buffer
being sent; change the probe to use cl->buf->pos (the destination/outgoing chain
buffer for the current cl) or ensure a local variable (e.g., outb) is set to
cl->buf before the copy and pass outb->pos to
ngx_stream_lua_probe_socket_tcp_send_start; update references around the send
loop where cl and b are manipulated (look for variables named cl, b,
src->bufs_in and the probe call ngx_stream_lua_probe_socket_tcp_send_start) so
the probe always receives the actual send buffer position and not an
uninitialized/source buffer pointer.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: b8aa85a8-ca92-446c-b267-7a01e6613de1
📒 Files selected for processing (4)
patch/1.29.2.4/lua-resty-core-enable_keepalive.patchpatch/1.29.2.4/lua-resty-core-tlshandshake.patchpatch/1.29.2.4/ngx_stream_lua-tlshandshake.patchpatch/1.29.2.4/ngx_stream_lua-xrpc.patch
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
patch/1.29.2.4/ngx_lua-enable_keepalive.patch (1)
1050-1163:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy liftKeep
bp->localin the implicit keepalive-pool key.The old cache key included the local bind address, but this fallback now derives
cpool_crc32frombp->hostalone when no explicit pool was set. That means two peers targeting the same remote address with differentbp->localvalues can share one pool and reuse a connection opened from the wrong source address. Please fold the local sockaddr into the implicit pool identity, or keep separate implicit pools perbp->local.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@patch/1.29.2.4/ngx_lua-enable_keepalive.patch` around lines 1050 - 1163, The implicit keepalive pool key currently computes cpool_crc32 from bp->host only, causing different local bind addresses (bp->local) to share a pool; change the implicit pool key calculation to include the local sockaddr (bp->local) so the derived cpool_crc32 (or pool identity) folds in bp->local; locate the code that computes cpool_crc32/implicit pool lookup (references to cpool_crc32, bp->host, and bp->local) and append a stable representation of bp->local (e.g., family + addr + port) into the CRC/update routine or use separate implicit pools per bp->local when no explicit pool is set so connections bound to different local addresses do not get reused across peers.patch/1.29.2.4/lua-resty-core-enable_keepalive.patch (1)
45-59:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy liftPreserve the legacy host override path here.
This turns existing
balancer.set_current_peer(addr, port, host)call sites into a runtime error, and the C-side patch no longer has any way to carry that override into upstream SSL/SNI handling. HTTPS balancers that select an IP/socket but rely on a logical host name will lose that capability entirely. Please keep backward compatibility here by accepting the legacy string form (or an explicitopts.host) and passing it separately from the keepalive-pool options.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@patch/1.29.2.4/lua-resty-core-enable_keepalive.patch` around lines 45 - 59, The change to _M.set_current_peer replaced the third parameter host with opts and broke backward compatibility; restore support for the legacy string form by accepting either a string third argument (treat as host) or a table opts, preserving previous behavior: if opts is a string, set local host = opts and set opts = nil (or leave pool_* unset), otherwise validate opts is a table and extract pool_crc32/pool_set/pool_size from it and also accept opts.host as the explicit host; ensure the function continues to pass the host separately from keepalive pool options so upstream SSL/SNI handling still receives the logical host.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@patch/1.29.2.4/lua-resty-core-enable_keepalive.patch`:
- Around line 45-59: The change to _M.set_current_peer replaced the third
parameter host with opts and broke backward compatibility; restore support for
the legacy string form by accepting either a string third argument (treat as
host) or a table opts, preserving previous behavior: if opts is a string, set
local host = opts and set opts = nil (or leave pool_* unset), otherwise validate
opts is a table and extract pool_crc32/pool_set/pool_size from it and also
accept opts.host as the explicit host; ensure the function continues to pass the
host separately from keepalive pool options so upstream SSL/SNI handling still
receives the logical host.
In `@patch/1.29.2.4/ngx_lua-enable_keepalive.patch`:
- Around line 1050-1163: The implicit keepalive pool key currently computes
cpool_crc32 from bp->host only, causing different local bind addresses
(bp->local) to share a pool; change the implicit pool key calculation to include
the local sockaddr (bp->local) so the derived cpool_crc32 (or pool identity)
folds in bp->local; locate the code that computes cpool_crc32/implicit pool
lookup (references to cpool_crc32, bp->host, and bp->local) and append a stable
representation of bp->local (e.g., family + addr + port) into the CRC/update
routine or use separate implicit pools per bp->local when no explicit pool is
set so connections bound to different local addresses do not get reused across
peers.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 537cdfd7-60ef-4113-93d8-9d356f4cb1d4
📒 Files selected for processing (2)
patch/1.29.2.4/lua-resty-core-enable_keepalive.patchpatch/1.29.2.4/ngx_lua-enable_keepalive.patch
nic-6443
approved these changes
May 22, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Add the patch set for OpenResty 1.29.2.4 and wire patch.sh to apply it against the bundled nginx 1.29.2, lua-resty-core 0.1.34rc2, ngx_lua 0.10.31rc2, and ngx_stream_lua 0.0.19rc3.\n\nVerified locally by applying the patch set to the official OpenResty 1.29.2.4 tarball and building nginx with apisix-nginx-module enabled.
Summary by CodeRabbit
New Features
Improvements