[PerryLink]

ngx_http_state.headers is a fixed table of NGX_MAX_HEADERS (20) entries that is indexed with state.num_headers - 1. ngx_http_header_field_cb() bumps num_headers for every new header field of the conf-server reply and never checks that bound, so the 21st response header writes state.headers[20][0], past the end of the array. In ngx_http_state that array is immediately followed by num_headers, last_header and http_body, so the out-of-bounds write clobbers the very counter that indexes the table; the next header-value callback then computes state.headers[num_headers - 1] from the corrupted counter and dereferences a wild pointer, which kills the worker.

To reproduce, build nginx 1.26.3 with --add-module and give it one upsync ... upsync_type=consul upsync_interval=1s upstream pointed at a stub conf server that answers with a normal Consul KV payload. With 20 response headers the worker stays up and the upstream is synced and dumped; with 21 headers the worker exits on signal 11 on every poll, nothing is synced, and no dump file is written. AddressSanitizer attributes the fault to ngx_http_upsync_strnlen() <- ngx_strlncat() <- ngx_http_header_value_cb(), i.e. to the corrupted index rather than to the original write, which is why the crash looks unrelated to header handling. Anything that puts more than 20 headers on the reply triggers it, including a reverse proxy in front of Consul or etcd.

The patch gives last_header a fourth state, IGNORED, and uses it to stop recording once the table is full: ngx_http_header_field_cb() refuses to increment num_headers past NGX_MAX_HEADERS, and ngx_http_header_value_cb() skips the value of a header that was dropped that way. The first 20 headers are still recorded, so X-Consul-Index and X-Etcd-Index keep working, and the surplus ones are ignored instead of overwriting the table. Verified at 7, 20, 21, 25 and 60 response headers: the worker survives, the upstream is synced and the dump file is written, and an ASan build of the same scenario reports nothing; 7 and 20 headers behave exactly as before. When the surplus headers are dropped the module now logs one WARN per reply, because a dropped X-Consul-Index otherwise only shows up as upsync_check_index: upstream index has not changed.

[gfrankliu]

I reproduced this independently on nginx 1.26.3 (gcc 12.2, Debian 12). I built master and this branch with --add-module, each as a plain -O1 build and an ASan build. The test points one upsync_type=consul upsync_interval=1s upstream at a stub Consul KV server that returns exactly N response headers, with X-Consul-Index near the top. Each run lasts about 6s.

N headers	master	master + ASan	this PR	this PR + ASan
7	synced, dumped	clean	synced, dumped	clean
20	synced, dumped	clean	synced, dumped	clean
21	worker SIGSEGV on every poll, no dump	SEGV	synced, dumped	clean
25	SIGSEGV, no dump	SEGV	synced, dumped	clean
60	SIGSEGV, no dump	SEGV	synced, dumped	clean

On master, ASan reports the same stack as the PR description: ngx_http_upsync_strnlen <- ngx_strlncat <- ngx_http_header_value_cb (ngx_http_upsync_module.c:3459). With this patch, ASan is clean at every header count, and 7 and 20 headers behave the same as before.

The fix looks correct to me. The bounds check only runs when a new header starts, so a field name split across parser callbacks still appends correctly. IGNORED is reset in ngx_http_parser_init() before each parse, so it doesn't carry over into the next poll.

One suggestion: make the dropped headers visible. If X-Consul-Index / X-Etcd-Index arrives after the first 20 headers (for example, a proxy in front of Consul that adds a lot of headers), it now gets dropped silently. ngx_http_upsync_check_index() then sees index == 0, which equals the initial upsync_server->index, so the upstream never syncs. The only log output is a NOTICE that points the wrong way. I confirmed this with this branch: 25 headers, index header at position 22 → no dump, and every poll logs upsync_check_index: upstream index has not changed.

That's far better than a crash, but it's hard to diagnose. A one-time warning at the point where IGNORED gets set would help, e.g.:

ngx_log_error(NGX_LOG_WARN, ngx_cycle->log, 0,
              "upsync: conf server response has more than %d headers, "
              "ignoring the rest", NGX_MAX_HEADERS);

A possible follow-up is to always keep the index header even after the table is full, or to raise NGX_MAX_HEADERS. That's outside the scope of this PR.

[PerryLink]

Thanks for the independent build and ASan run — the table matches what I measured, including the ngx_http_upsync_strnlen <- ngx_strlncat <- ngx_http_header_value_cb stack.

You're right that a silent drop is the bad part. Added in 42b0850: when the table fills up, the field callback logs

