From 6e2f16c48ef51e7e3b802eb521000a7ceb08b327 Mon Sep 17 00:00:00 2001 From: lijunlong Date: Sun, 25 Oct 2026 17:20:25 +0800 Subject: [PATCH 1/2] bugfix: validate array variables before use. Store array values as references containing a module tag and an array pointer. Check the reference size and tag before mapping or joining, and copy references safely from potentially unaligned NDK buffers. Keep validation independent of the number of arrays in the request. Preserve copied references and arrays used across internal redirects, named locations, and subrequests. Reject overflowing initial capacities and format variable values correctly in error logs. Add regression coverage for invalid scalars, invalid tags, copied references, array lifetimes, and creation of many arrays. All 78 assertions pass against the nginx 1.31.8 test build. --- README.md | 4 +- src/ngx_http_array_var_module.c | 101 +++++++++++++------ t/invalid-array.t | 169 ++++++++++++++++++++++++++++++++ t/sanity.t | 122 +++++++++++++++++++++++ 4 files changed, 366 insertions(+), 30 deletions(-) create mode 100644 t/invalid-array.t diff --git a/README.md b/README.md index d6c888c..1b01b7e 100644 --- a/README.md +++ b/README.md @@ -57,8 +57,8 @@ Description This module provides array typed nginx variables to `nginx.conf`. -Under the hood, this module just "abuses" the nginx string values to hold binary pointers -to C data structures (NGINX core's `ngx_array_t` struct on the C land). +Under the hood, this module uses nginx string values to hold tagged references +to NGINX core's `ngx_array_t` structures. The array type gives `nginx.onf` wonderful capabilities of handling value lists. Nowadays, however, you are highly recommended to use the [ngx_lua](https://github.com/openresty/lua-nginx-module) module diff --git a/src/ngx_http_array_var_module.c b/src/ngx_http_array_var_module.c index 76c4df7..32e93ca 100644 --- a/src/ngx_http_array_var_module.c +++ b/src/ngx_http_array_var_module.c @@ -17,6 +17,12 @@ static ngx_str_t ngx_http_array_it_key = ngx_string("array_it"); +typedef struct { + ngx_module_t *tag; + ngx_array_t *array; +} ngx_http_array_var_ref_t; + + typedef struct { ngx_uint_t nargs; } ngx_http_array_split_data_t; @@ -128,6 +134,55 @@ ngx_module_t ngx_http_array_var_module = { }; +static ngx_array_t * +ngx_http_array_var_create(ngx_http_request_t *r, ngx_uint_t n, ngx_str_t *res) +{ + ngx_array_t *array; + ngx_http_array_var_ref_t *a_ref; + + if (n > NGX_MAX_SIZE_T_VALUE / sizeof(ngx_str_t)) { + return NULL; + } + + array = ngx_array_create(r->pool, n, sizeof(ngx_str_t)); + if (array == NULL) { + return NULL; + } + + a_ref = ngx_palloc(r->pool, sizeof(ngx_http_array_var_ref_t)); + if (a_ref == NULL) { + return NULL; + } + + a_ref->tag = &ngx_http_array_var_module; + a_ref->array = array; + res->data = (u_char *) a_ref; + res->len = sizeof(ngx_http_array_var_ref_t); + + return array; +} + + +static ngx_array_t * +ngx_http_array_var_get(ngx_http_variable_value_t *v) +{ + ngx_http_array_var_ref_t a_ref; + + if (v->len != sizeof(ngx_http_array_var_ref_t)) { + return NULL; + } + + /* NDK copies values into buffers that need not be aligned. */ + ngx_memcpy(&a_ref, v->data, sizeof(ngx_http_array_var_ref_t)); + + if (a_ref.tag != &ngx_http_array_var_module) { + return NULL; + } + + return a_ref.array; +} + + static char * ngx_http_array_split(ngx_conf_t *cf, ngx_command_t *cmd, void *conf) { @@ -432,7 +487,7 @@ ngx_http_array_var_split(ngx_http_request_t *r, ngx_str_t *res, max = ngx_atosz(v[2].data, v[2].len); if (max == NGX_ERROR) { ngx_log_error(NGX_LOG_ERR, r->connection->log, 0, - "array_split: invalid max items: \"%V\"", + "array_split: invalid max items: \"%v\"", &v[2]); return NGX_ERROR; @@ -445,7 +500,7 @@ ngx_http_array_var_split(ngx_http_request_t *r, ngx_str_t *res, len = max; } - array = ngx_array_create(r->pool, len, sizeof(ngx_str_t)); + array = ngx_http_array_var_create(r, len, res); if (array == NULL) { return NGX_ERROR; } @@ -515,9 +570,6 @@ ngx_http_array_var_split(ngx_http_request_t *r, ngx_str_t *res, dd("split: array size: %d", (int) array->nelts); dd("split array ptr: %p", array); - res->data = (u_char *) array; - res->len = sizeof(ngx_array_t); - return NGX_OK; } @@ -540,16 +592,15 @@ ngx_http_array_var_map(ngx_http_request_t *r, ngx_str_t *res, return NGX_OK; } - if (v[0].len != sizeof(ngx_array_t)) { + array = ngx_http_array_var_get(&v[0]); + if (array == NULL) { ngx_log_error(NGX_LOG_ERR, r->connection->log, 0, - "array_join: invalid array variable value in the 2nd " - "argument: \"%.*s\"", &v[0]); + "array_map: invalid array variable value in the 2nd " + "argument: \"%v\"", &v[0]); return NGX_ERROR; } - array = (ngx_array_t *) v[0].data; - value = array->elts; array_it = ngx_http_get_indexed_variable(r, conf->array_it_index); @@ -559,10 +610,11 @@ ngx_http_array_var_map(ngx_http_request_t *r, ngx_str_t *res, if (conf->in_place) { new_array = array; + res->data = v[0].data; + res->len = v[0].len; } else { - new_array = ngx_array_create(r->pool, array->nelts, - sizeof(ngx_str_t)); + new_array = ngx_http_array_var_create(r, array->nelts, res); if (new_array == NULL) { return NGX_ERROR; } @@ -600,9 +652,6 @@ ngx_http_array_var_map(ngx_http_request_t *r, ngx_str_t *res, array_it->not_found = 1; array_it->valid = 0; - res->data = (u_char *) new_array; - res->len = sizeof(ngx_array_t); - return NGX_OK; } @@ -632,24 +681,24 @@ ngx_http_array_var_map_op(ngx_http_request_t *r, return NGX_ERROR; } - if (v[1].len != sizeof(ngx_array_t)) { + array = ngx_http_array_var_get(&v[1]); + if (array == NULL) { ngx_log_error(NGX_LOG_ERR, r->connection->log, 0, "array_map_op: invalid array variable value in the 2nd " - "argument: \"%.*s\"", &v[0]); + "argument: \"%v\"", &v[1]); return NGX_ERROR; } - array = (ngx_array_t *) v[1].data; - value = array->elts; if (conf->in_place) { new_array = array; + res->data = v[1].data; + res->len = v[1].len; } else { - new_array = ngx_array_create(r->pool, array->nelts, - sizeof(ngx_str_t)); + new_array = ngx_http_array_var_create(r, array->nelts, res); if (new_array == NULL) { return NGX_ERROR; } @@ -677,9 +726,6 @@ ngx_http_array_var_map_op(ngx_http_request_t *r, } } - res->data = (u_char *) new_array; - res->len = sizeof(ngx_array_t); - return NGX_OK; } @@ -699,16 +745,15 @@ ngx_http_array_var_join(ngx_http_request_t *r, dd("sep %.*s", sep->len, sep->data); - if (v[1].len != sizeof(ngx_array_t)) { + array = ngx_http_array_var_get(&v[1]); + if (array == NULL) { ngx_log_error(NGX_LOG_ERR, r->connection->log, 0, "array_join: invalid array variable value in the " - "2nd argument: \"%V\"", &v[1]); + "2nd argument: \"%v\"", &v[1]); return NGX_ERROR; } - array = (ngx_array_t *) v[1].data; - dd("join array ptr %p", array); dd("array->nelts: %d", (int) array->nelts); diff --git a/t/invalid-array.t b/t/invalid-array.t new file mode 100644 index 0000000..91ab84f --- /dev/null +++ b/t/invalid-array.t @@ -0,0 +1,169 @@ +# vi:filetype= + +use lib 'lib'; +use Config (); +use Test::Nginx::Socket; + +repeat_each(1); + +plan tests => repeat_each() * 3 * blocks(); + +no_long_string(); + +run_tests(); + +__DATA__ + +=== TEST 1: array map reports the scalar value after a successful array operation +--- config + location /foo { + error_page 500 = @invalid_array; + array_split ',' 'a,b' to=$array; + set $scalar 'invalid'; + array_map '[$array_it]' $scalar; + echo 'unexpected success'; + } + + location @invalid_array { + return 500 "invalid array\n"; + } +--- request +GET /foo +--- error_code: 500 +--- response_body +invalid array +--- grep_error_log eval +qr/array_map: invalid array variable value in the 2nd argument: "[^"]*"/ +--- grep_error_log_out +array_map: invalid array variable value in the 2nd argument: "invalid" + + + +=== TEST 2: array map op reports the scalar value rather than the directive name +--- config + location /foo { + error_page 500 = @invalid_array; + array_split ',' 'a,b' to=$array; + set $scalar 'invalid'; + array_map_op set_quote_sql_str $scalar; + echo 'unexpected success'; + } + + location @invalid_array { + return 500 "invalid array\n"; + } +--- request +GET /foo +--- error_code: 500 +--- response_body +invalid array +--- grep_error_log eval +qr/array_map_op: invalid array variable value in the 2nd argument: "[^"]*"/ +--- grep_error_log_out +array_map_op: invalid array variable value in the 2nd argument: "invalid" + + + +=== TEST 3: array join reports the scalar value +--- config + location /foo { + error_page 500 = @invalid_array; + array_split ',' 'a,b' to=$array; + set $scalar 'invalid'; + array_join ',' $scalar; + echo 'unexpected success'; + } + + location @invalid_array { + return 500 "invalid array\n"; + } +--- request +GET /foo +--- error_code: 500 +--- response_body +invalid array +--- grep_error_log eval +qr/array_join: invalid array variable value in the 2nd argument: "[^"]*"/ +--- grep_error_log_out +array_join: invalid array variable value in the 2nd argument: "invalid" + + + +=== TEST 4: array join rejects a scalar the same size as an array reference +--- config eval +my $scalar = 'a' x (2 * $Config::Config{ptrsize}); +sprintf q{ + location /foo { + error_page 500 = @invalid_array; + set $scalar '%s'; + array_join ',' $scalar; + echo 'unexpected success'; + } + + location @invalid_array { + return 500 "invalid array\n"; + } +}, $scalar; +--- request +GET /foo +--- error_code: 500 +--- response_body +invalid array +--- error_log +array_join: invalid array variable value in the 2nd argument: + + + +=== TEST 5: array map op rejects an array reference with a null tag +--- config eval +my $scalar = '0' x (4 * $Config::Config{ptrsize}); +sprintf q{ + location /foo { + error_page 500 = @invalid_array; + array_split ',' 'a,b' to=$array; + set_decode_hex $scalar '%s'; + array_map_op set_quote_sql_str $scalar; + echo 'unexpected success'; + } + + location @invalid_array { + return 500 "invalid array\n"; + } +}, $scalar; +--- request +GET /foo +--- error_code: 500 +--- response_body +invalid array +--- error_log +array_map_op: invalid array variable value in the 2nd argument: + + + +=== TEST 6: array map rejects an invalid tag with a valid array pointer +--- config eval +my $digits = 2 * $Config::Config{ptrsize}; +sprintf q{ + location /foo { + error_page 500 = @invalid_array; + array_split ',' 'a,b' to=$array; + set_encode_hex $encoded $array; + if ($encoded ~ "^.{%d}(.{%d})$") { + set $bad_hex "%s$1"; + } + set_decode_hex $scalar $bad_hex; + array_map '[$array_it]' $scalar; + echo 'unexpected success'; + } + + location @invalid_array { + return 500 "invalid array\n"; + } +}, $digits, $digits, '0' x $digits; +--- request +GET /foo +--- error_code: 500 +--- response_body +invalid array +--- error_log +array_map: invalid array variable value in the 2nd argument: diff --git a/t/sanity.t b/t/sanity.t index 0db489a..ab68817 100644 --- a/t/sanity.t +++ b/t/sanity.t @@ -369,3 +369,125 @@ GET /foo?names=bob,marry,nomas --- response_body "bob"+"marry"+"nomas" + + +=== TEST 25: copied array reference +--- config + location /foo { + array_split ',' 'a,b,c' to=$items; + set $alias "$items"; + array_join ':' $alias to=$out; + echo $out; + } +--- request +GET /foo +--- response_body +a:b:c + + + +=== TEST 26: internal redirect preserves an array reference +--- config + location /foo { + array_split ',' 'a,b,c' to=$items; + rewrite ^ /finish last; + } + + location /finish { + array_join ':' $items to=$out; + echo $out; + } +--- request +GET /foo +--- response_body +a:b:c + + + +=== TEST 27: named location preserves an array reference +--- config + location /foo { + error_page 418 = @finish; + array_split ',' 'a,b,c' to=$items; + return 418; + } + + location @finish { + array_map '[$array_it]' $items; + array_join ':' $items to=$out; + echo $out; + } +--- request +GET /foo +--- response_body +[a]:[b]:[c] + + + +=== TEST 28: parent and subrequest arrays share a pool +--- config + location /foo { + array_split ',' 'a,b,c' to=$items; + array_join ':' $items to=$parent_out; + echo_location /child; + echo $parent_out; + } + + location /child { + array_split ',' 'd,e' to=$items; + rewrite ^ /child-finish last; + } + + location /child-finish { + array_join ':' $items to=$out; + echo $out; + } +--- request +GET /foo +--- response_body +d:e +a:b:c + + + +=== TEST 29: mapped arrays and the original remain valid +--- config + location /foo { + array_split ',' 'a,b,c' to=$items; + array_map '[$array_it]' $items to=$mapped; + array_map_op set_quote_sql_str $mapped to=$quoted; + array_join ':' $items to=$original; + array_join ':' $mapped to=$mapped_out; + array_join ':' $quoted to=$quoted_out; + echo "$original|$mapped_out|$quoted_out"; + } +--- request +GET /foo +--- response_body +a:b:c|[a]:[b]:[c]|'[a]':'[b]':'[c]' + + + +=== TEST 30: a copied array reference survives creation of many arrays +--- config eval +my $config = q! + location /foo { + array_split ',' 'first,second' to=$array; + set $alias "$array"; +!; + +for (1 .. 512) { + $config .= q! array_split ',' 'x,y' to=$scratch; +!; +} + +$config . q! + array_map '[$array_it]' $alias; + array_join ':' $array to=$out; + echo $out; + } +!; +--- request +GET /foo +--- response_body +[first]:[second] From 6f2b1de68a80ce5d541737685ca28b6862648ae2 Mon Sep 17 00:00:00 2001 From: lijunlong Date: Sun, 25 Oct 2026 18:06:51 +0800 Subject: [PATCH 2/2] tests: update util/build.sh. --- util/build.sh | 13 ++++++++----- 1 file changed, 8 insertions(+), 5 deletions(-) diff --git a/util/build.sh b/util/build.sh index c9b2873..aa24568 100755 --- a/util/build.sh +++ b/util/build.sh @@ -8,6 +8,9 @@ version=$1 force=$2 ngx-build $force $version \ + --with-ld-opt="-L$OPENSSL_LIB -L$PCRE_LIB -Wl,-rpath,$OPENSSL_LIB:$PCRE_LIB:$LIBDRIZZLE_LIB:/usr/local/lib" \ + --with-cc-opt="-DDEBUG_MALLOC -I $OPENSSL_INC -I $PCRE_INC" \ + --with-http_ssl_module \ --without-mail_pop3_module \ --without-mail_imap_module \ --without-mail_smtp_module \ @@ -18,11 +21,11 @@ ngx-build $force $version \ --without-http_autoindex_module \ --without-http_auth_basic_module \ --without-http_userid_module \ - --add-module=$root/../echo-nginx-module \ - --add-module=$root/../ndk-nginx-module \ - --add-module=$root/../set-misc-nginx-module \ - --add-module=$root $opts \ - --with-debug || exit 1 + --add-module=$root/../echo-nginx-module \ + --add-module=$root/../ndk-nginx-module \ + --add-module=$root/../set-misc-nginx-module \ + --add-module=$root $opts \ + --with-debug || exit 1 #--add-module=$home/work/ndk \ #--without-http_ssi_module # we cannot disable ssi because echo_location_async depends on it (i dunno why?!)