[PATCH 2 of 3] Headers filter: add "add_trailer" directive
Maxim Dounin
mdounin at mdounin.ru
Fri Apr 21 17:42:07 UTC 2017
Hello!
On Mon, Apr 03, 2017 at 02:32:12AM -0700, Piotr Sikora via nginx-devel wrote:
> # HG changeset patch
> # User Piotr Sikora <piotrsikora at google.com>
> # Date 1490351854 25200
> # Fri Mar 24 03:37:34 2017 -0700
> # Node ID 5bab17ebe2b1f8ec42cf069bf484489c2a92c7a8
> # Parent 8af81a0d66c0f69bcf501edcf10deed4c8f7fbd4
> Headers filter: add "add_trailer" directive.
>
> Trailers added using this directive are evaluated after response body
> is processed by output filters (but before it's written to the wire),
> so it's possible to use variables calculated from the response body
> as the trailer value.
>
> Signed-off-by: Piotr Sikora <piotrsikora at google.com>
>
> diff -r 8af81a0d66c0 -r 5bab17ebe2b1 src/http/modules/ngx_http_chunked_filter_module.c
> --- a/src/http/modules/ngx_http_chunked_filter_module.c
> +++ b/src/http/modules/ngx_http_chunked_filter_module.c
> @@ -256,6 +256,10 @@ ngx_http_chunked_create_trailers(ngx_htt
> ngx_table_elt_t *header;
> ngx_http_chunked_filter_ctx_t *ctx;
>
> + if (ngx_http_eval_trailers(r) != NGX_OK) {
> + return NULL;
> + }
> +
> len = 0;
>
> part = &r->headers_out.trailers.part;
This introduces a layering violation between the headers filter
and the chunked filter.
> diff -r 8af81a0d66c0 -r 5bab17ebe2b1 src/http/modules/ngx_http_headers_filter_module.c
> --- a/src/http/modules/ngx_http_headers_filter_module.c
> +++ b/src/http/modules/ngx_http_headers_filter_module.c
> @@ -48,6 +48,7 @@ typedef struct {
> time_t expires_time;
> ngx_http_complex_value_t *expires_value;
> ngx_array_t *headers;
> + ngx_array_t *trailers;
> } ngx_http_headers_conf_t;
>
>
> @@ -72,6 +73,8 @@ static char *ngx_http_headers_expires(ng
> void *conf);
> static char *ngx_http_headers_add(ngx_conf_t *cf, ngx_command_t *cmd,
> void *conf);
> +static char *ngx_http_headers_add_trailer(ngx_conf_t *cf, ngx_command_t *cmd,
> + void *conf);
>
>
> static ngx_http_set_header_t ngx_http_set_headers[] = {
> @@ -108,6 +111,14 @@ static ngx_command_t ngx_http_headers_f
> 0,
> NULL},
>
> + { ngx_string("add_trailer"),
> + NGX_HTTP_MAIN_CONF|NGX_HTTP_SRV_CONF|NGX_HTTP_LOC_CONF|NGX_HTTP_LIF_CONF
> + |NGX_CONF_TAKE23,
> + ngx_http_headers_add_trailer,
> + NGX_HTTP_LOC_CONF_OFFSET,
> + 0,
> + NULL},
There should be a space between "NULL" and "}". It might worth
fixing the style in previously exiting directives in a separate
commit.
> +
> ngx_null_command
> };
>
> @@ -149,15 +160,24 @@ static ngx_http_output_header_filter_pt
> static ngx_int_t
> ngx_http_headers_filter(ngx_http_request_t *r)
> {
> - ngx_str_t value;
> - ngx_uint_t i, safe_status;
> - ngx_http_header_val_t *h;
> - ngx_http_headers_conf_t *conf;
> + u_char *p, *data;
> + size_t len;
> + ngx_str_t value;
> + ngx_uint_t i, safe_status;
> + ngx_table_elt_t *t;
> + ngx_http_header_val_t *h;
> + ngx_http_headers_conf_t *conf;
> + ngx_http_core_loc_conf_t *clcf;
> +
> + if (r != r->main) {
> + return ngx_http_next_header_filter(r);
> + }
>
> conf = ngx_http_get_module_loc_conf(r, ngx_http_headers_filter_module);
>
> - if ((conf->expires == NGX_HTTP_EXPIRES_OFF && conf->headers == NULL)
> - || r != r->main)
> + if (conf->expires == NGX_HTTP_EXPIRES_OFF
> + && conf->headers == NULL
> + && conf->trailers == NULL)
> {
> return ngx_http_next_header_filter(r);
> }
> @@ -205,6 +225,84 @@ ngx_http_headers_filter(ngx_http_request
> }
> }
>
> + if (conf->trailers && r->allow_trailers) {
> +
> + if (r->http_version < NGX_HTTP_VERSION_20) {
> + if (r->header_only
> + || r->headers_out.status == NGX_HTTP_NOT_MODIFIED
> + || r->headers_out.status == NGX_HTTP_NO_CONTENT
> + || r->headers_out.status < NGX_HTTP_OK
> + || r->method == NGX_HTTP_HEAD)
> + {
> + return ngx_http_next_header_filter(r);
> + }
> +
> + clcf = ngx_http_get_module_loc_conf(r, ngx_http_core_module);
> +
> + if (!clcf->chunked_transfer_encoding) {
> + return ngx_http_next_header_filter(r);
> + }
> + }
Predicting conditions which will result in header-only response
looks really fragile. As well as checks for chunked transfer
encoding.
See also comments to the previous patch about the difference
between HTTP/2 and HTTP/1.1 here.
> +
> + len = 0;
> +
> + h = conf->trailers->elts;
> + for (i = 0; i < conf->trailers->nelts; i++) {
> +
> + if (!safe_status && !h[i].always) {
> + continue;
> + }
> +
> + if (h[i].value.value.len) {
> + len += h[i].key.len + sizeof(", ") - 1;
> + }
> + }
> +
> + if (len == 0) {
> + return ngx_http_next_header_filter(r);
> + }
> +
> + len -= sizeof(", ") - 1;
> +
> + t = ngx_list_push(&r->headers_out.headers);
> + if (t == NULL) {
> + return NGX_ERROR;
> + }
> +
> + data = ngx_pnalloc(r->pool, len);
> + if (data == NULL) {
> + return NGX_ERROR;
> + }
Note: this can result in an access to uninitialized memory if
ngx_pnalloc() fails and $sent_http_trailer used in logs.
> +
> + p = data;
> +
> + h = conf->trailers->elts;
> + for (i = 0; i < conf->trailers->nelts; i++) {
> +
> + if (!safe_status && !h[i].always) {
> + continue;
> + }
> +
> + if (h[i].value.value.len) {
> + p = ngx_copy(p, h[i].key.data, h[i].key.len);
> +
> + if (p == data + len) {
> + break;
> + }
> +
> + *p++ = ','; *p++ = ' ';
> + }
> + }
Note that at this point we do not know if the trailer in question
is going to be present in the particular response or not, as
trailers with empty values are not returned (and this is often
used to only return some headers in some responses, at least with
add_header). It might be a better idea to avoid generating the
"Trailer" header automatically and let users to control this with
add_header instead, and/or introduce an option to control this.
Note well that with the approach taken it will be very
inconvenient for other modules to add their own trailers, due to
both a) layering violation introduced, which makes headers filter
special, and b) no easy way to add anything to the Trailer header
generated.
> +
> + ngx_str_set(&t->key, "Trailer");
> + t->value.data = data;
> + t->value.len = len;
> + t->hash = ngx_hash(ngx_hash(ngx_hash(ngx_hash(ngx_hash(
> + ngx_hash('t', 'r'), 'a'), 'i'), 'l'), 'e'), 'r');
There is no need to set hash for output headers, just t->hash = 1
is good enough.
> +
> + r->expect_trailers = 1;
> + }
> +
> return ngx_http_next_header_filter(r);
> }
>
> @@ -541,6 +639,67 @@ ngx_http_set_response_header(ngx_http_re
> }
>
>
> +ngx_int_t
> +ngx_http_eval_trailers(ngx_http_request_t *r)
> +{
> + ngx_str_t value;
> + ngx_uint_t i, safe_status;
> + ngx_table_elt_t *t;
> + ngx_http_header_val_t *h;
> + ngx_http_headers_conf_t *conf;
> +
> + conf = ngx_http_get_module_loc_conf(r, ngx_http_headers_filter_module);
> +
> + if (conf->trailers == NULL) {
> + return NGX_OK;
> + }
> +
> + switch (r->headers_out.status) {
> +
> + case NGX_HTTP_OK:
> + case NGX_HTTP_CREATED:
> + case NGX_HTTP_NO_CONTENT:
> + case NGX_HTTP_PARTIAL_CONTENT:
> + case NGX_HTTP_MOVED_PERMANENTLY:
> + case NGX_HTTP_MOVED_TEMPORARILY:
> + case NGX_HTTP_SEE_OTHER:
> + case NGX_HTTP_NOT_MODIFIED:
> + case NGX_HTTP_TEMPORARY_REDIRECT:
> + safe_status = 1;
> + break;
> +
> + default:
> + safe_status = 0;
> + break;
> + }
> +
> + h = conf->trailers->elts;
> + for (i = 0; i < conf->trailers->nelts; i++) {
> +
> + if (!safe_status && !h[i].always) {
> + continue;
> + }
> +
> + if (ngx_http_complex_value(r, &h[i].value, &value) != NGX_OK) {
> + return NGX_ERROR;
> + }
> +
> + if (value.len) {
> + t = ngx_list_push(&r->headers_out.trailers);
> + if (t == NULL) {
> + return NGX_ERROR;
> + }
> +
> + t->key = h[i].key;
> + t->value = value;
> + t->hash = 1;
> + }
> + }
> +
> + return NGX_OK;
> +}
> +
> +
> static void *
> ngx_http_headers_create_conf(ngx_conf_t *cf)
> {
> @@ -555,6 +714,7 @@ ngx_http_headers_create_conf(ngx_conf_t
> * set by ngx_pcalloc():
> *
> * conf->headers = NULL;
> + * conf->trailers = NULL;
> * conf->expires_time = 0;
> * conf->expires_value = NULL;
> */
> @@ -585,6 +745,10 @@ ngx_http_headers_merge_conf(ngx_conf_t *
> conf->headers = prev->headers;
> }
>
> + if (conf->trailers == NULL) {
> + conf->trailers = prev->trailers;
> + }
> +
> return NGX_CONF_OK;
> }
>
> @@ -739,3 +903,63 @@ ngx_http_headers_add(ngx_conf_t *cf, ngx
>
> return NGX_CONF_OK;
> }
> +
> +
> +static char *
> +ngx_http_headers_add_trailer(ngx_conf_t *cf, ngx_command_t *cmd, void *conf)
> +{
> + ngx_http_headers_conf_t *hcf = conf;
> +
> + ngx_str_t *value;
> + ngx_http_header_val_t *hv;
> + ngx_http_compile_complex_value_t ccv;
> +
> + value = cf->args->elts;
> +
> + if (hcf->trailers == NULL) {
> + hcf->trailers = ngx_array_create(cf->pool, 1,
> + sizeof(ngx_http_header_val_t));
> + if (hcf->trailers == NULL) {
> + return NGX_CONF_ERROR;
> + }
> + }
> +
> + hv = ngx_array_push(hcf->trailers);
> + if (hv == NULL) {
> + return NGX_CONF_ERROR;
> + }
> +
> + hv->key = value[1];
> + hv->handler = NULL;
> + hv->offset = 0;
> + hv->always = 0;
> +
> + if (value[2].len == 0) {
> + ngx_memzero(&hv->value, sizeof(ngx_http_complex_value_t));
> +
> + } else {
> + ngx_memzero(&ccv, sizeof(ngx_http_compile_complex_value_t));
> +
> + ccv.cf = cf;
> + ccv.value = &value[2];
> + ccv.complex_value = &hv->value;
> +
> + if (ngx_http_compile_complex_value(&ccv) != NGX_OK) {
> + return NGX_CONF_ERROR;
> + }
> + }
> +
> + if (cf->args->nelts == 3) {
> + return NGX_CONF_OK;
> + }
> +
> + if (ngx_strcmp(value[3].data, "always") != 0) {
> + ngx_conf_log_error(NGX_LOG_EMERG, cf, 0,
> + "invalid parameter \"%V\"", &value[3]);
> + return NGX_CONF_ERROR;
> + }
> +
> + hv->always = 1;
> +
> + return NGX_CONF_OK;
> +}
> diff -r 8af81a0d66c0 -r 5bab17ebe2b1 src/http/ngx_http.h
> --- a/src/http/ngx_http.h
> +++ b/src/http/ngx_http.h
> @@ -143,6 +143,7 @@ ngx_int_t ngx_http_special_response_hand
> ngx_int_t ngx_http_filter_finalize_request(ngx_http_request_t *r,
> ngx_module_t *m, ngx_int_t error);
> void ngx_http_clean_header(ngx_http_request_t *r);
> +ngx_int_t ngx_http_eval_trailers(ngx_http_request_t *r);
>
>
> ngx_int_t ngx_http_discard_request_body(ngx_http_request_t *r);
> diff -r 8af81a0d66c0 -r 5bab17ebe2b1 src/http/v2/ngx_http_v2_filter_module.c
> --- a/src/http/v2/ngx_http_v2_filter_module.c
> +++ b/src/http/v2/ngx_http_v2_filter_module.c
> @@ -667,6 +667,10 @@ ngx_http_v2_create_trailers_frame(ngx_ht
> ngx_table_elt_t *header;
> ngx_http_v2_out_frame_t *frame;
>
> + if (ngx_http_eval_trailers(r) != NGX_OK) {
> + return NGX_HTTP_V2_FRAME_ERROR;
> + }
> +
> len = 0;
> tmp_len = 0;
>
> _______________________________________________
> nginx-devel mailing list
> nginx-devel at nginx.org
> http://mailman.nginx.org/mailman/listinfo/nginx-devel
--
Maxim Dounin
http://nginx.org/
More information about the nginx-devel
mailing list