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 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;
     }
 

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