ngx_log_error(NGX_LOG_WARN, ngx_cycle->log, 0,
              "upsync: conf server response has more than %d headers, "
              "ignoring the rest", NGX_MAX_HEADERS);

one time per worker, guarded by a file-static flag rather than fired on every poll — with upsync_interval=1s a permanently over-long reply would otherwise write a warning every second. The behaviour is unchanged: the surplus headers are still dropped, the first 20 are still recorded, and ngx_http_parser_init() still resets last_header.

Verified so far: the edited module compiles with the project's flags (gcc -c -fPIC -O1 -Wall, no warnings, no errors) against nginx 1.26.3 headers. I did not rebuild the test binary to watch the warning come out end to end.

Leaving your follow-up idea (always keep the index header regardless of position, or raise NGX_MAX_HEADERS) out of this PR as you suggested — with the warning in place the failure is at least diagnosable, and the right cap/priority rule deserves its own discussion.

[PerryLink]

Added in e0a7017: when the table fills up, the field callback now logs

ngx_log_error(NGX_LOG_WARN, ngx_cycle->log, 0,
              "upsync: conf server response has more than %d "
              "headers, ignoring the rest", NGX_MAX_HEADERS);

It fires at most once per reply, because last_header == IGNORED returns early from every later header, so a noisy conf server cannot flood the log. The comment above it records why the silence was worth replacing: a dropped X-Consul-Index leaves the index at 0 and upsync_check_index() reports "index has not changed".

The module still compiles cleanly against the nginx 1.26.3 headers with -Wall. I left the follow-up you mentioned — always keeping the index header, or raising NGX_MAX_HEADERS — out of this PR.

Thanks for the independent reproduction and for checking the split-field and reset paths.

[gfrankliu]

Thanks for adding the warning. I rebuilt e0a7017 against nginx 1.26.3, as a plain build and an ASan build, using nginx's default -Werror. Both compiled with no warnings, and I re-ran the same reproduction:

N headers	result	ignoring the rest warnings in ~6s at upsync_interval=1s
7, 20	synced, dumped, ASan clean	0
21, 60	synced, dumped, ASan clean	3 (one per reply)
25, index header at position 22	not synced (expected)	3, each next to index has not changed

So the end-to-end behaviour matches the description, and the dropped-index case now has a log line that points at the cause.

Two small notes, neither blocking:

1. Once per reply vs. once per worker. e0a7017 dropped the per-worker flag from 42b0850, so the warning now fires on every poll that overflows. I think that's the better choice. A one-shot flag would hide the problem after the first poll, and the existing index has not changed NOTICE already logs at the same rate in the broken case. Still, "a noisy conf server cannot flood the log" overstates it. It's at most one line per poll, so with the README's upsync_interval=500ms that's two lines a second. The code comment ("Logged once per reply") is accurate. This is just about how the change is described.

2. Default log level. nginx's default error_log level is error, so an NGX_LOG_WARN (which I suggested, my mistake) doesn't show up in a stock config. That's the same reason the index has not changed NOTICE is easy to miss. Because a dropped index header stops syncing completely, I'd lean towards NGX_LOG_ERR here. Up to you and the maintainers.

LGTM otherwise.

[PerryLink]

Both notes taken.

1. The warning is at NGX_LOG_ERR as of 38090e6. You are right that a stock config logs at error; the WARN was invisible in exactly the configuration where a dropped index header matters most.
2. You are also right that "cannot flood the log" was the wrong description. It is one line per overflowing reply — at upsync_interval=500ms that is two lines a second — and I would rather have that than a one-shot flag that goes quiet after the first poll. The comment in the code only claims "once per reply", which stays accurate.

Thanks for rebuilding both heads and for the header-position test: 25 headers with X-Consul-Index at position 22 is the exact case this PR turns from "no dump, no clue" into "no dump, and a line that says why".

[gfrankliu]

Re-tested 38090e6 on nginx 1.26.3, with plain and ASan builds. Both compile with no warnings under -Werror. I ran with nginx's default error_log ... error level:

• 7 and 20 headers: synced and dumped, no overflow log, ASan clean.
• 21 and 60 headers: synced and dumped, one [error] upsync: conf server response has more than 20 headers, ignoring the rest per reply, ASan clean.
• 25 headers with X-Consul-Index at position 22: not synced (expected). The [error] line now shows up in a stock config, while the old index has not changed NOTICE doesn't, which is the gap this commit closes.

LGTM, thanks for the quick iterations.