Welcome! Log In Create A New Profile

Advanced

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

Maxim Dounin
December 03, 2023 10:18PM
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 <vl@wbsrv.ru>
> # 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, &old_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, &old_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);
> + }
> }
>
> 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; *b->last++ = LF;
>
> b->last = ngx_cpymem(b->last, "Auth-User: ", sizeof("Auth-User: ") - 1);
> - b->last = ngx_copy(b->last, login.data, login.len);
> + if (login.len) {
> + b->last = ngx_copy(b->last, login.data, login.len);
> + }
> *b->last++ = CR; *b->last++ = LF;
>
> b->last = ngx_cpymem(b->last, "Auth-Pass: ", sizeof("Auth-Pass: ") - 1);
> - b->last = ngx_copy(b->last, passwd.data, passwd.len);
> + if (passwd.len) {
> + b->last = ngx_copy(b->last, passwd.data, passwd.len);
> + }
> *b->last++ = CR; *b->last++ = LF;
>
> if (s->auth_method != NGX_MAIL_AUTH_PLAIN && s->salt.len) {
> @@ -1375,7 +1379,9 @@ ngx_mail_auth_http_create_request(ngx_ma
>
> b->last = ngx_cpymem(b->last, "Auth-SMTP-Helo: ",
> sizeof("Auth-SMTP-Helo: ") - 1);
> - b->last = ngx_copy(b->last, s->smtp_helo.data, s->smtp_helo.len);
> + if (s->smtp_helo.len) {
> + b->last = ngx_copy(b->last, s->smtp_helo.data, s->smtp_helo.len);
> + }
> *b->last++ = CR; *b->last++ = LF;
>
> b->last = ngx_cpymem(b->last, "Auth-SMTP-From: ",

If at all, these probably should check for login.len, passwd.len,
and s->smtp_helo.len, similarly to other cases nearby, and avoid
sending the headers altogether.

> diff --git a/src/stream/ngx_stream_script.c b/src/stream/ngx_stream_script.c
> --- a/src/stream/ngx_stream_script.c
> +++ b/src/stream/ngx_stream_script.c
> @@ -842,7 +842,9 @@ ngx_stream_script_copy_var_code(ngx_stre
>
> if (value && !value->not_found) {
> p = e->pos;
> - e->pos = ngx_copy(p, value->data, value->len);
> + if (value->len) {
> + e->pos = ngx_copy(p, value->data, value->len);
> + }
>
> ngx_log_debug2(NGX_LOG_DEBUG_STREAM,
> e->session->connection->log, 0,

Obviously enough, there should be a corresponding change in
ngx_http_script_copy_var_code().

Overall, similarly to the other patch in the series, I'm highly
sceptical about doing such scattered changes based on the UB
sanitizer reports from some test runs. Rather, we should use UB
sanitizer reports to identify problematic patterns, and fix these
patterns all other the code (if at all).

Further, in this particular case I tend to think that the problem
is not with nginx code, but rather with the memcpy() interface UB
sanitizer tries to enforce. It should be completely safe to call
memcpy(p, NULL, 0), and if it doesn't, we might consider adding
appropriate guards at interface level, such as in ngx_memcpy() /
ngx_cpymem() wrappers, and not in each call. Trying to check
length everywhere is just ugly and unreadable.

Also, while recent versions of gcc are known to miscompile some
code which uses memcpy(p, NULL, 0) (see [1], with "-O2" or with
"-O1 -ftree-vrp" optimization flags in my tests), I don't think
this affects nginx code. If it does, we might also consider
force-switching off relevant optimizations (if enabled, as we use
"-O1" by default).

[1] https://stackoverflow.com/questions/5243012/is-it-guaranteed-to-be-safe-to-perform-memcpy0-0-0

--
Maxim Dounin
http://mdounin.ru/
_______________________________________________
nginx-devel mailing list
nginx-devel@nginx.org
https://mailman.nginx.org/mailman/listinfo/nginx-devel
Subject Author Views Posted

[PATCH 0 of 2] [patch] some issues found by gcc undef sanitizer

Vladimir Homutov via nginx-devel 415 October 27, 2023 08:00AM

[PATCH 2 of 2] HTTP: suppressed possible overflow in interim r->uri_end calculation

Vladimir Homutov via nginx-devel 70 October 27, 2023 08:00AM

Re: [PATCH 2 of 2] HTTP: suppressed possible overflow in interim r->uri_end calculation

Maxim Dounin 82 October 27, 2023 02:52PM

[PATCH 0 of 2] [patch] some issues found by gcc undef sanitizer

Vladimir Homutov via nginx-devel 60 November 10, 2023 04:14AM

[PATCH 1 of 2] HTTP: uniform overflow checks in ngx_http_alloc_large_header_buffer

Vladimir Homutov via nginx-devel 79 November 10, 2023 04:14AM

Re: [PATCH 1 of 2] HTTP: uniform overflow checks in ngx_http_alloc_large_header_buffer

Maxim Dounin 52 November 27, 2023 10:00PM

Re: [PATCH 1 of 2] HTTP: uniform overflow checks in ngx_http_alloc_large_header_buffer

Vladimir Homutov via nginx-devel 50 November 29, 2023 03:24AM

Re: [PATCH 1 of 2] HTTP: uniform overflow checks in ngx_http_alloc_large_header_buffer

Maxim Dounin 53 November 29, 2023 08:16PM

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

Vladimir Homutov via nginx-devel 69 October 27, 2023 08:00AM

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

Maxim Dounin 59 December 03, 2023 10:18PM



Sorry, you do not have permission to post/reply in this forum.

Online Users

Guests: 245
Record Number of Users: 8 on April 13, 2023
Record Number of Guests: 421 on December 02, 2018
Powered by nginx      Powered by FreeBSD      PHP Powered      Powered by MariaDB      ipv6 ready