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@nginx.com>:

> details: http://hg.nginx.org/nginx/rev/0cdee26605f3
> branches:
> changeset: 6986:0cdee26605f3
> user: Sergey Kandaurov <pluknet@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@nginx.org
> http://mailman.nginx.org/mailman/listinfo/nginx-devel
>
_______________________________________________
nginx-devel mailing list
nginx-devel@nginx.org
http://mailman.nginx.org/mailman/listinfo/nginx-devel