Hello!

On Wed, May 11, 2022 at 07:32:32PM +0400, Sergey Kandaurov wrote:

> On Thu, Apr 21, 2022 at 01:18:42AM +0300, Maxim Dounin wrote:
> > # HG changeset patch
> > # User Maxim Dounin <mdounin@mdounin.ru>
> > # Date 1650492316 -10800
> > # Thu Apr 21 01:05:16 2022 +0300
> > # Node ID 61b29233a55216c6fa72e23b93a4a28d76a9fb94
> > # Parent e70fb0fdfbc0fb7b7e9f493cc2eb65de617b115a
> > FastCGI: combining headers with identical names (ticket #1724).
> >
> > FastCGI responder is expected to receive CGI/1.1 environment variables
> > in the parameters (see section "6.2 Responder" of the FastCGI specification).
> > Obviously enough, there cannot be multiple environment variables with
> > the same name.
> >
> > Further, CGI specification (RFC 3875, section "4.1.18. Protocol-Specific
> > Meta-Variables") explicitly requires to combine headers: "If multiple
> > header fields with the same field-name are received then the server MUST
> > rewrite them as a single value having the same semantics".
> >
> > diff --git a/src/core/ngx_hash.h b/src/core/ngx_hash.h
> > --- a/src/core/ngx_hash.h
> > +++ b/src/core/ngx_hash.h
> > @@ -89,12 +89,15 @@ typedef struct {
> > } ngx_hash_keys_arrays_t;
> >
> >
> > -typedef struct {
> > +typedef struct ngx_table_elt_s ngx_table_elt_t;
> > +
> > +struct ngx_table_elt_s {
> > ngx_uint_t hash;
> > ngx_str_t key;
> > ngx_str_t value;
> > u_char *lowcase_key;
> > -} ngx_table_elt_t;
> > + ngx_table_elt_t *next;
> > +};
> >
> >
> > void *ngx_hash_find(ngx_hash_t *hash, ngx_uint_t key, u_char *name, size_t len);
> > diff --git a/src/http/modules/ngx_http_fastcgi_module.c b/src/http/modules/ngx_http_fastcgi_module.c
> > --- a/src/http/modules/ngx_http_fastcgi_module.c
> > +++ b/src/http/modules/ngx_http_fastcgi_module.c
> > @@ -835,14 +835,14 @@ static ngx_int_t
> > ngx_http_fastcgi_create_request(ngx_http_request_t *r)
> > {
> > off_t file_pos;
> > - u_char ch, *pos, *lowcase_key;
> > + u_char ch, sep, *pos, *lowcase_key;
> > size_t size, len, key_len, val_len, padding,
> > allocated;
> > ngx_uint_t i, n, next, hash, skip_empty, header_params;
> > ngx_buf_t *b;
> > ngx_chain_t *cl, *body;
> > ngx_list_part_t *part;
> > - ngx_table_elt_t *header, **ignored;
> > + ngx_table_elt_t *header, *hn, **ignored;
> > ngx_http_upstream_t *u;
> > ngx_http_script_code_pt code;
> > ngx_http_script_engine_t e, le;
> > @@ -900,7 +900,11 @@ ngx_http_fastcgi_create_request(ngx_http
> > allocated = 0;
> > lowcase_key = NULL;
> >
> > - if (params->number) {
> > + if (ngx_http_link_multi_headers(r) != NGX_OK) {
> > + return NGX_ERROR;
> > + }
> > +
> > + if (params->number || r->headers_in.multi) {
> > n = 0;
> > part = &r->headers_in.headers.part;
> >
> > @@ -930,6 +934,12 @@ ngx_http_fastcgi_create_request(ngx_http
> > i = 0;
> > }
> >
> > + for (n = 0; n < header_params; n++) {
> > + if (&header[i] == ignored[n]) {
> > + goto next_length;
> > + }
> > + }
> > +
> > if (params->number) {
> > if (allocated < header[i].key.len) {
> > allocated = header[i].key.len + 16;
> > @@ -959,15 +969,23 @@ ngx_http_fastcgi_create_request(ngx_http
> > ignored[header_params++] = &header[i];
> > continue;
> > }
> > -
> > - n += sizeof("HTTP_") - 1;
> > -
> > - } else {
> > - n = sizeof("HTTP_") - 1 + header[i].key.len;
> > }
> >
> > - len += ((n > 127) ? 4 : 1) + ((header[i].value.len > 127) ? 4 : 1)
> > - + n + header[i].value.len;
> > + key_len = sizeof("HTTP_") - 1 + header[i].key.len;
> > +
> > + val_len = header[i].value.len;
> > +
> > + for (hn = header[i].next; hn; hn = hn->next) {
> > + val_len += hn->value.len + 2;
> > + ignored[header_params++] = hn;
> > + }
> > +
> > + len += ((key_len > 127) ? 4 : 1) + key_len
> > + + ((val_len > 127) ? 4 : 1) + val_len;
> > +
> > + next_length:
> > +
> > + continue;
> > }
> > }
> >
> > @@ -1109,7 +1127,7 @@ ngx_http_fastcgi_create_request(ngx_http
> >
> > for (n = 0; n < header_params; n++) {
> > if (&header[i] == ignored[n]) {
> > - goto next;
> > + goto next_value;
> > }
> > }
> >
> > @@ -1125,6 +1143,11 @@ ngx_http_fastcgi_create_request(ngx_http
> > }
> >
> > val_len = header[i].value.len;
> > +
> > + for (hn = header[i].next; hn; hn = hn->next) {
> > + val_len += hn->value.len + 2;
> > + }
> > +
> > if (val_len > 127) {
> > *b->last++ = (u_char) (((val_len >> 24) & 0x7f) | 0x80);
> > *b->last++ = (u_char) ((val_len >> 16) & 0xff);
> > @@ -1150,13 +1173,34 @@ ngx_http_fastcgi_create_request(ngx_http
> > *b->last++ = ch;
> > }
> >
> > - b->last = ngx_copy(b->last, header[i].value.data, val_len);
> > + b->last = ngx_copy(b->last, header[i].value.data,
> > + header[i].value.len);
> > +
> > + if (header[i].next) {
> > +
> > + if (header[i].key.len == sizeof("Cookie") - 1
> > + && ngx_strncasecmp(header[i].key.data, (u_char *) "Cookie",
> > + sizeof("Cookie") - 1)
> > + == 0)
> > + {
> > + sep = ';';
> > +
> > + } else {
> > + sep = ',';
> > + }
> > +
> > + for (hn = header[i].next; hn; hn = hn->next) {
> > + *b->last++ = sep;
> > + *b->last++ = ' ';
> > + b->last = ngx_copy(b->last, hn->value.data, hn->value.len);
> > + }
> > + }
> >
> > ngx_log_debug4(NGX_LOG_DEBUG_HTTP, r->connection->log, 0,
> > "fastcgi param: \"%*s: %*s\"",
> > key_len, b->last - (key_len + val_len),
> > val_len, b->last - val_len);
> > - next:
> > + next_value:
> >
> > continue;
> > }
>
> Overall, fastcgi/uwsgi/scgi parts look good, some observations below:
>
> - fastcgi_param (still) overrides client headers with the same name
> e.g., "fastcgi_param header value;" overrides client header "header: value"

That's how it's expected to work (with HTTP_<HEADER> fastcgi
param).

> - multiple directives itself aren't linked emitting separate params:
> fastcgi_param FOO BAR;
> fastcgi_param FOO BAZ;
>
> I don't think it's a major point though, it can be handled
> in configuration in principle.

I don't think this needs to be changed, or at least I don't think
this needs to be addressed in this patch series. If we'll decide
to address this somehow, rejecting such configurations might be a
way to go.

> BTW, what about proxy modifications? they aren't required but could be useful

I don't think that's needed. Unless required, I would rather
preserve original request headers.

> > diff --git a/src/http/ngx_http_core_module.c b/src/http/ngx_http_core_module.c
> > --- a/src/http/ngx_http_core_module.c
> > +++ b/src/http/ngx_http_core_module.c
> > @@ -2802,6 +2802,78 @@ ngx_http_get_forwarded_addr_internal(ngx
> > }
> >
> >
> > +ngx_int_t
> > +ngx_http_link_multi_headers(ngx_http_request_t *r)
> > +{
> > + ngx_uint_t i, j;
> > + ngx_list_part_t *part, *ppart;
> > + ngx_table_elt_t *header, *pheader, **ph;
> > +
> > + if (r->headers_in.multi_linked) {
> > + return NGX_OK;
> > + }
>
> multi_linked is never set, was the intension to avoid doing the work
> twice on repetitive invocation? it doesn't seem to be possible.
> I'd just axe it.

Yes, thanks, missed this somehow. It should be set right after
the test, updated with the following change:

diff --git a/src/http/ngx_http_core_module.c b/src/http/ngx_http_core_module.c
--- a/src/http/ngx_http_core_module.c
+++ b/src/http/ngx_http_core_module.c
@@ -2813,6 +2813,8 @@ ngx_http_link_multi_headers(ngx_http_req
return NGX_OK;
}

+ r->headers_in.multi_linked = 1;
+
part = &r->headers_in.headers.part;
header = part->elts;


Not sure what do you mean by "doesn't seem to be possible". The
request headers are never changed after parsing, so it is enough
to link them just once.

> > +
> > + part = &r->headers_in.headers.part;
> > + header = part->elts;
> > +
> > + for (i = 0; /* void */; i++) {
> > +
> > + if (i >= part->nelts) {
> > + if (part->next == NULL) {
> > + break;
> > + }
> > +
> > + part = part->next;
> > + header = part->elts;
> > + i = 0;
> > + }
> > +
> > + header[i].next = NULL;
> > +
> > + /*
> > + * search for previous headers with the same name;
> > + * if there are any, link to them
> > + */
> > +
> > + ppart = &r->headers_in.headers.part;
> > + pheader = part->elts;
>
> pheader = ppart->elts;

Fixed, thanks.

> > +
> > + for (j = 0; /* void */; j++) {
> > +
> > + if (j >= ppart->nelts) {
> > + if (ppart->next == NULL) {
> > + break;
> > + }
> > +
> > + ppart = ppart->next;
> > + pheader = ppart->elts;
> > + i = 0;
>
> j = 0;

Fixed, thanks.

> > + }
> > +
> > + if (part == ppart && i == j) {
> > + break;
> > + }
> > +
> > + if (header[i].key.len == pheader[j].key.len
> > + && ngx_strncasecmp(header[i].key.data, pheader[j].key.data,
> > + header[i].key.len)
> > + == 0)
> > + {
> > + ph = &pheader[j].next;
> > + while (*ph) { ph = &(*ph)->next; }
> > + *ph = &header[i];
> > +
> > + r->headers_in.multi = 1;
> > +
> > + break;
> > + }
> > + }
> > + }
> > +
> > + return NGX_OK;
> > +}
> > +
> > +
> > static char *
> > ngx_http_core_server(ngx_conf_t *cf, ngx_command_t *cmd, void *dummy)
> > {
> > diff --git a/src/http/ngx_http_core_module.h b/src/http/ngx_http_core_module.h
> > --- a/src/http/ngx_http_core_module.h
> > +++ b/src/http/ngx_http_core_module.h
> > @@ -532,6 +532,8 @@ ngx_int_t ngx_http_get_forwarded_addr(ng
> > ngx_array_t *headers, ngx_str_t *value, ngx_array_t *proxies,
> > int recursive);
> >
> > +ngx_int_t ngx_http_link_multi_headers(ngx_http_request_t *r);
> > +
> >
> > extern ngx_module_t ngx_http_core_module;
> >
> > diff --git a/src/http/ngx_http_request.h b/src/http/ngx_http_request.h
> > --- a/src/http/ngx_http_request.h
> > +++ b/src/http/ngx_http_request.h
> > @@ -242,6 +242,8 @@ typedef struct {
> >
> > unsigned connection_type:2;
> > unsigned chunked:1;
> > + unsigned multi:1;
> > + unsigned multi_linked:1;
> > unsigned msie:1;
> > unsigned msie6:1;
> > unsigned opera:1;
> >
> > _______________________________________________
> > nginx-devel mailing list -- nginx-devel@nginx.org
> > To unsubscribe send an email to nginx-devel-leave@nginx.org
> _______________________________________________
> nginx-devel mailing list -- nginx-devel@nginx.org
> To unsubscribe send an email to nginx-devel-leave@nginx.org

--
Maxim Dounin
http://mdounin.ru/
_______________________________________________
nginx-devel mailing list -- nginx-devel@nginx.org
To unsubscribe send an email to nginx-devel-leave@nginx.org