[PATCH]Upstream: delete temp file when error in ngx_http_upstream_store().

[Maxim Dounin]

Hello!

On Mon, Jul 07, 2014 at 05:19:45PM +0800, flygoast wrote:

> # HG changeset patch
> # User FengGu <flygoast at 126.com>
> # Date 1404723967 -28800
> #      Mon Jul 07 17:06:07 2014 +0800
> # Node ID d1d597fbf6d8e08059a308d8900e5f90def5377a
> # Parent  a680bf4dddd5c4b106419e3dfb0264815c401275
> Upstream: delete temp file when error in ngx_http_upstream_store().
> 
> 
> Previously, didn't process the case when ngx_http_map_uri_to_path() returned
> NULL. And when error occured, didn't delete the temp file.
> 
> 
> diff -r a680bf4dddd5 -r d1d597fbf6d8 src/http/ngx_http_upstream.c
> --- a/src/http/ngx_http_upstream.cSat Jul 05 23:29:47 2014 +0400
> +++ b/src/http/ngx_http_upstream.cMon Jul 07 17:06:07 2014 +0800
> @@ -3375,14 +3375,16 @@
>  
>      if (u->conf->store_lengths == NULL) {
>  
> -        ngx_http_map_uri_to_path(r, &path, &root, 0);
> +        if (ngx_http_map_uri_to_path(r, &path, &root, 0) == NULL) {
> +            goto error;
> +        }
>  
>      } else {
>          if (ngx_http_script_run(r, &path, u->conf->store_lengths->elts, 0,
>                                  u->conf->store_values->elts)
>              == NULL)
>          {
> -            return;
> +            goto error;
>          }
>      }
>  
> @@ -3393,6 +3395,16 @@
>                     tf->file.name.data, path.data);
>  
>      (void) ngx_ext_rename_file(&tf->file.name, &path, &ext);
> +
> +    return;
> +
> +error:
> +
> +    if (ngx_delete_file(tf->file.name.data) == NGX_FILE_ERROR) {
> +        ngx_log_error(NGX_LOG_CRIT, r->connection->log, ngx_errno,
> +                      ngx_delete_file_n " \"%s\" failed",
> +                      tf->file.name.data);
> +    }
>  }
>  

Thanks for noting this.  What do you think about the following patch 
instead?

There is a code in ngx_http_upstream_finalize_request() which 
normally removes temporary files after errors with proxy_store, 
but in this particular case it won't be triggered due to u->store 
set to 0 even in case of errors.  With this patch, u->store is set 
to 0 only in if there are no errors, and thus temporary files are 
properly removed in case of errors in ngx_http_upstream_store(). 

diff --git a/src/http/ngx_http_upstream.c b/src/http/ngx_http_upstream.c
--- a/src/http/ngx_http_upstream.c
+++ b/src/http/ngx_http_upstream.c
@@ -3253,7 +3253,6 @@ ngx_http_upstream_process_request(ngx_ht
                         || u->headers_in.content_length_n == tf->offset))
                 {
                     ngx_http_upstream_store(r, u);
-                    u->store = 0;
                 }
             }
         }
@@ -3375,7 +3374,9 @@ ngx_http_upstream_store(ngx_http_request
 
     if (u->conf->store_lengths == NULL) {
 
-        ngx_http_map_uri_to_path(r, &path, &root, 0);
+        if (ngx_http_map_uri_to_path(r, &path, &root, 0) == NULL) {
+            return;
+        }
 
     } else {
         if (ngx_http_script_run(r, &path, u->conf->store_lengths->elts, 0,
@@ -3392,7 +3393,11 @@ ngx_http_upstream_store(ngx_http_request
                    "upstream stores \"%s\" to \"%s\"",
                    tf->file.name.data, path.data);
 
-    (void) ngx_ext_rename_file(&tf->file.name, &path, &ext);
+    if (ngx_ext_rename_file(&tf->file.name, &path, &ext) != NGX_OK) {
+        return;
+    }
+
+    u->store = 0;
 }
 
 
-- 
Maxim Dounin
http://nginx.org/