s3: VFS: vfs_fruit. Fix the NetAtalk deny mode compatibility code.

This exhibited itself as a problem with OFD locks reported
as:

BUG: https://bugzilla.samba.org/show_bug.cgi?id=13770

However, due to underlying bugs in the vfs_fruit
code the file locks were not being properly applied.

There are two problems in fruit_check_access().

Problem #1:

Inside fruit_check_access() we have:

flags = fcntl(fsp->fh->fd, F_GETFL);
..
if (flags & (O_RDONLY|O_RDWR)) {

We shouldn't be calling fcntl(fsp->fh->fd, ..) directly.
fsp->fh->fd may be a made up number from an underlying
VFS module that has no meaning to a system call.

Secondly, in all POSIX systems - O_RDONLY is defined as
*zero*. O_RDWR = 2.

Which means flags & (O_RDONLY|O_RDWR) becomes (flags & 2),
not what we actually thought.

Problem #2:

deny_mode is *not* a bitmask, it's a set of discrete values.

Inside fruit_check_access() we have:

if (deny_mode & DENY_READ) and also (deny_mode & DENY_WRITE)

However, deny modes are defined as:

/* deny modes */
define DENY_DOS 0
define DENY_ALL 1
define DENY_WRITE 2
define DENY_READ 3
define DENY_NONE 4
define DENY_FCB 7

so if deny_mode = DENY_WRITE, or if deny_mode = DENY_READ
then it's going to trigger both the if (deny_mode & DENY_READ)
*and* the (deny_mode & DENY_WRITE) conditions.

These problems allowed the original test test_netatalk_lock code to
pass (which was added for BUG: https://bugzilla.samba.org/show_bug.cgi?id=13584
to demonstrate the lock order violation).

This patch refactors the fruit_check_access()
code to be much simpler (IMHO) to understand.

Firstly, pass in the SMB1/2 share mode, not old
DOS deny modes.

Secondly, read all the possible NetAtalk locks
into local variables:

netatalk_already_open_for_reading
netatalk_already_open_with_deny_read
netatalk_already_open_for_writing
netatalk_already_open_with_deny_write

Then do the share mode/access mode checks
with the requested values against any stored
netatalk modes/access modes.

Finally add in NetATalk compatible locks
that represent our share modes/access modes
into the file, with an early return if we don't
have FILE_READ_DATA (in which case we can't
write locks anyway).

The patch is easier to understand by looking
at the completed patched fruit_check_access()
function, rather than trying to look at the
diff.

Signed-off-by: Jeremy Allison <jra@samba.org>
Reviewed-by: Ralph Böhme <slow@samba.org>

diff --git a/source3/modules/vfs_fruit.c b/source3/modules/vfs_fruit.c
index 9f3fe24e5fcd2a2df392541134bdeb4673f8c2ad..c801f98eafb5f5e691db86d9cb0cae2078f02875 100644 (file)
--- a/source3/modules/vfs_fruit.c
+++ b/source3/modules/vfs_fruit.c
@@ -2664,156 +2664,146 @@ static bool test_netatalk_lock(files_struct *fsp, off_t in_offset)
 static NTSTATUS fruit_check_access(vfs_handle_struct *handle,
                                   files_struct *fsp,
                                   uint32_t access_mask,
-                                  uint32_t deny_mode)
+                                  uint32_t share_mode)
 {
        NTSTATUS status = NT_STATUS_OK;
-       bool open_for_reading, open_for_writing, deny_read, deny_write;
        off_t off;
-       bool have_read = false;
-       int flags;
+       bool share_for_read = (share_mode & FILE_SHARE_READ);
+       bool share_for_write = (share_mode & FILE_SHARE_WRITE);
+       bool netatalk_already_open_for_reading = false;
+       bool netatalk_already_open_for_writing = false;
+       bool netatalk_already_open_with_deny_read = false;
+       bool netatalk_already_open_with_deny_write = false;
 
        /* FIXME: hardcoded data fork, add resource fork */
        enum apple_fork fork_type = APPLE_FORK_DATA;
 
-       DEBUG(10, ("fruit_check_access: %s, am: %s/%s, dm: %s/%s\n",
+       DBG_DEBUG("fruit_check_access: %s, am: %s/%s, sm: 0x%x\n",
                  fsp_str_dbg(fsp),
                  access_mask & FILE_READ_DATA ? "READ" :"-",
                  access_mask & FILE_WRITE_DATA ? "WRITE" : "-",
-                 deny_mode & DENY_READ ? "DENY_READ" : "-",
-                 deny_mode & DENY_WRITE ? "DENY_WRITE" : "-"));
+                 share_mode);
 
        if (fsp->fh->fd == -1) {
                return NT_STATUS_OK;
        }
 
-       flags = fcntl(fsp->fh->fd, F_GETFL);
-       if (flags == -1) {
-               DBG_ERR("fcntl get flags [%s] fd [%d] failed [%s]\n",
-                       fsp_str_dbg(fsp), fsp->fh->fd, strerror(errno));
-               return map_nt_error_from_unix(errno);
-       }
-
-       if (flags & (O_RDONLY|O_RDWR)) {
-               /*
-                * Applying fcntl read locks requires an fd opened for
-                * reading. This means we won't be applying locks for
-                * files openend write-only, but what can we do...
-                */
-               have_read = true;
-       }
+       /* Read NetATalk opens and deny modes on the file. */
+       netatalk_already_open_for_reading = test_netatalk_lock(fsp,
+                               access_to_netatalk_brl(fork_type,
+                                       FILE_READ_DATA));
 
-       /*
-        * Check read access and deny read mode
-        */
-       if ((access_mask & FILE_READ_DATA) || (deny_mode & DENY_READ)) {
-               /* Check access */
-               open_for_reading = test_netatalk_lock(
-                       fsp, access_to_netatalk_brl(fork_type, FILE_READ_DATA));
+       netatalk_already_open_with_deny_read = test_netatalk_lock(fsp,
+                               denymode_to_netatalk_brl(fork_type,
+                                       DENY_READ));
 
-               deny_read = test_netatalk_lock(
-                       fsp, denymode_to_netatalk_brl(fork_type, DENY_READ));
+       netatalk_already_open_for_writing = test_netatalk_lock(fsp,
+                               access_to_netatalk_brl(fork_type,
+                                       FILE_WRITE_DATA));
 
-               DEBUG(10, ("read: %s, deny_write: %s\n",
-                         open_for_reading == true ? "yes" : "no",
-                         deny_read == true ? "yes" : "no"));
+       netatalk_already_open_with_deny_write = test_netatalk_lock(fsp,
+                               denymode_to_netatalk_brl(fork_type,
+                                       DENY_WRITE));
 
-               if (((access_mask & FILE_READ_DATA) && deny_read)
-                   || ((deny_mode & DENY_READ) && open_for_reading)) {
-                       return NT_STATUS_SHARING_VIOLATION;
-               }
+       /* If there are any conflicts - sharing violation. */
+       if ((access_mask & FILE_READ_DATA) &&
+                       netatalk_already_open_with_deny_read) {
+               return NT_STATUS_SHARING_VIOLATION;
+       }
 
-               /* Set locks */
-               if ((access_mask & FILE_READ_DATA) && have_read) {
-                       struct byte_range_lock *br_lck = NULL;
+       if (!share_for_read &&
+                       netatalk_already_open_for_reading) {
+               return NT_STATUS_SHARING_VIOLATION;
+       }
 
-                       off = access_to_netatalk_brl(fork_type, FILE_READ_DATA);
-                       br_lck = do_lock(
-                               handle->conn->sconn->msg_ctx, fsp,
-                               fsp->op->global->open_persistent_id, 1, off,
-                               READ_LOCK, POSIX_LOCK, false,
-                               &status, NULL);
+       if ((access_mask & FILE_WRITE_DATA) &&
+                       netatalk_already_open_with_deny_write) {
+               return NT_STATUS_SHARING_VIOLATION;
+       }
 
-                       TALLOC_FREE(br_lck);
+       if (!share_for_write &&
+                       netatalk_already_open_for_writing) {
+               return NT_STATUS_SHARING_VIOLATION;
+       }
 
-                       if (!NT_STATUS_IS_OK(status))  {
-                               return status;
-                       }
-               }
+       if (!(access_mask & FILE_READ_DATA)) {
+               /*
+                * Nothing we can do here, we need read access
+                * to set locks.
+                */
+               return NT_STATUS_OK;
+       }
 
-               if ((deny_mode & DENY_READ) && have_read) {
-                       struct byte_range_lock *br_lck = NULL;
+       /* Set NetAtalk locks matching our access */
+       if (access_mask & FILE_READ_DATA) {
+               struct byte_range_lock *br_lck = NULL;
 
-                       off = denymode_to_netatalk_brl(fork_type, DENY_READ);
-                       br_lck = do_lock(
-                               handle->conn->sconn->msg_ctx, fsp,
-                               fsp->op->global->open_persistent_id, 1, off,
-                               READ_LOCK, POSIX_LOCK, false,
-                               &status, NULL);
+               off = access_to_netatalk_brl(fork_type, FILE_READ_DATA);
+               br_lck = do_lock(
+                       handle->conn->sconn->msg_ctx, fsp,
+                       fsp->op->global->open_persistent_id, 1, off,
+                       READ_LOCK, POSIX_LOCK, false,
+                       &status, NULL);
 
-                       TALLOC_FREE(br_lck);
+               TALLOC_FREE(br_lck);
 
-                       if (!NT_STATUS_IS_OK(status)) {
-                               return status;
-                       }
+               if (!NT_STATUS_IS_OK(status))  {
+                       return status;
                }
        }
 
-       /*
-        * Check write access and deny write mode
-        */
-       if ((access_mask & FILE_WRITE_DATA) || (deny_mode & DENY_WRITE)) {
-               /* Check access */
-               open_for_writing = test_netatalk_lock(
-                       fsp, access_to_netatalk_brl(fork_type, FILE_WRITE_DATA));
+       if (!share_for_read) {
+               struct byte_range_lock *br_lck = NULL;
 
-               deny_write = test_netatalk_lock(
-                       fsp, denymode_to_netatalk_brl(fork_type, DENY_WRITE));
+               off = denymode_to_netatalk_brl(fork_type, DENY_READ);
+               br_lck = do_lock(
+                       handle->conn->sconn->msg_ctx, fsp,
+                       fsp->op->global->open_persistent_id, 1, off,
+                       READ_LOCK, POSIX_LOCK, false,
+                       &status, NULL);
 
-               DEBUG(10, ("write: %s, deny_write: %s\n",
-                         open_for_writing == true ? "yes" : "no",
-                         deny_write == true ? "yes" : "no"));
+               TALLOC_FREE(br_lck);
 
-               if (((access_mask & FILE_WRITE_DATA) && deny_write)
-                   || ((deny_mode & DENY_WRITE) && open_for_writing)) {
-                       return NT_STATUS_SHARING_VIOLATION;
+               if (!NT_STATUS_IS_OK(status)) {
+                       return status;
                }
+       }
 
-               /* Set locks */
-               if ((access_mask & FILE_WRITE_DATA) && have_read) {
-                       struct byte_range_lock *br_lck = NULL;
+       if (access_mask & FILE_WRITE_DATA) {
+               struct byte_range_lock *br_lck = NULL;
 
-                       off = access_to_netatalk_brl(fork_type, FILE_WRITE_DATA);
-                       br_lck = do_lock(
-                               handle->conn->sconn->msg_ctx, fsp,
-                               fsp->op->global->open_persistent_id, 1, off,
-                               READ_LOCK, POSIX_LOCK, false,
-                               &status, NULL);
+               off = access_to_netatalk_brl(fork_type, FILE_WRITE_DATA);
+               br_lck = do_lock(
+                       handle->conn->sconn->msg_ctx, fsp,
+                       fsp->op->global->open_persistent_id, 1, off,
+                       READ_LOCK, POSIX_LOCK, false,
+                       &status, NULL);
 
-                       TALLOC_FREE(br_lck);
+               TALLOC_FREE(br_lck);
 
-                       if (!NT_STATUS_IS_OK(status)) {
-                               return status;
-                       }
+               if (!NT_STATUS_IS_OK(status)) {
+                       return status;
                }
-               if ((deny_mode & DENY_WRITE) && have_read) {
-                       struct byte_range_lock *br_lck = NULL;
+       }
 
-                       off = denymode_to_netatalk_brl(fork_type, DENY_WRITE);
-                       br_lck = do_lock(
-                               handle->conn->sconn->msg_ctx, fsp,
-                               fsp->op->global->open_persistent_id, 1, off,
-                               READ_LOCK, POSIX_LOCK, false,
-                               &status, NULL);
+       if (!share_for_write) {
+               struct byte_range_lock *br_lck = NULL;
 
-                       TALLOC_FREE(br_lck);
+               off = denymode_to_netatalk_brl(fork_type, DENY_WRITE);
+               br_lck = do_lock(
+                       handle->conn->sconn->msg_ctx, fsp,
+                       fsp->op->global->open_persistent_id, 1, off,
+                       READ_LOCK, POSIX_LOCK, false,
+                       &status, NULL);
 
-                       if (!NT_STATUS_IS_OK(status)) {
-                               return status;
-                       }
+               TALLOC_FREE(br_lck);
+
+               if (!NT_STATUS_IS_OK(status)) {
+                       return status;
                }
        }
 
-       return status;
+       return NT_STATUS_OK;
 }
 
 static NTSTATUS check_aapl(vfs_handle_struct *handle,
@@ -6121,7 +6111,7 @@ static NTSTATUS fruit_create_file(vfs_handle_struct *handle,
                status = fruit_check_access(
                        handle, *result,
                        access_mask,
-                       map_share_mode_to_deny_mode(share_access, 0));
+                       share_access);
                if (!NT_STATUS_IS_OK(status)) {
                        goto fail;
                }