[bugfix] Range filter: more appropriate restriction on max ranges.

[Maxim Dounin]

Hello!

On Fri, Nov 10, 2017 at 01:08:58AM +0800, 胡聪 (hucc) wrote:

> Hi,
> 
> On Thursday, Nov 9, 2017 10:18 PM +0300, Maxim Dounin wrote:
> 
> >On Fri, Oct 27, 2017 at 06:48:52PM +0800, 胡聪 (hucc) wrote:
> >
> >> # HG changeset patch
> >> # User hucongcong <hucong.c at foxmail.com>
> >> # Date 1509099660 -28800
> >> #      Fri Oct 27 18:21:00 2017 +0800
> >> # Node ID b9850d3deb277bd433a689712c40a84401443520
> >> # Parent  9ef704d8563af4aff6817ab1c694fb40591f20b3
> >> Range filter: more appropriate restriction on max ranges.
> >> 
> >> diff -r 9ef704d8563a -r b9850d3deb27 src/http/modules/ngx_http_range_filter_module.c
> >> --- a/src/http/modules/ngx_http_range_filter_module.c Tue Oct 17 19:52:16 2017 +0300
> >> +++ b/src/http/modules/ngx_http_range_filter_module.c Fri Oct 27 18:21:00 2017 +0800
> >> @@ -369,6 +369,11 @@ ngx_http_range_parse(ngx_http_request_t 
> >>      found:
> >>  
> >>          if (start < end) {
> >> +
> >> +            if (ranges-- == 0) {
> >> +                return NGX_DECLINED;
> >> +            }
> >> +
> >>              range = ngx_array_push(&ctx->ranges);
> >>              if (range == NULL) {
> >>                  return NGX_ERROR;
> >> @@ -383,10 +388,6 @@ ngx_http_range_parse(ngx_http_request_t 
> >>  
> >>              size += end - start;
> >>  
> >> -            if (ranges-- == 0) {
> >> -                return NGX_DECLINED;
> >> -            }
> >> -
> >>          } else if (start == 0) {
> >>              return NGX_DECLINED;
> >>          }
> >
> >There is no real difference, and the current code looks slightly 
> >more readable for me, so I would rather leave it as is.
> 
> We assume that max_ranges is configured as 1, CL is the length of
> representation and is equal to NGX_MAX_OFF_T_VALUE. Based on this,
> 416 will be returned if the byte-range-set is '0-9, 7-CL';
> and 200 will be returned if the byte-range-set is '0-9, 7-100'.
> This is the difference, although the situation is rare, but it exists. 
> 
> I knew that the current code is slightly more readable. The problem
> here is consistency and predictability from user point of view, which
> is the rule I learned from
> http://mailman.nginx.org/pipermail/nginx-devel/2017-March/009687.html.
> Not to mention that correctness is more important than readability
> from server (nginx) point of view.

Both 200 and 416 are correct responses in the particular situation 
described: 416 is quite normal when a user tries to request more 
than NGX_MAX_OFF_T_VALUE bytes, and 200 is quite normal when a 
user tries to request more ranges than allowed by max_ranges.

Which status to use doesn't really matter, as in both cases no 
range processing will be done and both status codes are correct.  
In this particular case I would prefer 416 as currently returned, 
as it is more in line with what is returned when there is no 
max_ranges limit configured.

(It might be also a good idea to apply max_ranges limit only after 
parsing all ranges, so that "Range: bytes=0-0,1-1,foo" would 
consistently result in 416 regardless of max_ranges configured, 
but this does not seem to be important enough to care.)

-- 
Maxim Dounin
http://mdounin.ru/