httpd-cvs mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From s.@apache.org
Subject svn commit: r834049 - in /httpd/httpd/trunk: CHANGES modules/dav/fs/lock.c modules/dav/fs/repos.c
Date Mon, 09 Nov 2009 13:14:08 GMT
Author: sf
Date: Mon Nov  9 13:14:07 2009
New Revision: 834049

URL: http://svn.apache.org/viewvc?rev=834049&view=rev
Log:
Make PUT with DAV_MODE_WRITE_TRUNC create a temporary file first and, when the
transfer has been completed successfully, move it over the old file.

Since this would break inode keyed locking, switch to filename keyed locking
exclusively.

PR: 39815
Submitted by: Paul Querna, Stefan Fritsch

Modified:
    httpd/httpd/trunk/CHANGES
    httpd/httpd/trunk/modules/dav/fs/lock.c
    httpd/httpd/trunk/modules/dav/fs/repos.c

Modified: httpd/httpd/trunk/CHANGES
URL: http://svn.apache.org/viewvc/httpd/httpd/trunk/CHANGES?rev=834049&r1=834048&r2=834049&view=diff
==============================================================================
--- httpd/httpd/trunk/CHANGES [utf-8] (original)
+++ httpd/httpd/trunk/CHANGES [utf-8] Mon Nov  9 13:14:07 2009
@@ -10,6 +10,13 @@
      mod_proxy_ftp: NULL pointer dereference on error paths.
      [Stefan Fritsch <sf fritsch.de>, Joe Orton]
 
+  *) mod_dav_fs: Make PUT create files atomically and no longer destroy the
+     old file if the transfer aborted. PR 39815. [Paul Querna, Stefan Fritsch]
+
+  *) mod_dav_fs: Remove inode keyed locking as this conflicts with atomically
+     creating files. This is a format cange of the DavLockDB. The old
+     DavLockDB must be deleted on upgrade. [Stefan Fritsch]
+
   *) mod_log_config: Make ${cookie}C correctly match whole cookie names
      instead of substrings. PR 28037. [Dan Franklin <dan dan-franklin.com>,
      Stefan Fritsch]

Modified: httpd/httpd/trunk/modules/dav/fs/lock.c
URL: http://svn.apache.org/viewvc/httpd/httpd/trunk/modules/dav/fs/lock.c?rev=834049&r1=834048&r2=834049&view=diff
==============================================================================
--- httpd/httpd/trunk/modules/dav/fs/lock.c (original)
+++ httpd/httpd/trunk/modules/dav/fs/lock.c Mon Nov  9 13:14:07 2009
@@ -48,9 +48,7 @@
 **
 ** KEY
 **
-** The database is keyed by a key_type unsigned char (DAV_TYPE_INODE or
-** DAV_TYPE_FNAME) followed by inode and device number if possible,
-** otherwise full path (in the case of Win32 or lock-null resources).
+** The database is keyed by the full path.
 **
 ** VALUE
 **
@@ -82,9 +80,6 @@
 #define DAV_LOCK_DIRECT         1
 #define DAV_LOCK_INDIRECT       2
 
-#define DAV_TYPE_INODE          10
-#define DAV_TYPE_FNAME          11
-
 
 /* ack. forward declare. */
 static dav_error * dav_fs_remove_locknull_member(apr_pool_t *p,
@@ -372,68 +367,26 @@
 }
 
 /*
-** dav_fs_build_fname_key
-**
-** Given a pathname, build a DAV_TYPE_FNAME lock database key.
+** dav_fs_build_key:  Given a resource, return a apr_datum_t key
+**    to look up lock information for this file.
 */
