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@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/
_______________________________________________
nginx-devel mailing list
nginx-devel@nginx.org
http://mailman.nginx.org/mailman/listinfo/nginx-devel