[nginx] Cleaned up r->headers_out.headers allocation error handling.

Sorin Manole sorin.v.manole at gmail.com
Mon Apr 24 16:23:13 UTC 2017


Hello!

>> For the Cache-Control header, the fix is to postpone pushing
>> r->headers_out.cache_control until its value is completed.

Why not do the same thing for a bunch of other places like:
ngx_http_auth_basic_set_realm
ngx_http_range_not_satisfiable

That is, for the latter, first initialize content_range, and then add it to
headers.
Looks like a safer strategy then rolling back a change in case of failure.

Thank you!


2017-04-20 19:33 GMT+03:00 Sergey Kandaurov <pluknet at nginx.com>:

> details:   http://hg.nginx.org/nginx/rev/0cdee26605f3
> branches:
> changeset: 6986:0cdee26605f3
> user:      Sergey Kandaurov <pluknet at nginx.com>
> date:      Thu Apr 20 18:26:37 2017 +0300
> description:
> Cleaned up r->headers_out.headers allocation error handling.
>
> If initialization of a header failed for some reason after ngx_list_push(),
> leaving the header as is can result in uninitialized memory access by
> the header filter or the log module.  The fix is to clear partially
> initialized headers in case of errors.
>
> For the Cache-Control header, the fix is to postpone pushing
> r->headers_out.cache_control until its value is completed.
>
> diffstat:
>
>  src/http/modules/ngx_http_auth_basic_module.c     |   2 ++
>  src/http/modules/ngx_http_dav_module.c            |   1 +
>  src/http/modules/ngx_http_headers_filter_module.c |  21
> +++++++++++----------
>  src/http/modules/ngx_http_range_filter_module.c   |   4 ++++
>  src/http/modules/ngx_http_static_module.c         |   1 +
>  src/http/modules/perl/nginx.xs                    |   2 ++
>  src/http/ngx_http_core_module.c                   |   1 +
>  src/http/ngx_http_upstream.c                      |  13 +++++++------
>  8 files changed, 29 insertions(+), 16 deletions(-)
>
> diffs (162 lines):
>
> diff -r 23ecffd5bcfe -r 0cdee26605f3 src/http/modules/ngx_http_
> auth_basic_module.c
> --- a/src/http/modules/ngx_http_auth_basic_module.c     Thu Apr 20
> 13:58:16 2017 +0300
> +++ b/src/http/modules/ngx_http_auth_basic_module.c     Thu Apr 20
> 18:26:37 2017 +0300
> @@ -361,6 +361,8 @@ ngx_http_auth_basic_set_realm(ngx_http_r
>
>      basic = ngx_pnalloc(r->pool, len);
>      if (basic == NULL) {
> +        r->headers_out.www_authenticate->hash = 0;
> +        r->headers_out.www_authenticate = NULL;
>          return NGX_HTTP_INTERNAL_SERVER_ERROR;
>      }
>
> diff -r 23ecffd5bcfe -r 0cdee26605f3 src/http/modules/ngx_http_dav_
> module.c
> --- a/src/http/modules/ngx_http_dav_module.c    Thu Apr 20 13:58:16 2017
> +0300
> +++ b/src/http/modules/ngx_http_dav_module.c    Thu Apr 20 18:26:37 2017
> +0300
> @@ -1080,6 +1080,7 @@ ngx_http_dav_location(ngx_http_request_t
>      } else {
>          location = ngx_pnalloc(r->pool, r->uri.len);
>          if (location == NULL) {
> +            ngx_http_clear_location(r);
>              return NGX_ERROR;
>          }
>
> diff -r 23ecffd5bcfe -r 0cdee26605f3 src/http/modules/ngx_http_
> headers_filter_module.c
> --- a/src/http/modules/ngx_http_headers_filter_module.c Thu Apr 20
> 13:58:16 2017 +0300
> +++ b/src/http/modules/ngx_http_headers_filter_module.c Thu Apr 20
> 18:26:37 2017 +0300
> @@ -271,11 +271,6 @@ ngx_http_set_expires(ngx_http_request_t
>              return NGX_ERROR;
>          }
>
> -        ccp = ngx_array_push(&r->headers_out.cache_control);
> -        if (ccp == NULL) {
> -            return NGX_ERROR;
> -        }
> -
>          cc = ngx_list_push(&r->headers_out.headers);
>          if (cc == NULL) {
>              return NGX_ERROR;
> @@ -283,6 +278,12 @@ ngx_http_set_expires(ngx_http_request_t
>
>          cc->hash = 1;
>          ngx_str_set(&cc->key, "Cache-Control");
> +
> +        ccp = ngx_array_push(&r->headers_out.cache_control);
> +        if (ccp == NULL) {
> +            return NGX_ERROR;
> +        }
> +
>          *ccp = cc;
>
>      } else {
> @@ -470,11 +471,6 @@ ngx_http_add_cache_control(ngx_http_requ
>          }
>      }
>
> -    ccp = ngx_array_push(&r->headers_out.cache_control);
> -    if (ccp == NULL) {
> -        return NGX_ERROR;
> -    }
> -
>      cc = ngx_list_push(&r->headers_out.headers);
>      if (cc == NULL) {
>          return NGX_ERROR;
> @@ -484,6 +480,11 @@ ngx_http_add_cache_control(ngx_http_requ
>      ngx_str_set(&cc->key, "Cache-Control");
>      cc->value = *value;
>
> +    ccp = ngx_array_push(&r->headers_out.cache_control);
> +    if (ccp == NULL) {
> +        return NGX_ERROR;
> +    }
> +
>      *ccp = cc;
>
>      return NGX_OK;
> diff -r 23ecffd5bcfe -r 0cdee26605f3 src/http/modules/ngx_http_
> range_filter_module.c
> --- a/src/http/modules/ngx_http_range_filter_module.c   Thu Apr 20
> 13:58:16 2017 +0300
> +++ b/src/http/modules/ngx_http_range_filter_module.c   Thu Apr 20
> 18:26:37 2017 +0300
> @@ -425,6 +425,8 @@ ngx_http_range_singlepart_header(ngx_htt
>      content_range->value.data = ngx_pnalloc(r->pool,
>                                      sizeof("bytes -/") - 1 + 3 *
> NGX_OFF_T_LEN);
>      if (content_range->value.data == NULL) {
> +        content_range->hash = 0;
> +        r->headers_out.content_range = NULL;
>          return NGX_ERROR;
>      }
>
> @@ -594,6 +596,8 @@ ngx_http_range_not_satisfiable(ngx_http_
>      content_range->value.data = ngx_pnalloc(r->pool,
>                                         sizeof("bytes */") - 1 +
> NGX_OFF_T_LEN);
>      if (content_range->value.data == NULL) {
> +        content_range->hash = 0;
> +        r->headers_out.content_range = NULL;
>          return NGX_ERROR;
>      }
>
> diff -r 23ecffd5bcfe -r 0cdee26605f3 src/http/modules/ngx_http_
> static_module.c
> --- a/src/http/modules/ngx_http_static_module.c Thu Apr 20 13:58:16 2017
> +0300
> +++ b/src/http/modules/ngx_http_static_module.c Thu Apr 20 18:26:37 2017
> +0300
> @@ -169,6 +169,7 @@ ngx_http_static_handler(ngx_http_request
>
>              location = ngx_pnalloc(r->pool, len);
>              if (location == NULL) {
> +                ngx_http_clear_location(r);
>                  return NGX_HTTP_INTERNAL_SERVER_ERROR;
>              }
>
> diff -r 23ecffd5bcfe -r 0cdee26605f3 src/http/modules/perl/nginx.xs
> --- a/src/http/modules/perl/nginx.xs    Thu Apr 20 13:58:16 2017 +0300
> +++ b/src/http/modules/perl/nginx.xs    Thu Apr 20 18:26:37 2017 +0300
> @@ -510,10 +510,12 @@ header_out(r, key, value)
>      header->hash = 1;
>
>      if (ngx_http_perl_sv2str(aTHX_ r, &header->key, key) != NGX_OK) {
> +        header->hash = 0;
>          XSRETURN_EMPTY;
>      }
>
>      if (ngx_http_perl_sv2str(aTHX_ r, &header->value, value) != NGX_OK) {
> +        header->hash = 0;
>          XSRETURN_EMPTY;
>      }
>
> diff -r 23ecffd5bcfe -r 0cdee26605f3 src/http/ngx_http_core_module.c
> --- a/src/http/ngx_http_core_module.c   Thu Apr 20 13:58:16 2017 +0300
> +++ b/src/http/ngx_http_core_module.c   Thu Apr 20 18:26:37 2017 +0300
> @@ -1002,6 +1002,7 @@ ngx_http_core_find_config_phase(ngx_http
>              p = ngx_pnalloc(r->pool, len);
>
>              if (p == NULL) {
> +                ngx_http_clear_location(r);
>                  ngx_http_finalize_request(r, NGX_HTTP_INTERNAL_SERVER_
> ERROR);
>                  return NGX_OK;
>              }
> diff -r 23ecffd5bcfe -r 0cdee26605f3 src/http/ngx_http_upstream.c
> --- a/src/http/ngx_http_upstream.c      Thu Apr 20 13:58:16 2017 +0300
> +++ b/src/http/ngx_http_upstream.c      Thu Apr 20 18:26:37 2017 +0300
> @@ -4897,17 +4897,18 @@ ngx_http_upstream_copy_multi_header_line
>          }
>      }
>
> +    ho = ngx_list_push(&r->headers_out.headers);
> +    if (ho == NULL) {
> +        return NGX_ERROR;
> +    }
> +
> +    *ho = *h;
> +
>      ph = ngx_array_push(pa);
>      if (ph == NULL) {
>          return NGX_ERROR;
>      }
>
> -    ho = ngx_list_push(&r->headers_out.headers);
> -    if (ho == NULL) {
> -        return NGX_ERROR;
> -    }
> -
> -    *ho = *h;
>      *ph = ho;
>
>      return NGX_OK;
> _______________________________________________
> nginx-devel mailing list
> nginx-devel at nginx.org
> http://mailman.nginx.org/mailman/listinfo/nginx-devel
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mailman.nginx.org/pipermail/nginx-devel/attachments/20170424/8d945646/attachment.html>


More information about the nginx-devel mailing list