-static apr_datum_t dav_fs_build_fname_key(apr_pool_t *p, const char *pathname)
+static apr_datum_t dav_fs_build_key(apr_pool_t *p,
+                                    const dav_resource *resource)
 {
+    const char *pathname = dav_fs_pathname(resource);
     apr_datum_t key;
 
     /* ### does this allocation have a proper lifetime? need to check */
     /* ### can we use a buffer for this? */
 
-    /* size is TYPE + pathname + null */
-    key.dsize = strlen(pathname) + 2;
-    key.dptr = apr_palloc(p, key.dsize);
-    *key.dptr = DAV_TYPE_FNAME;
-    memcpy(key.dptr + 1, pathname, key.dsize - 1);
+    key.dsize = strlen(pathname) + 1;
+    key.dptr = apr_pstrmemdup(p, pathname, key.dsize - 1);
     if (key.dptr[key.dsize - 2] == '/')
         key.dptr[--key.dsize - 1] = '\0';
     return key;
 }
 
 /*
-** dav_fs_build_key:  Given a resource, return a apr_datum_t key
-**    to look up lock information for this file.
-**
-**    (inode/dev not supported or file is lock-null):
-**       apr_datum_t->dvalue = full path
-**
-**    (inode/dev supported and file exists ):
-**       apr_datum_t->dvalue = inode, dev
-*/
-static apr_datum_t dav_fs_build_key(apr_pool_t *p,
-                                    const dav_resource *resource)
-{
-    const char *file = dav_fs_pathname(resource);
-    apr_datum_t key;
-    apr_finfo_t finfo;
-    apr_status_t rv;
-
-    /* ### use lstat() ?? */
-    /*
-     * XXX: What for platforms with no IDENT (dev/inode)?
-     */
-    rv = apr_stat(&finfo, file, APR_FINFO_IDENT, p);
-    if ((rv == APR_SUCCESS || rv == APR_INCOMPLETE)
-        && ((finfo.valid & APR_FINFO_IDENT) == APR_FINFO_IDENT))
-    {
-        /* ### can we use a buffer for this? */
-        key.dsize = 1 + sizeof(finfo.inode) + sizeof(finfo.device);
-        key.dptr = apr_palloc(p, key.dsize);
-        *key.dptr = DAV_TYPE_INODE;
-        memcpy(key.dptr + 1, &finfo.inode, sizeof(finfo.inode));
-        memcpy(key.dptr + 1 + sizeof(finfo.inode), &finfo.device,
-               sizeof(finfo.device));
-
-        return key;
-    }
-
-    return dav_fs_build_fname_key(p, file);
-}
-
-/*
 ** dav_fs_lock_expired:  return 1 (true) if the given timeout is in the past
 **    or present (the lock has expired), or 0 (false) if in the future
 **    (the lock has not yet expired).
@@ -626,15 +579,14 @@
                 need_save = DAV_TRUE;
 
                 /* Remove timed-out locknull fm .locknull list */
