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>
(cherry picked from commit 3204dc66f6801a7c8c87c48f601e0ebdee9e3d40)

1 files changed, 96 insertions, 106 deletions

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;
 		}