[PATCH] Fixing the "redis sent invalid trailer" error in ngx_http_redis 0.3.5

[agentzh]

Hi, Sergey!

Thank you for creating the ngx_http_redis module in the first place.
I'm now looking into combining ngx_srcache and this redis module (as
well as my ngx_redis2) to form a transparent redis-backed caching
system.

I've just run a simple test of your ngx_http_redis module (version
0.3.5) with our etcproxy in our Nginx development toolchain:

    https://github.com/chaoslawful/etcproxy

And it makes your module throws out the following error:

    [error] 28417\#0: *1 redis sent invalid trailer while reading upstream

The etcproxy tool intercepted and splitted the TCP packets into many
small bits (by default, one byte per packet) and it has helped us find
a lot of subtle bugs in our nginx modules as well as libdrizzle.

After a little debugging, I've found the offending lines around
ngx_http_redis_module.c:566:

        if (ngx_strncmp(b->last,
                   ngx_http_redis_end + NGX_HTTP_REDIS_END - ctx->rest,
                   ctx->rest)
            != 0)
        {
            ngx_log_error(NGX_LOG_ERR, ctx->request->connection->log, 0,
                          "redis sent invalid trailer");

Here I think the 3rd argument to ngx_strncmp should be really "bytes"
instead of "ctx->rest". Because while enabling etcproxy, nginx will
receive only one byte at a time from the redis server, hence bytes ==
1 and ctx->rest == 2 here. So reading overflow can happen here. The
attached patch fixed this and helped your module passed my simple test
with etcproxy.

Hopefully you can merge my patch to the mainstream version :)

Thanks!
-agentzh

P.S. I've cc'd the openresty mailing group so that other people can
see our discussion here too (if you don't mind) :)

--- ngx_http_redis-0.3.5/ngx_http_redis_module.c 2012-03-21
15:38:33.022237313 +0800
+++ ngx_http_redis-0.3.5-patched/ngx_http_redis_module.c 2012-03-21
15:38:54.984205184 +0800
@@ -565,7 +565,7 @@

if (ngx_strncmp(b->last,
ngx_http_redis_end + NGX_HTTP_REDIS_END - ctx->rest,
- ctx->rest)
+ bytes)
!= 0)
{
ngx_log_error(NGX_LOG_ERR, ctx->request->connection->log, 0,

[Sergey A. Osokin]

Hi agentzh,

sorry for delay. Today I released ngx_http_redis-0.3.6.

<ChangeLog>

*) Feature: redis_gzip_flag. Usefull if you are prefer to
store data compressed in redis. Works with ngx_http_gunzip_filter
module.
Thanks to Maxim Dounin.

*) Bugfix: ngx_http_redis_module might issue the error message
"redis sent invalid trailer".
Thanks to agentzh.

</ChangeLog>

Thank you for report!

On Wed, Mar 21, 2012 at 03:45:13PM +0800, agentzh wrote:
> Hi, Sergey!
>
> Thank you for creating the ngx_http_redis module in the first place.
> I'm now looking into combining ngx_srcache and this redis module (as
> well as my ngx_redis2) to form a transparent redis-backed caching
> system.
>
> I've just run a simple test of your ngx_http_redis module (version
> 0.3.5) with our etcproxy in our Nginx development toolchain:
>

> О©╫О©╫О©╫ https://github.com/chaoslawful/etcproxy

>
> And it makes your module throws out the following error:
>

> О©╫О©╫О©╫ [error] 28417\#0: *1 redis sent invalid trailer while reading upstream

>
> The etcproxy tool intercepted and splitted the TCP packets into many
> small bits (by default, one byte per packet) and it has helped us find
> a lot of subtle bugs in our nginx modules as well as libdrizzle.
>
> After a little debugging, I've found the offending lines around
> ngx_http_redis_module.c:566:
>

> О©╫О©╫О©╫О©╫О©╫О©╫О©╫ if (ngx_strncmp(b->last,
> О©╫О©╫О©╫О©╫О©╫О©╫О©╫О©╫О©╫О©╫О©╫О©╫О©╫О©╫О©╫О©╫О©╫О©╫ ngx_http_redis_end + NGX_HTTP_REDIS_END - ctx->rest,
> О©╫О©╫О©╫О©╫О©╫О©╫О©╫О©╫О©╫О©╫О©╫О©╫О©╫О©╫О©╫О©╫О©╫О©╫ ctx->rest)
> О©╫О©╫О©╫О©╫О©╫О©╫О©╫О©╫О©╫О©╫О©╫ != 0)
> О©╫О©╫О©╫О©╫О©╫О©╫О©╫ {
> О©╫О©╫О©╫О©╫О©╫О©╫О©╫О©╫О©╫О©╫О©╫ ngx_log_error(NGX_LOG_ERR, ctx->request->connection->log, 0,
> О©╫О©╫О©╫О©╫О©╫О©╫О©╫О©╫О©╫О©╫О©╫О©╫О©╫О©╫О©╫О©╫О©╫О©╫О©╫О©╫О©╫О©╫О©╫О©╫О©╫ "redis sent invalid trailer");

>
> Here I think the 3rd argument to ngx_strncmp should be really "bytes"
> instead of "ctx->rest". Because while enabling etcproxy, nginx will
> receive only one byte at a time from the redis server, hence bytes ==
> 1 and ctx->rest == 2 here. So reading overflow can happen here. The
> attached patch fixed this and helped your module passed my simple test
> with etcproxy.
>
> Hopefully you can merge my patch to the mainstream version :)
>
> Thanks!
> -agentzh
>
> P.S. I've cc'd the openresty mailing group so that other people can
> see our discussion here too (if you don't mind) :)
>
> --- ngx_http_redis-0.3.5/ngx_http_redis_module.c 2012-03-21
> 15:38:33.022237313 +0800
> +++ ngx_http_redis-0.3.5-patched/ngx_http_redis_module.c 2012-03-21
> 15:38:54.984205184 +0800
> @@ -565,7 +565,7 @@
>
> if (ngx_strncmp(b->last,
> ngx_http_redis_end + NGX_HTTP_REDIS_END - ctx->rest,
> - ctx->rest)
> + bytes)
> != 0)
> {
> ngx_log_error(NGX_LOG_ERR, ctx->request->connection->log, 0,

--
Sergey A. Osokin
o...@FreeBSD.org