Hi Piotr,

On Wed, Feb 28, 2024 at 01:21:40AM +0000, Piotr Sikora via nginx-devel wrote:
> # HG changeset patch
> # User Piotr Sikora <piotr@aviatrix.com>
> # Date 1708977621 0
> # Mon Feb 26 20:00:21 2024 +0000
> # Branch patch005
> # Node ID fe6f8a72d42970df176ea53f4f0aea16947ba5b8
> # Parent 52936793ac076072c3544aa4e27f973d2f8fecda
> Geo: fix uninitialized memory access.
>
> Found with MemorySanitizer.
>
> Signed-off-by: Piotr Sikora <piotr@aviatrix.com>
>
> diff -r 52936793ac07 -r fe6f8a72d429 src/http/modules/ngx_http_geo_module.c
> --- a/src/http/modules/ngx_http_geo_module.c Mon Feb 26 20:00:19 2024 +0000
> +++ b/src/http/modules/ngx_http_geo_module.c Mon Feb 26 20:00:21 2024 +0000
> @@ -1259,7 +1259,7 @@
> return gvvn->value;
> }
>
> - val = ngx_palloc(ctx->pool, sizeof(ngx_http_variable_value_t));
> + val = ngx_pcalloc(ctx->pool, sizeof(ngx_http_variable_value_t));
> if (val == NULL) {
> return NULL;
> }
> diff -r 52936793ac07 -r fe6f8a72d429 src/stream/ngx_stream_geo_module.c
> --- a/src/stream/ngx_stream_geo_module.c Mon Feb 26 20:00:19 2024 +0000
> +++ b/src/stream/ngx_stream_geo_module.c Mon Feb 26 20:00:21 2024 +0000
> @@ -1209,7 +1209,7 @@
> return gvvn->value;
> }
>
> - val = ngx_palloc(ctx->pool, sizeof(ngx_stream_variable_value_t));
> + val = ngx_pcalloc(ctx->pool, sizeof(ngx_stream_variable_value_t));
> if (val == NULL) {
> return NULL;
> }
> _______________________________________________
> nginx-devel mailing list
> nginx-devel@nginx.org
> https://mailman.nginx.org/mailman/listinfo/nginx-devel

Thanks for the patch, looks valid, except we no longer need to explicitly
initialize fields to zero. Also, I think we need more details about the
uninitialized memory access. See updated patch.

--
Roman Arutyunyan
# HG changeset patch
# User Piotr Sikora <piotr@aviatrix.com>
# Date 1710427040 -14400
# Thu Mar 14 18:37:20 2024 +0400
# Node ID bd1a4807521bd830ab73c11d6ff3c9b75f5c45f0
# Parent 2ed3f57dca0a664340bca2236c7d614902db4180
Geo: fix uninitialized memory access.

While copying ngx_http_variable_value_t structures to geo binary base in
ngx_http_geo_copy_values(), uninitialized parts of these structures are copied
as well. These include the "escape" field and possible holes. Calculating
crc32 of this data triggers uninitialized memory access.

Found with MemorySanitizer.

Signed-off-by: Piotr Sikora <piotr@aviatrix.com>

diff --git a/src/http/modules/ngx_http_geo_module.c b/src/http/modules/ngx_http_geo_module.c
--- a/src/http/modules/ngx_http_geo_module.c
+++ b/src/http/modules/ngx_http_geo_module.c
@@ -1259,7 +1259,7 @@ ngx_http_geo_value(ngx_conf_t *cf, ngx_h
return gvvn->value;
}

- val = ngx_palloc(ctx->pool, sizeof(ngx_http_variable_value_t));
+ val = ngx_pcalloc(ctx->pool, sizeof(ngx_http_variable_value_t));
if (val == NULL) {
return NULL;
}
@@ -1271,8 +1271,6 @@ ngx_http_geo_value(ngx_conf_t *cf, ngx_h
}

val->valid = 1;
- val->no_cacheable = 0;
- val->not_found = 0;

gvvn = ngx_palloc(ctx->temp_pool,
sizeof(ngx_http_geo_variable_value_node_t));
diff --git a/src/stream/ngx_stream_geo_module.c b/src/stream/ngx_stream_geo_module.c
--- a/src/stream/ngx_stream_geo_module.c
+++ b/src/stream/ngx_stream_geo_module.c
@@ -1209,7 +1209,7 @@ ngx_stream_geo_value(ngx_conf_t *cf, ngx
return gvvn->value;
}

- val = ngx_palloc(ctx->pool, sizeof(ngx_stream_variable_value_t));
+ val = ngx_pcalloc(ctx->pool, sizeof(ngx_stream_variable_value_t));
if (val == NULL) {
return NULL;
}
@@ -1221,8 +1221,6 @@ ngx_stream_geo_value(ngx_conf_t *cf, ngx
}

val->valid = 1;
- val->no_cacheable = 0;
- val->not_found = 0;

gvvn = ngx_palloc(ctx->temp_pool,
sizeof(ngx_stream_geo_variable_value_node_t));
_______________________________________________
nginx-devel mailing list
nginx-devel@nginx.org
https://mailman.nginx.org/mailman/listinfo/nginx-devel