-                if (*key.dptr == DAV_TYPE_FNAME) {
-                    const char *fname = key.dptr + 1;
+                {
                     apr_finfo_t finfo;
                     apr_status_t rv;
 
                     /* if we don't see the file, then it's a locknull */
-                    rv = apr_stat(&finfo, fname, APR_FINFO_MIN | APR_FINFO_LINK, p);
+                    rv = apr_stat(&finfo, key.dptr, APR_FINFO_MIN | APR_FINFO_LINK, p);
                     if (rv != APR_SUCCESS && rv != APR_INCOMPLETE) {
-                        if ((err = dav_fs_remove_locknull_member(p, fname, &buf)) !=
NULL) {
+                        if ((err = dav_fs_remove_locknull_member(p, key.dptr, &buf))
!= NULL) {
                             /* ### push a higher-level description? */
                             return err;
                         }
@@ -989,13 +941,8 @@
 
 /*
 ** dav_fs_remove_locknull_state:  Given a request, check to see if r->filename
-**    is/was a lock-null resource.  If so, return it to an existant state.
-**
-**    ### this function is broken... it doesn't check!
-**
-**    In this implementation, this involves two things:
-**    (a) remove it from the list in the appropriate .DAV/locknull file
-**    (b) on *nix, convert the key from a filename to an inode.
+**    is/was a lock-null resource.  If so, return it to an existant state, i.e.
+**    remove it from the list in the appropriate .DAV/locknull file.
 */
 static dav_error * dav_fs_remove_locknull_state(
     dav_lockdb *lockdb,
@@ -1011,35 +958,6 @@
         return err;
     }
 
-    {
-        dav_lock_discovery *ld;
-        dav_lock_indirect  *id;
-        apr_datum_t key;
-
-        /*
-        ** Fetch the lock(s) that made the resource lock-null. Remove
-        ** them under the filename key. Obtain the new inode key, and
-        ** save the same lock information under it.
-        */
-        key = dav_fs_build_fname_key(p, pathname);
-        if ((err = dav_fs_load_lock_record(lockdb, key, DAV_CREATE_LIST,
-                                           &ld, &id)) != NULL) {
-            /* ### insert a higher-level error description */
-            return err;
-        }
-
-        if ((err = dav_fs_save_lock_record(lockdb, key, NULL, NULL)) != NULL) {
-            /* ### insert a higher-level error description */
-            return err;
-        }
-
-        key = dav_fs_build_key(p, resource);
-        if ((err = dav_fs_save_lock_record(lockdb, key, ld, id)) != NULL) {
-            /* ### insert a higher-level error description */
-            return err;
-        }
-    }
-
     return NULL;
 }
 

Modified: httpd/httpd/trunk/modules/dav/fs/repos.c
URL: http://svn.apache.org/viewvc/httpd/httpd/trunk/modules/dav/fs/repos.c?rev=834049&r1=834048&r2=834049&view=diff
==============================================================================
--- httpd/httpd/trunk/modules/dav/fs/repos.c (original)
+++ httpd/httpd/trunk/modules/dav/fs/repos.c Mon Nov  9 13:14:07 2009
@@ -140,6 +140,11 @@
 */
 #define DAV_PROPID_FS_executable        1
 
+/*
+ * prefix for temporary files
+ */
+#define DAV_FS_TMP_PREFIX ".davfs." 
+
 static const dav_liveprop_spec dav_fs_props[] =
 {
     /* standard DAV properties */
@@ -192,6 +197,7 @@
     apr_pool_t *p;
     apr_file_t *f;
     const char *pathname;       /* we may need to remove it at close time */
+    const char *temppath;
 };
 
 /* returns an appropriate HTTP status code given an APR status code for a
@@ -852,6 +858,14 @@
             && ctx2->pathname[len1] == '/');
 }
 
+static apr_status_t tmpfile_cleanup(void *data) {
+        dav_stream *ds = data;
+        if (ds->temppath) {
+                apr_file_remove(ds->temppath, ds->p);
+        }
+        return APR_SUCCESS;
+}
+
 static dav_error * dav_fs_open_stream(const dav_resource *resource,
                                       dav_stream_mode mode,
                                       dav_stream **stream)
@@ -876,7 +890,19 @@
 
     ds->p = p;
     ds->pathname = resource->info->pathname;
-    rv = apr_file_open(&ds->f, ds->pathname, flags, APR_OS_DEFAULT, ds->p);
+    ds->temppath = NULL;
+
+    if (mode == DAV_MODE_WRITE_TRUNC) {
+        ds->temppath = apr_pstrcat(p, ap_make_dirstr_parent(p, ds->pathname),
+                                   DAV_FS_TMP_PREFIX "XXXXXX", NULL);
+        rv = apr_file_mktemp(&ds->f, ds->temppath, flags, ds->p);
+        apr_pool_cleanup_register(p, ds, tmpfile_cleanup,
+                                  apr_pool_cleanup_null);
+    }
+    else {
+        rv = apr_file_open(&ds->f, ds->pathname, flags, APR_OS_DEFAULT, ds->p);
+    }
+
     if (rv != APR_SUCCESS) {
         return dav_new_error(p, MAP_IO2HTTP(rv), 0,
                              "An error occurred while opening a resource.");
@@ -890,16 +916,32 @@
 
 static dav_error * dav_fs_close_stream(dav_stream *stream, int commit)
 {
+    apr_status_t rv;
+
     apr_file_close(stream->f);
 
     if (!commit) {
-        if (apr_file_remove(stream->pathname, stream->p) != APR_SUCCESS) {
-            /* ### use a better description? */
-            return dav_new_error(stream->p, HTTP_INTERNAL_SERVER_ERROR, 0,
-                                 "There was a problem removing (rolling "
-                                 "back) the resource "
-                                 "when it was being closed.");
+        if (stream->temppath) {
+            apr_pool_cleanup_run(stream->p, stream, tmpfile_cleanup);
+        }
+        else {
+            if (apr_file_remove(stream->pathname, stream->p) != APR_SUCCESS) {
+                /* ### use a better description? */
+                return dav_new_error(stream->p, HTTP_INTERNAL_SERVER_ERROR, 0,
+                                     "There was a problem removing (rolling "
+                                     "back) the resource "
+                                     "when it was being closed.");
+            }
+        }
+    }
+    else if (stream->temppath) {
+        rv = apr_file_rename(stream->temppath, stream->pathname, stream->p);
+        if (rv) {
+            return dav_new_error(stream->p, HTTP_INTERNAL_SERVER_ERROR, rv,
+                                 "There was a problem writing the file "
+                                 "atomically after writes.");
         }
+        apr_pool_cleanup_kill(stream->p, stream, tmpfile_cleanup);
     }
 
     return NULL;
@@ -1451,14 +1493,18 @@
             /* ### need to authorize each file */
             /* ### example: .htaccess is normally configured to fail auth */
 
-            /* stuff in the state directory is never authorized! */
-            if (!strcmp(dirent.name, DAV_FS_STATE_DIR)) {
+            /* stuff in the state directory and temp files are never authorized! */
+            if (!strcmp(dirent.name, DAV_FS_STATE_DIR) ||
+                !strncmp(dirent.name, DAV_FS_TMP_PREFIX,
+                         strlen(DAV_FS_TMP_PREFIX))) {
                 continue;
             }
         }
-        /* skip the state dir unless a HIDDEN is performed */
+        /* skip the state dir and temp files unless a HIDDEN is performed */
         if (!(params->walk_type & DAV_WALKTYPE_HIDDEN)
-            && !strcmp(dirent.name, DAV_FS_STATE_DIR)) {
+            && (!strcmp(dirent.name, DAV_FS_STATE_DIR) ||
+                !strncmp(dirent.name, DAV_FS_TMP_PREFIX,
+                         strlen(DAV_FS_TMP_PREFIX)))) {
             continue;
         }
 



Mime
View raw message