Hello!

On Fri, Feb 25, 2011 at 12:11:26PM +0600, Serge Sitnikov wrote:

> Adds deflate decompression support for gunzip module:

Short answer:

No.

Long answer:

There are multiple issues with the patch itself, but more
importantly - there is a big issue with deflate itself. See below
for more details.

>
> --- ngx_http_gunzip_filter_module.c
> +++ ngx_http_gunzip_filter_module.c 2011-02-24 09:07:08.000000000 +0100
> @@ -30,6 +30,8 @@
> ngx_buf_t *out_buf;
> ngx_int_t bufs;
>
> + unsigned deflate:1;
> +
> unsigned started:1;
> unsigned flush:4;
> unsigned redo:1;
> @@ -122,42 +124,45 @@
> {
> ngx_http_gunzip_ctx_t *ctx;
> ngx_http_gunzip_conf_t *conf;
> + int deflate = 0, client_deflate = 0;
>
> conf = ngx_http_get_module_loc_conf(r, ngx_http_gunzip_filter_module);
>
> - /* TODO support multiple content-codings */
> + /* TODO support even more encodings */

This comment was about correctly handling multiple content-codings
specified in Content-Encoding header, not about supporting more
encodings.

> /* TODO always gunzip - due to configuration or module request */
> /* TODO ignore content encoding? */
>
> - if (!conf->enable
> - || r->headers_out.content_encoding == NULL
> - || r->headers_out.content_encoding->value.len != 4
> - || ngx_strncasecmp(r->headers_out.content_encoding->value.data,
> - (u_char *) "gzip", 4) != 0)
> - {
> + if (!conf->enable || r->headers_out.content_encoding == NULL) {
> return ngx_http_next_header_filter(r);
> + } else if (r->headers_out.content_encoding->value.len != 4 ||
> + ngx_strncasecmp(r->headers_out.content_encoding->value.data,
> (u_char *) "gzip", 4) != 0)
> + {
> + deflate = r->headers_out.content_encoding->value.len == 7 &&
> + ngx_strncasecmp(r->headers_out.content_encoding->value.data,
> (u_char *) "deflate", 7) == 0;
> +
> + if (!deflate) return ngx_http_next_header_filter(r);
> + else client_deflate = r->headers_in.accept_encoding != NULL ?
> + ngx_strcasestrn(r->headers_in.accept_encoding->value.data,
> "deflate", 7 - 1) != NULL : 0;

Checking for accept encoding to see if encoding may be actually
sent to client isn't really enough. Gzip has lots of options for
reason.

> }
>
> #if (nginx_version >= 8025 || (nginx_version >= 7065 && nginx_version < 8000))
>
> r->gzip_vary = 1;
> + int ok = r->gzip_tested ? r->gzip_ok : ngx_http_gzip_ok(r) == NGX_OK;
>
> - if (!r->gzip_tested) {
> - if (ngx_http_gzip_ok(r) == NGX_OK) {
> - return ngx_http_next_header_filter(r);
> - }
> +#else
>
> - } else if (!r->gzip_ok) {
> - return ngx_http_next_header_filter(r);
> - }
> + int ok = ngx_http_gzip_ok(r) == NGX_OK;
>
> -#else
> +#endif
>
> - if (ngx_http_gzip_ok(r) == NGX_OK) {
> - return ngx_http_next_header_filter(r);
> + if (deflate) {
> + if (ok && client_deflate) return ngx_http_next_header_filter(r);
> + } else {
> + if (ok) return ngx_http_next_header_filter(r);
> }
>
> -#endif
> + // --

Don't even talking about style.

>
> ctx = ngx_pcalloc(r->pool, sizeof(ngx_http_gunzip_ctx_t));
> if (ctx == NULL) {
> @@ -167,6 +172,7 @@
> ngx_http_set_ctx(r, ctx, ngx_http_gunzip_filter_module);
>
> ctx->request = r;
> + ctx->deflate = deflate;
>
> r->filter_need_in_memory = 1;
>
> @@ -314,8 +320,8 @@
> ctx->zstream.zfree = ngx_http_gunzip_filter_free;
> ctx->zstream.opaque = ctx;
>
> - /* windowBits +16 to decode gzip, zlib 1.2.0.4+ */
> - rc = inflateInit2(&ctx->zstream, MAX_WBITS + 16);
> + /* windowBits +32 to auto decode gzip and deflate */
> + rc = inflateInit2(&ctx->zstream, MAX_WBITS + 32);

1. Zlib version was specified on purpose.

2. This will allow gzip to be decoded with Content-Encoding set to
deflate and vice versa, which is not a good thing.

>
> if (rc != Z_OK) {
> ngx_log_error(NGX_LOG_ALERT, r->connection->log, 0,
> @@ -328,6 +334,33 @@
> ctx->last_out = &ctx->out;
> ctx->flush = Z_NO_FLUSH;
>
> + // --
> +
> + if (!ctx->deflate) return NGX_OK;
> +
> + /* Header inflation should not produce any output, so all
> + these hacks below are valid. */
> +
> + static unsigned char deflate_header[] = { 0x78, 0x9C };
> + unsigned char temp;
> +
> + ctx->zstream.next_in = deflate_header;
> + ctx->zstream.avail_in = 2;
> + ctx->zstream.next_out = &temp;
> + ctx->zstream.avail_out = 1;

And, finally, here is key point why deflate isn't supported in
nginx at all and I don't plan to implement it in gunzip as well:

Correct response with Content-Encoding deflate should have zlib
stream in it, and an attempt to to add header indicate that you
are trying to parse incorrect responses. This in turn will
render correct responses unparseable.

Moreover, it's impossible to actually return correct deflate
responses in real world - as many browsers (notably IE) implement
deflate incorrectly. Parsing should be possible, but I believe
better aproach is to don't touch this piece at all as nginx did
for years.

More details may be found here:

http://trac.tools.ietf.org/wg/httpbis/trac/ticket/73
http://lists.w3.org/Archives/Public/ietf-http-wg/2010JanMar/thread.html#msg324

> +
> + rc = inflate(&ctx->zstream, Z_NO_FLUSH);
> + if (rc != Z_OK) {
> + ngx_log_error(NGX_LOG_ALERT, r->connection->log, 0,
> + "deflate: inflate() failed: %d, %d", ctx->flush, rc);
> + return NGX_ERROR;
> + }
> +
> + ctx->zstream.next_in = Z_NULL;
> + ctx->zstream.avail_in = 0;
> + ctx->zstream.next_out = Z_NULL;
> + ctx->zstream.avail_out = 0;
> +
> return NGX_OK;
> }

Maxim Dounin

_______________________________________________
nginx-devel mailing list
nginx-devel@nginx.org
http://nginx.org/mailman/listinfo/nginx-devel