Re: [PATCH 1 of 2] Core: avoid calling memcpy() in edge cases

2023-12-03 Thread Maxim Dounin 
Hello!

On Fri, Oct 27, 2023 at 02:58:44PM +0300, Vladimir Homutov via nginx-devel 
wrote:

> Patch subject is complete summary.
> 
> 
>  src/core/ngx_cycle.c |  10 ++
>  src/core/ngx_resolver.c  |   2 +-
>  src/core/ngx_string.c|  15 +++
>  src/http/modules/ngx_http_proxy_module.c |   4 ++--
>  src/http/ngx_http_file_cache.c   |   4 +++-
>  src/http/ngx_http_variables.c|   3 +++
>  src/mail/ngx_mail_auth_http_module.c |  12 +---
>  src/stream/ngx_stream_script.c   |   4 +++-
>  8 files changed, 42 insertions(+), 12 deletions(-)
> 
> 

> # HG changeset patch
> # User Vladimir Khomutov 
> # Date 1698407658 -10800
> #  Fri Oct 27 14:54:18 2023 +0300
> # Node ID ef9f124b156aff0e9f66057e438af835bd7a60d2
> # Parent  ea1f29c2010cda4940b741976f103d547308815a
> Core: avoid calling memcpy() in edge cases.
> 
> diff --git a/src/core/ngx_cycle.c b/src/core/ngx_cycle.c
> --- a/src/core/ngx_cycle.c
> +++ b/src/core/ngx_cycle.c
> @@ -115,10 +115,12 @@ ngx_init_cycle(ngx_cycle_t *old_cycle)
>  old_cycle->conf_file.len + 1);
>  
>  cycle->conf_param.len = old_cycle->conf_param.len;
> -cycle->conf_param.data = ngx_pstrdup(pool, _cycle->conf_param);
> -if (cycle->conf_param.data == NULL) {
> -ngx_destroy_pool(pool);
> -return NULL;
> +if (cycle->conf_param.len) {
> +cycle->conf_param.data = ngx_pstrdup(pool, _cycle->conf_param);
> +if (cycle->conf_param.data == NULL) {
> +ngx_destroy_pool(pool);
> +return NULL;
> +}
>  }
>  
>  
> diff --git a/src/core/ngx_resolver.c b/src/core/ngx_resolver.c
> --- a/src/core/ngx_resolver.c
> +++ b/src/core/ngx_resolver.c
> @@ -4206,7 +4206,7 @@ ngx_resolver_dup(ngx_resolver_t *r, void
>  
>  dst = ngx_resolver_alloc(r, size);
>  
> -if (dst == NULL) {
> +if (dst == NULL || size == 0 || src == NULL) {
>  return dst;
>  }
>  

This looks simply wrong: ngx_resolver_dup() with src == NULL and 
with size != 0 should dereference the NULL pointer and segfault.

Also, I can't say I'm happy with allocation error handling mixed 
with (size == 0) special case handling.

> diff --git a/src/core/ngx_string.c b/src/core/ngx_string.c
> --- a/src/core/ngx_string.c
> +++ b/src/core/ngx_string.c
> @@ -252,6 +252,11 @@ ngx_vslprintf(u_char *buf, u_char *last,
>  case 'V':
>  v = va_arg(args, ngx_str_t *);
>  
> +if (v->len == 0 || v->data == NULL) {
> +fmt++;
> +continue;
> +}
> +
>  buf = ngx_sprintf_str(buf, last, v->data, v->len, hex);
>  fmt++;
>  
> @@ -260,6 +265,11 @@ ngx_vslprintf(u_char *buf, u_char *last,
>  case 'v':
>  vv = va_arg(args, ngx_variable_value_t *);
>  
> +if (vv->len == 0 || vv->data == NULL) {
> +fmt++;
> +continue;
> +}
> +
>  buf = ngx_sprintf_str(buf, last, vv->data, vv->len, hex);
>  fmt++;
>  
> @@ -268,6 +278,11 @@ ngx_vslprintf(u_char *buf, u_char *last,
>  case 's':
>  p = va_arg(args, u_char *);
>  
> +if (slen == 0 || p == NULL) {
> +fmt++;
> +continue;
> +}
> +
>  buf = ngx_sprintf_str(buf, last, p, slen, hex);
>  fmt++;
>  

I tend to think that these should be handled in ngx_sprintf_str() 
or in ngx_cpymem(), if at all, see below.

> diff --git a/src/http/modules/ngx_http_proxy_module.c 
> b/src/http/modules/ngx_http_proxy_module.c
> --- a/src/http/modules/ngx_http_proxy_module.c
> +++ b/src/http/modules/ngx_http_proxy_module.c
> @@ -1205,7 +1205,7 @@ ngx_http_proxy_create_key(ngx_http_reque
>  
>  key->data = p;
>  
> -if (r->valid_location) {
> +if (r->valid_location && ctx->vars.uri.len) {
>  p = ngx_copy(p, ctx->vars.uri.data, ctx->vars.uri.len);
>  }
>  
> @@ -1422,7 +1422,7 @@ ngx_http_proxy_create_request(ngx_http_r
>  b->last = ngx_copy(b->last, r->unparsed_uri.data, 
> r->unparsed_uri.len);
>  
>  } else {
> -if (r->valid_location) {
> +if (r->valid_location && ctx->vars.uri.len) {
>  b->last = ngx_copy(b->last, ctx->vars.uri.data, 
> ctx->vars.uri.len);
>  }
>  
> diff --git a/src/http/ngx_http_file_cache.c b/src/http/ngx_http_file_cache.c
> --- a/src/http/ngx_http_file_cache.c
> +++ b/src/http/ngx_http_file_cache.c
> @@ -1270,7 +1270,9 @@ ngx_http_file_cache_set_header(ngx_http_
>  
>  if (c->etag.len <= NGX_HTTP_CACHE_ETAG_LEN) {
>  h->etag_len = (u_char) c->etag.len;
> -ngx_memcpy(h->etag, c->etag.data, c->etag.len);
> +if (c->etag.len) {
> +ngx_memcpy(h->etag, c->etag.data, c->etag.len);
> +}
>  }
>  
>

[PATCH 1 of 2] Core: avoid calling memcpy() in edge cases

2023-10-27 Thread Vladimir Homutov via nginx-devel 
Patch subject is complete summary.


 src/core/ngx_cycle.c |  10 ++
 src/core/ngx_resolver.c  |   2 +-
 src/core/ngx_string.c|  15 +++
 src/http/modules/ngx_http_proxy_module.c |   4 ++--
 src/http/ngx_http_file_cache.c   |   4 +++-
 src/http/ngx_http_variables.c|   3 +++
 src/mail/ngx_mail_auth_http_module.c |  12 +---
 src/stream/ngx_stream_script.c   |   4 +++-
 8 files changed, 42 insertions(+), 12 deletions(-)


# HG changeset patch
# User Vladimir Khomutov 
# Date 1698407658 -10800
#  Fri Oct 27 14:54:18 2023 +0300
# Node ID ef9f124b156aff0e9f66057e438af835bd7a60d2
# Parent  ea1f29c2010cda4940b741976f103d547308815a
Core: avoid calling memcpy() in edge cases.

diff --git a/src/core/ngx_cycle.c b/src/core/ngx_cycle.c
--- a/src/core/ngx_cycle.c
+++ b/src/core/ngx_cycle.c
@@ -115,10 +115,12 @@ ngx_init_cycle(ngx_cycle_t *old_cycle)
 old_cycle->conf_file.len + 1);
 
 cycle->conf_param.len = old_cycle->conf_param.len;
-cycle->conf_param.data = ngx_pstrdup(pool, _cycle->conf_param);
-if (cycle->conf_param.data == NULL) {
-ngx_destroy_pool(pool);
-return NULL;
+if (cycle->conf_param.len) {
+cycle->conf_param.data = ngx_pstrdup(pool, _cycle->conf_param);
+if (cycle->conf_param.data == NULL) {
+ngx_destroy_pool(pool);
+return NULL;
+}
 }
 
 
diff --git a/src/core/ngx_resolver.c b/src/core/ngx_resolver.c
--- a/src/core/ngx_resolver.c
+++ b/src/core/ngx_resolver.c
@@ -4206,7 +4206,7 @@ ngx_resolver_dup(ngx_resolver_t *r, void
 
 dst = ngx_resolver_alloc(r, size);
 
-if (dst == NULL) {
+if (dst == NULL || size == 0 || src == NULL) {
 return dst;
 }
 
diff --git a/src/core/ngx_string.c b/src/core/ngx_string.c
--- a/src/core/ngx_string.c
+++ b/src/core/ngx_string.c
@@ -252,6 +252,11 @@ ngx_vslprintf(u_char *buf, u_char *last,
 case 'V':
 v = va_arg(args, ngx_str_t *);
 
+if (v->len == 0 || v->data == NULL) {
+fmt++;
+continue;
+}
+
 buf = ngx_sprintf_str(buf, last, v->data, v->len, hex);
 fmt++;
 
@@ -260,6 +265,11 @@ ngx_vslprintf(u_char *buf, u_char *last,
 case 'v':
 vv = va_arg(args, ngx_variable_value_t *);
 
+if (vv->len == 0 || vv->data == NULL) {
+fmt++;
+continue;
+}
+
 buf = ngx_sprintf_str(buf, last, vv->data, vv->len, hex);
 fmt++;
 
@@ -268,6 +278,11 @@ ngx_vslprintf(u_char *buf, u_char *last,
 case 's':
 p = va_arg(args, u_char *);
 
+if (slen == 0 || p == NULL) {
+fmt++;
+continue;
+}
+
 buf = ngx_sprintf_str(buf, last, p, slen, hex);
 fmt++;
 
diff --git a/src/http/modules/ngx_http_proxy_module.c b/src/http/modules/ngx_http_proxy_module.c
--- a/src/http/modules/ngx_http_proxy_module.c
+++ b/src/http/modules/ngx_http_proxy_module.c
@@ -1205,7 +1205,7 @@ ngx_http_proxy_create_key(ngx_http_reque
 
 key->data = p;
 
-if (r->valid_location) {
+if (r->valid_location && ctx->vars.uri.len) {
 p = ngx_copy(p, ctx->vars.uri.data, ctx->vars.uri.len);
 }
 
@@ -1422,7 +1422,7 @@ ngx_http_proxy_create_request(ngx_http_r
 b->last = ngx_copy(b->last, r->unparsed_uri.data, r->unparsed_uri.len);
 
 } else {
-if (r->valid_location) {
+if (r->valid_location && ctx->vars.uri.len) {
 b->last = ngx_copy(b->last, ctx->vars.uri.data, ctx->vars.uri.len);
 }
 
diff --git a/src/http/ngx_http_file_cache.c b/src/http/ngx_http_file_cache.c
--- a/src/http/ngx_http_file_cache.c
+++ b/src/http/ngx_http_file_cache.c
@@ -1270,7 +1270,9 @@ ngx_http_file_cache_set_header(ngx_http_
 
 if (c->etag.len <= NGX_HTTP_CACHE_ETAG_LEN) {
 h->etag_len = (u_char) c->etag.len;
-ngx_memcpy(h->etag, c->etag.data, c->etag.len);
+if (c->etag.len) {
+ngx_memcpy(h->etag, c->etag.data, c->etag.len);
+}
 }
 
 if (c->vary.len) {
diff --git a/src/http/ngx_http_variables.c b/src/http/ngx_http_variables.c
--- a/src/http/ngx_http_variables.c
+++ b/src/http/ngx_http_variables.c
@@ -2157,6 +2157,9 @@ ngx_http_variable_request_body(ngx_http_
 
 for ( /* void */ ; cl; cl = cl->next) {
 buf = cl->buf;
+if (buf->last == buf->pos) {
+continue;
+}
 p = ngx_cpymem(p, buf->pos, buf->last - buf->pos);
 }
 
diff --git a/src/mail/ngx_mail_auth_http_module.c b/src/mail/ngx_mail_auth_http_module.c
--- a/src/mail/ngx_mail_auth_http_module.c
+++ b/src/mail/ngx_mail_auth_http_module.c
@@ -1314,11 +1314,15 @@ ngx_mail_auth_http_create_request(ngx_ma
 *b->last++ = CR;