Client disconnect in ngx_http_image_filter_module

[Dan Podeanu]

Thank you for the feedback- http://trac.nginx.org/nginx/ticket/756 <http://trac.nginx.org/nginx/ticket/756>

Regarding the fix, I do not believe bytes are silently skipped- if you look further below in src/http/modules/ngx_http_image_filter_module.c:

switch (ctx->phase) {
	case NGX_HTTP_IMAGE_START:
		ctx->type = ngx_http_image_test(r, in);
		….
		if (ctx->type == NGX_HTTP_IMAGE_NONE) {
			….
			/* wait to consume more data, do not give up on the request right away */
			return NGX_OK; /* as in the patch */
		}
		….
		/* fall-through */
	case NGX_HTTP_IMAGE_READ:
		…
		/* fall-through */
	case NGX_HTTP_IMAGE_PROCESS:
		…
}

i.e. that section of code is, correctly, designed to maintain state and be called repeatedly for the same image. In any case, we are no longer able to replicate the bug once we have applied the patch.


I think a better fix would be to "return NGX_OK" if we do not have enough data in “case NGX_HTTP_IMAGE_START", and "return NGX_HTTP_UNSUPPORTED_MEDIA_TYPE" (as per the original code) if enough data has been read, but it’s really not an image- but this exceeds the scope of the fix and my use case.

Thoughts ?

Thank you,
Dan

> On Apr 29, 2015, at 2:32 AM, Maxim Dounin <mdounin at mdounin.ru> wrote:
> 
> Hello!
> 
> On Tue, Apr 28, 2015 at 04:56:54PM +0700, Dan Podeanu wrote:
> 
>> 
>> I have encountered a bug in ngx_http_image_filter_module when used in conjunction with ngx_http_proxy_module ; the configuration is as following:
>> 
>> location /img/ {
>> 	proxy_pass http://mybucket.s3.amazonaws.com <http://mybucket.s3.amazonaws.com/>;
>> 	image_filter resize 150 100;
>> }
>> 
>> The steps to reproduce are rather complicated as they depend on how TCP fragments the response coming from the proxy:
>> 
>> - If http://mybucket.s3.amazonaws.com <http://mybucket.s3.amazonaws.com/> returns, in the first TCP packet, a sizable amount of data (1-2k), the image is resized correctly.
>> 
>> - If http://mybucket.s3.amazonaws.com <http://mybucket.s3.amazonaws.com/> returns, in the first TCP packet, a small amount of data (HTTP header, or HTTP header + a few bytes), the content is marked as not an image and NGX_HTTP_UNSUPPORTED_MEDIA_TYPE is returned (disconnecting the client), irrespective on whether or not subsequent data would complete the response to a valid image.
>> 
>> Nginx appears to give up right away on waiting for data if the contents of the first TCP packet received from the proxy does not contain a valid image header- i.e. ngx_http_image_test() will return NGX_HTTP_IMAGE_SIZE, etc.
>> 
>> In my experience this was triggered by a subtle change in AWS S3 that fragments the TCP header more.
>> 
>> Versions affected: 1.6.2, 1.6.3, 1.7.2, 1.8.0, etc.
>> 
>> 
>> 
>> Attaching a patch which fixes it for my use case (1.6.2) below; the other versions can be fixed similarly:
>> 
>> ————————————————————————
>> diff -ruN nginx-1.6.2.orig/src/http/modules/ngx_http_image_filter_module.c nginx-1.6.2/src/http/modules/ngx_http_image_filter_module.c
>> --- nginx-1.6.2.orig/src/http/modules/ngx_http_image_filter_module.c	2014-09-16 19:23:19.000000000 +0700
>> +++ nginx-1.6.2/src/http/modules/ngx_http_image_filter_module.c	2015-04-28 16:25:47.000000000 +0700
>> @@ -317,9 +317,8 @@
>>                 }
>>             }
>> 
>> -            return ngx_http_filter_finalize_request(r,
>> -                                              &ngx_http_image_filter_module,
>> -                                              NGX_HTTP_UNSUPPORTED_MEDIA_TYPE);
>> +            /* wait to consume more data, do not give up on the request right away */
>> +            return NGX_OK;
>>         }
>> 
>>         /* override content type */
>> ————————————————————————
>> 
>> 
>> Thoughts ? Is this the right fix ? Should I also post on the nginx Trac ?
> 
> Thanks, looks like a valid problem - and yes, please file a ticket 
> on trac.  The fix doesn't looks correct for me though - with such 
> a change nginx will silently skip first bytes of a response.
> 
> -- 
> Maxim Dounin
> http://nginx.org/
> 
> _______________________________________________
> nginx-devel mailing list
> nginx-devel at nginx.org
> http://mailman.nginx.org/mailman/listinfo/nginx-devel

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mailman.nginx.org/pipermail/nginx-devel/attachments/20150429/e7105b4f/attachment-0001.html>