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


git-svn-id: https://svn.apache.org/repos/asf/httpd/httpd/trunk@834049 13f79535-47bb-0310-9956-ffa450edef68

3 files changed, 77 insertions, 106 deletions

diff --git a/CHANGES b/CHANGES
index 6138e3a701..f7aaaf7938 100644
--- a/CHANGES
+++ b/CHANGES
@@ -10,6 +10,13 @@ Changes with Apache 2.3.3
      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]
diff --git a/modules/dav/fs/lock.c b/modules/dav/fs/lock.c
index 32220a79ba..97aa7eab45 100644
--- a/modules/dav/fs/lock.c
+++ b/modules/dav/fs/lock.c
@@ -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 @@ static void dav_fs_close_lockdb(dav_lockdb *lockdb)
 }
 
 /*
-** 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 @@ static dav_error * dav_fs_load_lock_record(dav_lockdb *lockdb, apr_datum_t key,
                 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 @@ static dav_error * dav_fs_add_locknull_state(
 
 /*
 ** 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 @@ static dav_error * dav_fs_remove_locknull_state(
         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;
 }
 
diff --git a/modules/dav/fs/repos.c b/modules/dav/fs/repos.c
index 0149697cb8..3e0b5d2b43 100644
--- a/modules/dav/fs/repos.c
+++ b/modules/dav/fs/repos.c
@@ -140,6 +140,11 @@ enum {
 */
 #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 @@ struct dav_stream {
     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 @@ static int dav_fs_is_parent_resource(
             && 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 @@ static dav_error * dav_fs_open_stream(const dav_resource *resource,
 
     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_open_stream(const dav_resource *resource,
 
 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 @@ static dav_error * dav_fs_walker(dav_fs_walker_context *fsctx, int depth)
             /* ### 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;
         }