Skip to content

fix(controller): cap JSON parser depth/elements and tighten request-boundary state #25

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
andypost wants to merge 1 commit into
base: master
Choose a base branch
from
Open

fix(controller): cap JSON parser depth/elements and tighten request-boundary state #25

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 12 additions & 0 deletions CHANGES
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,18 @@

Changes with FreeUnit 1.35.4 xx xxx 2026

*) Bugfix: cap JSON parser depth (100) and per-array/object element
count (100k) in the controller so a malformed POST /config
payload like "[[[[…]]]]" or a million-element flat array can no
longer exhaust the stack or heap. Scrub PHP TrueAsync EG
exception state before the prototype-fork so workers do not
inherit a stale exception across the boundary, and guard the
Ruby rack.input / rack.errors handles against NULL-deref if an
app captures them outside the request lifetime, and bind each
handle to its originating request so a captured handle cannot
read/write the body of a later, unrelated request handled by
the same worker.

*) Bugfix: fix router process CPU spin and connection hang under port
scanning load; CLOSE-WAIT sockets are now cleaned up properly on
client FIN, idle connection queue iteration fixed, systemd file
Expand Down
59 changes: 57 additions & 2 deletions src/nxt_conf.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -103,6 +103,23 @@ typedef struct {
static nxt_int_t nxt_conf_path_next_token(nxt_conf_path_parse_t *parse,
nxt_str_t *token);

/*
* Hard caps on JSON inputs to the controller. Both values are
* intentionally well above any legitimate Unit config (configs rarely
* nest more than 6 levels; the largest production configs we've seen
* have a few thousand elements per array). These exist to bound the
* controller's stack and heap usage on malformed input.
*/
#define NXT_CONF_JSON_MAX_DEPTH 100
#define NXT_CONF_JSON_MAX_ELEMENTS 100000

/*
* Recursion-depth counter. The controller is single-threaded so a
* file-static suffices. Reset on every entry to nxt_conf_json_parse()
* so a stale value from an aborted prior call cannot leak.
*/
static nxt_uint_t nxt_conf_json_parse_depth;

@gemini-code-assist gemini-code-assist Bot May 11, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

The use of a file-static variable for recursion depth is not thread-safe. While the Unit controller process is currently single-threaded, this file (src/nxt_conf.c) is part of the core library and is also linked into the router process, which supports multi-threading (e.g., via the listen_threads setting). If nxt_conf_json_parse is ever called from a multi-threaded context in the future, this will lead to race conditions and incorrect depth tracking. Consider using thread-local storage (e.g., __thread or _Thread_local) or passing the depth as an argument through the recursive calls.


static u_char *nxt_conf_json_skip_space(u_char *start, const u_char *end);
static u_char *nxt_conf_json_parse_value(nxt_mp_t *mp, nxt_conf_value_t *value,
u_char *start, u_char *end, nxt_conf_json_error_t *error);
Expand Down Expand Up @@ -1277,6 +1294,8 @@ nxt_conf_json_parse(nxt_mp_t *mp, u_char *start, u_char *end,
return NULL;
}

nxt_conf_json_parse_depth = 0;

p = nxt_conf_json_parse_value(mp, value, p, end, error);

if (nxt_slow_path(p == NULL)) {
Expand Down Expand Up @@ -1393,10 +1412,34 @@ nxt_conf_json_parse_value(nxt_mp_t *mp, nxt_conf_value_t *value, u_char *start,

switch (ch) {
case '{':
return nxt_conf_json_parse_object(mp, value, start, end, error);
/*
* Bound recursion depth so a deeply-nested payload such as
* "[[[[...]]]]" cannot blow the controller's stack. Only
* '{' and '[' deepen the recursion; leaf cases below don't
* need to touch the counter.
*/
if (nxt_slow_path(nxt_conf_json_parse_depth
>= NXT_CONF_JSON_MAX_DEPTH)) {
nxt_conf_json_parse_error(error, start,
"JSON nesting exceeds the maximum supported depth.");
return NULL;
}
nxt_conf_json_parse_depth++;
p = nxt_conf_json_parse_object(mp, value, start, end, error);
nxt_conf_json_parse_depth--;
return p;

case '[':
return nxt_conf_json_parse_array(mp, value, start, end, error);
if (nxt_slow_path(nxt_conf_json_parse_depth
>= NXT_CONF_JSON_MAX_DEPTH)) {
nxt_conf_json_parse_error(error, start,
"JSON nesting exceeds the maximum supported depth.");
return NULL;
}
nxt_conf_json_parse_depth++;
p = nxt_conf_json_parse_array(mp, value, start, end, error);
nxt_conf_json_parse_depth--;
return p;

case '"':
return nxt_conf_json_parse_string(mp, value, start, end, error);
Expand Down Expand Up @@ -1545,6 +1588,12 @@ nxt_conf_json_parse_object(nxt_mp_t *mp, nxt_conf_value_t *value, u_char *start,

name = p;

if (nxt_slow_path(count >= NXT_CONF_JSON_MAX_ELEMENTS)) {
nxt_conf_json_parse_error(error, p,
"JSON object has too many members.");
goto error;
}

count++;

member = nxt_mp_get(mp_temp, sizeof(nxt_conf_object_member_t));
Expand Down Expand Up @@ -1761,6 +1810,12 @@ nxt_conf_json_parse_array(nxt_mp_t *mp, nxt_conf_value_t *value, u_char *start,
break;
}

if (nxt_slow_path(count >= NXT_CONF_JSON_MAX_ELEMENTS)) {
nxt_conf_json_parse_error(error, p,
"JSON array has too many elements.");
goto error;
}

count++;

element = nxt_list_add(list);
Expand Down
12 changes: 12 additions & 0 deletions src/nxt_conf_validation.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1327,6 +1327,18 @@ static nxt_conf_vldt_object_t nxt_conf_vldt_app_processes_members[] = {
};


/*
* The isolation validator accepts arbitrary "executable" paths and
* lets the operator disable every isolation feature here. This is
* intentional: any peer who can write to the control socket already
* has the same authority as the unitd main process (see the
* SO_PEERCRED check landed in andypost/unit#14 — non-root local
* users are rejected at the socket layer, not by this validator).
* Allow-listing executable paths or forcing isolation = true here
* is a deployment policy decision, not a config-schema concern;
* deployments needing that should add a wrapping admission gate
* upstream of the control API.
*/
static nxt_conf_vldt_object_t nxt_conf_vldt_app_isolation_members[] = {
{
.name = nxt_string("namespaces"),
Expand Down
20 changes: 19 additions & 1 deletion src/nxt_php_sapi.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2389,7 +2389,25 @@ nxt_php_async_load_entrypoint(nxt_task_t *task, nxt_str_t *entrypoint)
nxt_php_request_callback ? Z_TYPE_P(nxt_php_request_callback) : -1);
}

/* DON'T call php_request_shutdown() - we want the callback to persist after fork */
/*
* DON'T call php_request_shutdown() — we want the callback zval to
* persist across the prototype → worker fork. But scrub any
* pending exception left in EG: if the entrypoint script raised
* something that wasn't caught before HttpServer->onRequest()
* stored the callback, every forked worker would inherit the
* exception on its first request. The callback zval itself
* (nxt_php_request_callback) is not on the exception path; it
* was registered explicitly by the userland code.
*
* Other EG globals (output buffers, error_reporting, the symbol
* table) are also inherited, but those are reset implicitly when
* the worker enters a fresh request_init. Exception state is
* the one that bites hardest because the next request's
* php_execute_script() can early-exit on a stale EG(exception).
*/
if (EG(exception) != NULL) {
zend_clear_exception();
}

return NXT_OK;
}
Expand Down
91 changes: 63 additions & 28 deletions src/ruby/nxt_ruby.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -283,11 +283,12 @@ nxt_ruby_start(nxt_task_t *task, nxt_process_data_t *data)
ruby_script("NGINX_Unit");

ruby_ctx.env = Qnil;
ruby_ctx.io_input = Qnil;
ruby_ctx.io_error = Qnil;
ruby_ctx.io_input_class = Qnil;
ruby_ctx.io_error_class = Qnil;
ruby_ctx.thread = Qnil;
ruby_ctx.ctx = NULL;
ruby_ctx.req = NULL;
ruby_ctx.req_seq = 0;

rack_init.task = task;
rack_init.script = &c->script;
Expand Down Expand Up @@ -570,8 +571,12 @@ nxt_ruby_rack_env_create(VALUE arg)

rb_hash_aset(hash_env, rb_str_new2("SCRIPT_NAME"), rb_str_new("", 0));
rb_hash_aset(hash_env, rb_str_new2("rack.version"), version);
rb_hash_aset(hash_env, rb_str_new2("rack.input"), rctx->io_input);
rb_hash_aset(hash_env, rb_str_new2("rack.errors"), rctx->io_error);
/*
* rack.input and rack.errors are minted per-request in
* nxt_ruby_rack_app_run; the template env intentionally omits
* them so a fresh, request-bound instance is the only thing
* each app callback ever sees.
*/
rb_hash_aset(hash_env, rb_str_new2("rack.multithread"),
nxt_ruby_threads > 1 ? Qtrue : Qfalse);
rb_hash_aset(hash_env, rb_str_new2("rack.multiprocess"), Qtrue);
Expand All @@ -591,33 +596,28 @@ nxt_ruby_rack_env_create(VALUE arg)
static int
nxt_ruby_init_io(nxt_ruby_ctx_t *rctx)
{
VALUE io_input, io_error;

io_input = nxt_ruby_stream_io_input_init();

rctx->io_input = rb_funcall(io_input, rb_intern("new"), 1,
(VALUE) (uintptr_t) rctx);
if (nxt_slow_path(rctx->io_input == Qnil)) {
/*
* Store only the class references on the ctx; instances are
* minted per request in nxt_ruby_rack_app_run so each instance
* can snapshot rctx->req_seq for its originating request.
*/
rctx->io_input_class = nxt_ruby_stream_io_input_init();
if (nxt_slow_path(rctx->io_input_class == Qnil)) {
nxt_unit_alert(NULL,
"Ruby: Failed to create environment 'rack.input' var");

"Ruby: Failed to register 'rack.input' class");
return NXT_UNIT_ERROR;
}

rb_gc_register_address(&rctx->io_input);

io_error = nxt_ruby_stream_io_error_init();
rb_gc_register_address(&rctx->io_input_class);

rctx->io_error = rb_funcall(io_error, rb_intern("new"), 1,
(VALUE) (uintptr_t) rctx);
if (nxt_slow_path(rctx->io_error == Qnil)) {
rctx->io_error_class = nxt_ruby_stream_io_error_init();
if (nxt_slow_path(rctx->io_error_class == Qnil)) {
nxt_unit_alert(NULL,
"Ruby: Failed to create environment 'rack.error' var");

"Ruby: Failed to register 'rack.errors' class");
return NXT_UNIT_ERROR;
}

rb_gc_register_address(&rctx->io_error);
rb_gc_register_address(&rctx->io_error_class);

return NXT_UNIT_OK;
}
Expand All @@ -641,6 +641,14 @@ nxt_ruby_request_handler_gvl(void *data)
req = data;

rctx = req->ctx->data;
/*
* Bump req_seq before publishing rctx->req: rack.input /
* rack.errors instances minted during this request snapshot
* req_seq at construction time, so any handle issued for an
* earlier request reads a mismatched seq and is rejected as
* stale (see nxt_ruby_bind_req).
*/
rctx->req_seq++;
rctx->req = req;

res = rb_protect(nxt_ruby_rack_app_run, (VALUE) (uintptr_t) req, &state);
Expand Down Expand Up @@ -669,12 +677,38 @@ nxt_ruby_rack_app_run(VALUE arg)
nxt_ruby_ctx_t *rctx;
nxt_unit_request_info_t *req;

VALUE io_input, io_error;

req = (nxt_unit_request_info_t *) arg;

rctx = req->ctx->data;

env = rb_hash_dup(rctx->env);

/*
* Mint per-request rack.input / rack.errors instances bound to
* the current rctx + req_seq. This is what prevents a handle
* captured during request A from operating on request B handled
* by the same worker: each instance snapshots req_seq, and
* stream-IO ops reject mismatches.
*/
io_input = nxt_ruby_stream_io_input_new(rctx->io_input_class, rctx);
if (nxt_slow_path(io_input == Qnil)) {
nxt_unit_req_alert(req,
"Ruby: Failed to create per-request 'rack.input'");
goto fail;
}

io_error = nxt_ruby_stream_io_error_new(rctx->io_error_class, rctx);
if (nxt_slow_path(io_error == Qnil)) {
nxt_unit_req_alert(req,
"Ruby: Failed to create per-request 'rack.errors'");
goto fail;
}

rb_hash_aset(env, rb_str_new2("rack.input"), io_input);
rb_hash_aset(env, rb_str_new2("rack.errors"), io_error);

rc = nxt_ruby_read_request(req, env);
if (nxt_slow_path(rc != NXT_UNIT_OK)) {
nxt_unit_req_alert(req,
Expand Down Expand Up @@ -1291,12 +1325,12 @@ nxt_ruby_exception_log(nxt_unit_request_info_t *req, uint32_t level,
static void
nxt_ruby_ctx_done(nxt_ruby_ctx_t *rctx)
{
if (rctx->io_input != Qnil) {
rb_gc_unregister_address(&rctx->io_input);
if (rctx->io_input_class != Qnil) {
rb_gc_unregister_address(&rctx->io_input_class);
}

if (rctx->io_error != Qnil) {
rb_gc_unregister_address(&rctx->io_error);
if (rctx->io_error_class != Qnil) {
rb_gc_unregister_address(&rctx->io_error_class);
}

if (rctx->env != Qnil) {
Expand Down Expand Up @@ -1456,9 +1490,10 @@ nxt_ruby_init_threads(nxt_ruby_app_conf_t *c)
rctx = &nxt_ruby_ctxs[i];

rctx->env = Qnil;
rctx->io_input = Qnil;
rctx->io_error = Qnil;
rctx->io_input_class = Qnil;
rctx->io_error_class = Qnil;
rctx->thread = Qnil;
rctx->req_seq = 0;
}

for (i = 0; i < c->threads - 1; i++) {
Expand Down
15 changes: 13 additions & 2 deletions src/ruby/nxt_ruby.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,15 +22,26 @@

typedef struct {
VALUE env;
VALUE io_input;
VALUE io_error;
VALUE io_input_class;
VALUE io_error_class;
VALUE thread;
nxt_unit_ctx_t *ctx;
nxt_unit_request_info_t *req;
/*
* Monotonic per-rctx counter, bumped on each request entry.
* Each rack.input / rack.errors instance snapshots this on
* creation; stream-IO ops reject calls whose snapshot no longer
* matches rctx->req_seq, so a handle captured during request A
* cannot read/write the body of request B handled by the same
* worker.
*/
uint64_t req_seq;
} nxt_ruby_ctx_t;


VALUE nxt_ruby_stream_io_input_init(void);
VALUE nxt_ruby_stream_io_error_init(void);
VALUE nxt_ruby_stream_io_input_new(VALUE class, nxt_ruby_ctx_t *rctx);
VALUE nxt_ruby_stream_io_error_new(VALUE class, nxt_ruby_ctx_t *rctx);

#endif /* _NXT_RUBY_H_INCLUDED_ */
Loading
Loading