filesystem entries that are neither 'file' nor 'dir' can result in double ngx_close_file() if processed as FLV or MP4

[Maxim Dounin]

Hello!

On Thu, Dec 10, 2020 at 08:24:16PM +0300, Maxim Dounin wrote:

> Hello!
> 
> On Thu, Dec 10, 2020 at 04:04:44PM +0000, Chris Newton wrote:
> 
> > It has been noticed that when 'of' as returned by ngx_open_cached_file() is
> > not is_file, but otherwise valid and also not is_dir, then both the
> > ngx_http_flv_handler() and ngx_http_mp4_handler() functions will call
> > ngx_close_file() immediately. However, the ngx_pool_cleanup_file() will
> > still be called, leading to a duplicate ngx_close_file() being performed.
> > 
> > It seems that these calls to ngx_close_file() should just be removed; eg.,
> > with the following.
> 
> Thanks for the report.  This was an omission in the flv module 
> during introduction of open_file_cache in 1454:f497ed7682a7.  And 
> later it was copied to the mp4 module.
> 
> Indeed, removing the ngx_close_file() close call is the most 
> simple solution, and that's what 1454:f497ed7682a7 does in the 
> static module.
> 
> # HG changeset patch
> # User Maxim Dounin <mdounin at mdounin.ru>
> # Date 1607620894 -10800
> #      Thu Dec 10 20:21:34 2020 +0300
> # Node ID 09b25d66cf7e8fe1dc1c521867387ee828c7245e
> # Parent  2fec22332ff45b220b59e72266c5d0a622f21d15
> Fixed double close of non-regular files in flv and mp4.
> 
> With introduction of open_file_cache in 1454:f497ed7682a7, opening a file
> with ngx_open_cached_file() automatically adds a cleanup handler to close
> the file.  As such, calling ngx_close_file() directly for non-regular files
> is no longer needed and will result in duplicate close() call.
> 
> In 1454:f497ed7682a7 ngx_close_file() call for non-regular files was removed
> in the static module, but wasn't in the flv module.  And the resulting
> incorrect code was later copied to the mp4 module.  Fix is to remove the
> ngx_close_file() call from both modules.
> 
> Reported by Chris Newton.
> 
> diff --git a/src/http/modules/ngx_http_flv_module.c b/src/http/modules/ngx_http_flv_module.c
> --- a/src/http/modules/ngx_http_flv_module.c
> +++ b/src/http/modules/ngx_http_flv_module.c
> @@ -156,12 +156,6 @@ ngx_http_flv_handler(ngx_http_request_t 
>      }
>  
>      if (!of.is_file) {
> -
> -        if (ngx_close_file(of.fd) == NGX_FILE_ERROR) {
> -            ngx_log_error(NGX_LOG_ALERT, log, ngx_errno,
> -                          ngx_close_file_n " \"%s\" failed", path.data);
> -        }
> -
>          return NGX_DECLINED;
>      }
>  
> diff --git a/src/http/modules/ngx_http_mp4_module.c b/src/http/modules/ngx_http_mp4_module.c
> --- a/src/http/modules/ngx_http_mp4_module.c
> +++ b/src/http/modules/ngx_http_mp4_module.c
> @@ -521,12 +521,6 @@ ngx_http_mp4_handler(ngx_http_request_t 
>      }
>  
>      if (!of.is_file) {
> -
> -        if (ngx_close_file(of.fd) == NGX_FILE_ERROR) {
> -            ngx_log_error(NGX_LOG_ALERT, log, ngx_errno,
> -                          ngx_close_file_n " \"%s\" failed", path.data);
> -        }
> -
>          return NGX_DECLINED;
>      }
>  

Committed after an internal review:

http://hg.nginx.org/nginx/rev/7a55311b0dc3

Thanks.

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