[PATCH] deflate support for gunzip module

[Maxim Dounin]

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