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
osa@FreeBSD.org
