From: Jeremy Allison Date: Thu, 7 Feb 2019 01:49:16 +0000 (-0800) Subject: s3: VFS: vfs_fruit. Fix the NetAtalk deny mode compatibility code. X-Git-Tag: ldb-1.3.8~14 X-Git-Url: http://git.ipfire.org/cgi-bin/gitweb.cgi?a=commitdiff_plain;h=aec654431dd8117c78da3b658f2dd5ee221b621f;p=thirdparty%2Fsamba.git 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 Reviewed-by: Ralph Böhme (cherry picked from commit 3204dc66f6801a7c8c87c48f601e0ebdee9e3d40) --- diff --git a/source3/modules/vfs_fruit.c b/source3/modules/vfs_fruit.c index f7e0bbce2ce..1a237676981 100644 --- 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, @@ -6091,7 +6081,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; }