Mailing-List: contact cvs-help@httpd.apache.org; run by ezmlm
Precedence: bulk
Reply-To: dev@httpd.apache.org
Date: 22 Sep 2004 22:28:54 -0000
Message-ID: <20040922222854.66397.qmail@minotaur.apache.org>
From: jerenkrantz@apache.org
To: httpd-2.0-cvs@apache.org
Subject: cvs commit: httpd-2.0/modules/experimental mod_disk_cache.c

jerenkrantz    2004/09/22 15:28:54

  Modified:    .        CHANGES
               modules/experimental mod_disk_cache.c
  Log:
  Fix race conditions in mod_disk_cache by properly using the tempfile rather
  than the data file.  (We rename the tempfile when we're completed with the data
  file which is an atomic operation.)
  
  Part of the code assumed that it was using a temporary file; other parts
  wrote directly to the body file - which was incorrect.  So, clean up the
  whole mess to be consistent and more correct.
  
  Revision  Changes    Path
  1.1597    +2 -0      httpd-2.0/CHANGES
  
  Index: CHANGES
  ===================================================================
  RCS file: /home/cvs/httpd-2.0/CHANGES,v
  retrieving revision 1.1596
  retrieving revision 1.1597
  diff -u -u -r1.1596 -r1.1597
  --- CHANGES	21 Sep 2004 22:56:22 -0000	1.1596
  +++ CHANGES	22 Sep 2004 22:28:53 -0000	1.1597
  @@ -2,6 +2,8 @@
   
     [Remove entries to the current 2.0 section below, when backported]
   
  +  *) mod_disk_cache: Fix races in saving responses.  [Justin Erenkrantz]
  +
     *) Fix Expires handling in mod_cache.  [Justin Erenkrantz]
   
     *) Alter mod_expires to run at a different filter priority to allow
  
  
  
  1.62      +67 -89    httpd-2.0/modules/experimental/mod_disk_cache.c
  
  Index: mod_disk_cache.c
  ===================================================================
  RCS file: /home/cvs/httpd-2.0/modules/experimental/mod_disk_cache.c,v
  retrieving revision 1.61
  retrieving revision 1.62
  diff -u -u -r1.61 -r1.62
  --- mod_disk_cache.c	21 Sep 2004 22:56:23 -0000	1.61
  +++ mod_disk_cache.c	22 Sep 2004 22:28:54 -0000	1.62
  @@ -67,6 +67,7 @@
       char *name;
       apr_file_t *fd;          /* data file */
       apr_file_t *hfd;         /* headers file */
  +    apr_file_t *tfd;         /* temporary file for data */
       apr_off_t file_size;     /*  File size of the cached data file  */
       disk_cache_info_t disk_info; /* Header information. */
   } disk_cache_object_t;
  @@ -160,30 +161,16 @@
       }
   }
   
  -static apr_status_t file_cache_el_final(cache_handle_t *h, request_rec *r)
  +static apr_status_t file_cache_el_final(disk_cache_object_t *dobj,
  +                                        request_rec *r)
   {
  -    apr_status_t rv;
  -    disk_cache_conf *conf = ap_get_module_config(r->server->module_config,
  -                                                 &disk_cache_module);
  -    disk_cache_object_t *dobj = (disk_cache_object_t *) h->cache_obj->vobj;
  -
       /* move the data over */
  -    if (dobj->fd) {
  -        apr_file_flush(dobj->fd);
  -        if (!dobj->datafile) {
  -            dobj->datafile = data_file(r->pool, conf, dobj, h->cache_obj->key);
  -        }
  -        /* Remove old file with the same name. If remove fails, then
  -         * perhaps we need to create the directory tree where we are
  -         * about to write the new file.
  -         */
  -        rv = apr_file_remove(dobj->datafile, r->pool);
  -        if (rv != APR_SUCCESS) {
  -            mkdir_structure(conf, dobj->datafile, r->pool);
  -        }
  +    if (dobj->tfd) {
  +        apr_status_t rv;
  +
  +        apr_file_close(dobj->tfd);
   
  -        /*
  -         * This assumes that the tempfile is on the same file system
  +        /* This assumes that the tempfile is on the same file system
            * as the cache_root. If not, then we need a file copy/move
            * rather than a rename.
            */
  @@ -192,9 +179,7 @@
               /* XXX log */
           }
   
  -        apr_file_close(dobj->fd);
  -        dobj->fd = NULL;
  -        /* XXX log */
  +        dobj->tfd = NULL;
       }
   
       return APR_SUCCESS;
  @@ -202,17 +187,18 @@
   
   static apr_status_t file_cache_errorcleanup(disk_cache_object_t *dobj, request_rec *r)
   {
  -    if (dobj->fd) {
  -        apr_file_close(dobj->fd);
  -        dobj->fd = NULL;
  -    }
  -    /* Remove the header file, the temporary body file, and a potential old body file */
  +    /* Remove the header file and the body file. */
       apr_file_remove(dobj->hdrsfile, r->pool);
  -    apr_file_remove(dobj->tempfile, r->pool);
       apr_file_remove(dobj->datafile, r->pool);
   
  -    /* Return non-APR_SUCCESS in order to have mod_cache remove the disk_cache filter */
  -    return DECLINED;
  +    /* If we opened the temporary data file, close and remove it. */
  +    if (dobj->tfd) {
  +        apr_file_close(dobj->tfd);
  +        apr_file_remove(dobj->tempfile, r->pool);
  +        dobj->tfd = NULL;
  +    }
  +
  +    return APR_SUCCESS;
   }
   
   
  @@ -298,7 +284,7 @@
       }
   
       /* Allocate and initialize cache_object_t and disk_cache_object_t */
  -    obj = apr_pcalloc(r->pool, sizeof(*obj));
  +    h->cache_obj = obj = apr_pcalloc(r->pool, sizeof(*obj));
       obj->vobj = dobj = apr_pcalloc(r->pool, sizeof(*dobj));
   
       obj->key = apr_pstrdup(r->pool, key);
  @@ -307,25 +293,9 @@
       obj->complete = 0;   /* Cache object is not complete */
   
       dobj->name = obj->key;
  -
  -    /* open temporary file */
  +    dobj->datafile = data_file(r->pool, conf, dobj, key);
  +    dobj->hdrsfile = header_file(r->pool, conf, dobj, key);
       dobj->tempfile = apr_pstrcat(r->pool, conf->cache_root, AP_TEMPFILE, NULL);
  -    rv = apr_file_mktemp(&tmpfile, dobj->tempfile,
  -                         APR_CREATE | APR_READ | APR_WRITE | APR_EXCL, r->pool);
  -
  -    if (rv == APR_SUCCESS) {
  -        /* Populate the cache handle */
  -        h->cache_obj = obj;
  -
  -        ap_log_error(APLOG_MARK, APLOG_INFO, 0, r->server,
  -                     "disk_cache: Storing URL %s",  key);
  -    }
  -    else {
  -        ap_log_error(APLOG_MARK, APLOG_INFO, 0, r->server,
  -                     "disk_cache: Could not store URL %s [%d]", key, rv);
  -
  -        return DECLINED;
  -    }
   
       return OK;
   }
  @@ -354,7 +324,6 @@
           return DECLINED;
       }
   
  -
       /* Create and init the cache object */
       h->cache_obj = obj = apr_pcalloc(r->pool, sizeof(cache_object_t));
       obj->vobj = dobj = apr_pcalloc(r->pool, sizeof(disk_cache_object_t));
  @@ -364,6 +333,7 @@
       dobj->name = (char *) key;
       dobj->datafile = data_file(r->pool, conf, dobj, key);
       dobj->hdrsfile = header_file(r->pool, conf, dobj, key);
  +    dobj->tempfile = apr_pstrcat(r->pool, conf->cache_root, AP_TEMPFILE, NULL);
   
       /* Open the data file */
       flags = APR_READ|APR_BINARY;
  @@ -588,17 +558,11 @@
       apr_status_t rv;
       apr_size_t amt;
       disk_cache_object_t *dobj = (disk_cache_object_t*) h->cache_obj->vobj;
  -    apr_file_t *hfd = dobj->hfd;
   
  -    if (!hfd)  {
  +    if (!dobj->hfd)  {
           disk_cache_info_t disk_info;
           struct iovec iov[2];
   
  -        if (!dobj->hdrsfile) {
  -            dobj->hdrsfile = header_file(r->pool, conf, dobj,
  -                                         h->cache_obj->key);
  -        }
  -
           /* This is flaky... we need to manage the cache_info differently */
           h->cache_obj->info = *info;
   
  @@ -617,7 +581,6 @@
           if (rv != APR_SUCCESS) {
               return rv;
           }
  -        hfd = dobj->hfd;
           dobj->name = h->cache_obj->key;
   
           disk_info.format = DISK_FORMAT_VERSION;
  @@ -640,7 +603,7 @@
           iov[1].iov_base = dobj->name;
           iov[1].iov_len = disk_info.name_len;
   
  -        rv = apr_file_writev(hfd, (const struct iovec *) &iov, 2, &amt);
  +        rv = apr_file_writev(dobj->hfd, (const struct iovec *) &iov, 2, &amt);
           if (rv != APR_SUCCESS) {
               return rv;
           }
  @@ -650,7 +613,7 @@
   
               headers_out = ap_cache_cacheable_hdrs_out(r->pool, r->headers_out);
   
  -            rv = store_table(hfd, headers_out);
  +            rv = store_table(dobj->hfd, headers_out);
               if (rv != APR_SUCCESS) {
                   return rv;
               }
  @@ -666,12 +629,12 @@
           /* Make call to the same thing cache_select_url calls to crack Vary. */
           /* @@@ Some day, not today. */
           if (r->headers_in) {
  -            rv = store_table(hfd, r->headers_in);
  +            rv = store_table(dobj->hfd, r->headers_in);
               if (rv != APR_SUCCESS) {
                   return rv;
               }
           }
  -        apr_file_close(hfd); /* flush and close */
  +        apr_file_close(dobj->hfd); /* flush and close */
       }
       else {
           /* XXX log message */
  @@ -682,7 +645,8 @@
       return APR_SUCCESS;
   }
   
  -static apr_status_t store_body(cache_handle_t *h, request_rec *r, apr_bucket_brigade *b)
  +static apr_status_t store_body(cache_handle_t *h, request_rec *r,
  +                               apr_bucket_brigade *bb)
   {
       apr_bucket *e;
       apr_status_t rv;
  @@ -690,41 +654,51 @@
       disk_cache_conf *conf = ap_get_module_config(r->server->module_config,
                                                    &disk_cache_module);
   
  -    if (!dobj->fd) {
  -        rv = apr_file_open(&dobj->fd, dobj->tempfile,
  -                           APR_WRITE | APR_CREATE | APR_BINARY| APR_TRUNCATE | APR_BUFFERED,
  -                           APR_UREAD | APR_UWRITE, r->pool);
  +    /* We write to a temp file and then atomically rename the file over
  +     * in file_cache_el_final().
  +     */
  +    if (!dobj->tfd) {
  +        rv = apr_file_mktemp(&dobj->tfd, dobj->tempfile,
  +                             APR_CREATE | APR_WRITE | APR_BINARY |
  +                             APR_BUFFERED | APR_EXCL, r->pool);
           if (rv != APR_SUCCESS) {
               return rv;
           }
           dobj->file_size = 0;
       }
  -    for (e = APR_BRIGADE_FIRST(b);
  -         e != APR_BRIGADE_SENTINEL(b);
  +
  +    for (e = APR_BRIGADE_FIRST(bb);
  +         e != APR_BRIGADE_SENTINEL(bb);
            e = APR_BUCKET_NEXT(e))
       {
           const char *str;
  -        apr_size_t length;
  +        apr_size_t length, written;
           apr_bucket_read(e, &str, &length, APR_BLOCK_READ);
  -        if (apr_file_write(dobj->fd, str, &length) != APR_SUCCESS) {
  -          ap_log_error(APLOG_MARK, APLOG_ERR, 0, r->server,
  -                     "cache_disk: Error when writing cache file for URL %s",
  -                     h->cache_obj->key);
  -          /* Remove the intermediate cache file and return non-APR_SUCCESS */
  -          return file_cache_errorcleanup(dobj, r);
  +        rv = apr_file_write_full(dobj->tfd, str, length, &written);
  +        if (rv != APR_SUCCESS) {
  +            ap_log_error(APLOG_MARK, APLOG_ERR, 0, r->server,
  +                         "cache_disk: Error when writing cache file for URL %s",
  +                         h->cache_obj->key);
  +            /* Remove the intermediate cache file and return non-APR_SUCCESS */
  +            file_cache_errorcleanup(dobj, r);
  +            return APR_EGENERAL;
           }
  -        dobj->file_size += length;
  +        dobj->file_size += written;
           if (dobj->file_size > conf->maxfs) {
  -          ap_log_error(APLOG_MARK, APLOG_DEBUG, 0, r->server,
  -                     "cache_disk: URL %s failed the size check (%lu>%lu)",
  -                     h->cache_obj->key, (unsigned long)dobj->file_size, (unsigned long)conf->maxfs);
  -          /* Remove the intermediate cache file and return non-APR_SUCCESS */
  -          return file_cache_errorcleanup(dobj, r);
  +            ap_log_error(APLOG_MARK, APLOG_DEBUG, 0, r->server,
  +                         "cache_disk: URL %s failed the size check (%lu>%lu)",
  +                         h->cache_obj->key, (unsigned long)dobj->file_size,
  +                         (unsigned long)conf->maxfs);
  +            /* Remove the intermediate cache file and return non-APR_SUCCESS */
  +            file_cache_errorcleanup(dobj, r);
  +            return APR_EGENERAL;
           }
       }
   
  -    /* Was this the final bucket? If yes, close the body file and make sanity checks */
  -    if (APR_BUCKET_IS_EOS(APR_BRIGADE_LAST(b))) {
  +    /* Was this the final bucket? If yes, close the temp file and perform
  +     * sanity checks.
  +     */
  +    if (APR_BUCKET_IS_EOS(APR_BRIGADE_LAST(bb))) {
           if (h->cache_obj->info.len <= 0) {
             /* XXX Fixme: file_size isn't constrained by size_t. */
             h->cache_obj->info.len = dobj->file_size;
  @@ -737,17 +711,21 @@
                          (unsigned long)h->cache_obj->info.len,
                          (unsigned long)dobj->file_size);
             /* Remove the intermediate cache file and return non-APR_SUCCESS */
  -          return file_cache_errorcleanup(dobj, r);
  +          file_cache_errorcleanup(dobj, r);
  +          return APR_EGENERAL;
           }
           if (dobj->file_size < conf->minfs) {
             ap_log_error(APLOG_MARK, APLOG_DEBUG, 0, r->server,
                        "cache_disk: URL %s failed the size check (%lu<%lu)",
                        h->cache_obj->key, (unsigned long)dobj->file_size, (unsigned long)conf->minfs);
             /* Remove the intermediate cache file and return non-APR_SUCCESS */
  -          return file_cache_errorcleanup(dobj, r);
  +          file_cache_errorcleanup(dobj, r);
  +          return APR_EGENERAL;
           }
  +
           /* All checks were fine. Move tempfile to final destination */
  -        file_cache_el_final(h, r);    /* Link to the perm file, and close the descriptor */
  +        /* Link to the perm file, and close the descriptor */
  +        file_cache_el_final(dobj, r);
           ap_log_error(APLOG_MARK, APLOG_DEBUG, 0, r->server,
                        "disk_cache: Body for URL %s cached.",  dobj->name);
       }