locking: put the Solaris share reservation after our locking stuff

Fix for Bug 560: put the Solaris share reservation after our locking stuff.
Note that this still leaves room for a race condition where between placing our own
locks above and putting the Solaris share move below another client puts a lock.
We then end up with set locks from above and return with an error code, the proper
fix requires a sane cleanup function for the error path in this function.

Signed-off-by: Ralph Boehme <rb@sernet.de>

diff --git a/NEWS b/NEWS
index 70304f7b181973e43e150f0a5facb10a511a2db6..22b43072739b8cbc2a417e7e720ce6f1d4feea01 100644 (file)
--- a/NEWS
+++ b/NEWS
@@ -3,6 +3,7 @@ Changes in 3.1.2
 * FIX: Option "vol dbpath" was broken in 3.1.1
 * FIX: Spotlight: file modification date, bug #545
 * FIX: Improve reliability of afpd child handler
+* FIX: put the Solaris share reservation after our locking stuff, bug #560.
 
 Changes in 3.1.1
 ================

diff --git a/etc/afpd/fork.c b/etc/afpd/fork.c
index 24284c1d745f0bc62fce6668637b7333943a8964..4311655ec6597a19a55d9bff6cffbb061fbfa204 100644 (file)
--- a/etc/afpd/fork.c
+++ b/etc/afpd/fork.c
@@ -153,27 +153,6 @@ static int fork_setmode(const AFPObj *obj, struct adouble *adp, int eid, int acc
     int denyreadset;
     int denywriteset;
 
-#ifdef HAVE_FSHARE_T
-    fshare_t shmd;
-
-    if (obj->options.flags & OPTION_SHARE_RESERV) {
-        shmd.f_access = (access & OPENACC_RD ? F_RDACC : 0) | (access & OPENACC_WR ? F_WRACC : 0);
-        if (shmd.f_access == 0)
-            /* we must give an access mode, otherwise fcntl will complain */
-            shmd.f_access = F_RDACC;
-        shmd.f_deny = (access & OPENACC_DRD ? F_RDDNY : F_NODNY) | (access & OPENACC_DWR) ? F_WRDNY : 0;
-        shmd.f_id = ofrefnum;
-
-        int fd = (eid == ADEID_DFORK) ? ad_data_fileno(adp) : ad_reso_fileno(adp);
-
-        if (fd != -1 && fd != AD_SYMLINK && fcntl(fd, F_SHARE, &shmd) != 0) {
-            LOG(log_debug, logtype_afpd, "fork_setmode: fcntl: %s", strerror(errno));
-            errno = EACCES;
-            return -1;
-        }
-    }
-#endif
-
     if (! (access & (OPENACC_WR | OPENACC_RD | OPENACC_DWR | OPENACC_DRD))) {
         return ad_lock(adp, eid, ADLOCK_RD | ADLOCK_FILELOCK, AD_FILELOCK_OPEN_NONE, 1, ofrefnum);
     }
@@ -233,6 +212,35 @@ static int fork_setmode(const AFPObj *obj, struct adouble *adp, int eid, int acc
         }
     }
 
+    /*
+     * Fix for Bug 560: put the Solaris share reservation after our locking stuff.
+     * Note that this still leaves room for a race condition where between placing our own
+     * locks above and putting the Solaris share move below another client puts a lock.
+     * We then end up with set locks from above and return with an error code, the proper
+     * fix requires a sane cleanup function for the error path in this function.
+     */
+
+#ifdef HAVE_FSHARE_T
+    fshare_t shmd;
+
+    if (obj->options.flags & OPTION_SHARE_RESERV) {
+        shmd.f_access = (access & OPENACC_RD ? F_RDACC : 0) | (access & OPENACC_WR ? F_WRACC : 0);
+        if (shmd.f_access == 0)
+            /* we must give an access mode, otherwise fcntl will complain */
+            shmd.f_access = F_RDACC;
+        shmd.f_deny = (access & OPENACC_DRD ? F_RDDNY : F_NODNY) | (access & OPENACC_DWR) ? F_WRDNY : 0;
+        shmd.f_id = ofrefnum;
+
+        int fd = (eid == ADEID_DFORK) ? ad_data_fileno(adp) : ad_reso_fileno(adp);
+
+        if (fd != -1 && fd != AD_SYMLINK && fcntl(fd, F_SHARE, &shmd) != 0) {
+            LOG(log_debug, logtype_afpd, "fork_setmode: fcntl: %s", strerror(errno));
+            errno = EACCES;
+            return -1;
+        }
+    }
+#endif
+
     return 0;
 }