Attachment 12949 Details for Bug 12572

[patch] git-am fix for 4.4.next

bug-12572-v4.4 (text/plain), 5.03 KB, created by Jeremy Allison on 2017-02-16 21:33 UTC

(hide)

Description:

Filename:

MIME Type:

Creator: Jeremy Allison

Created: 2017-02-16 21:33 UTC

Size: 5.03 KB

patch

obsolete

>From 5919a9e9d6052ed073f9b00096005cad359de92c Mon Sep 17 00:00:00 2001
>From: Jeremy Allison <jra@samba.org>
>Date: Wed, 15 Feb 2017 15:42:52 -0800
>Subject: [PATCH] s3: smbd: Don't loop infinitely on bad-symlink resolution.
>
>In the FILE_OPEN_IF case we have O_CREAT, but not
>O_EXCL. Previously we went into a loop trying first
>~(O_CREAT|O_EXCL), and if that returned ENOENT
>try (O_CREAT|O_EXCL). We kept looping indefinately
>until we got an error, or the file was created or
>opened.
>
>The big problem here is dangling symlinks. Opening
>without O_NOFOLLOW means both bad symlink
>and missing path return -1, ENOENT from open(). As POSIX
>is pathname based it's not possible to tell
>the difference between these two cases in a
>non-racy way, so change to try only two attempts before
>giving up.
>
>We don't have this problem for the O_NOFOLLOW
>case as we just return NT_STATUS_OBJECT_PATH_NOT_FOUND
>mapped from the ELOOP POSIX error and immediately
>returned.
>
>Unroll the loop logic to two tries instead.
>
>BUG: https://bugzilla.samba.org/show_bug.cgi?id=12572
>
>Pair-programmed-with: Ralph Boehme <slow@samba.org>
>
>Signed-off-by: Jeremy Allison <jra@samba.org>
>Signed-off-by: Ralph Boehme <slow@samba.org>
>Reviewed-by: Ralph Boehme <slow@samba.org>
>(cherry picked from commit 10c3e3923022485c720f322ca4f0aca5d7501310)
>---
> source3/smbd/open.c | 104 ++++++++++++++++++++++++++++------------------------
> 1 file changed, 56 insertions(+), 48 deletions(-)
>
>diff --git a/source3/smbd/open.c b/source3/smbd/open.c
>index ced3bb0afaa..e95a2e947ec 100644
>--- a/source3/smbd/open.c
>+++ b/source3/smbd/open.c
>@@ -621,7 +621,9 @@ static NTSTATUS fd_open_atomic(struct connection_struct *conn,
> 			bool *file_created)
> {
> 	NTSTATUS status = NT_STATUS_UNSUCCESSFUL;
>+	NTSTATUS retry_status;
> 	bool file_existed = VALID_STAT(fsp->fsp_name->st);
>+	int curr_flags;
> 
> 	*file_created = false;
> 
>@@ -653,59 +655,65 @@ static NTSTATUS fd_open_atomic(struct connection_struct *conn,
> 	 * we can never call O_CREAT without O_EXCL. So if
> 	 * we think the file existed, try without O_CREAT|O_EXCL.
> 	 * If we think the file didn't exist, try with
>-	 * O_CREAT|O_EXCL. Keep bouncing between these two
>-	 * requests until either the file is created, or
>-	 * opened. Either way, we keep going until we get
>-	 * a returnable result (error, or open/create).
>+	 * O_CREAT|O_EXCL.
>+	 *
>+	 * The big problem here is dangling symlinks. Opening
>+	 * without O_NOFOLLOW means both bad symlink
>+	 * and missing path return -1, ENOENT from open(). As POSIX
>+	 * is pathname based it's not possible to tell
>+	 * the difference between these two cases in a
>+	 * non-racy way, so change to try only two attempts before
>+	 * giving up.
>+	 *
>+	 * We don't have this problem for the O_NOFOLLOW
>+	 * case as it just returns NT_STATUS_OBJECT_PATH_NOT_FOUND
>+	 * mapped from the ELOOP POSIX error.
> 	 */
> 
>-	while(1) {
>-		int curr_flags = flags;
>+	curr_flags = flags;
> 
>-		if (file_existed) {
>-			/* Just try open, do not create. */
>-			curr_flags &= ~(O_CREAT);
>-			status = fd_open(conn, fsp, curr_flags, mode);
>-			if (NT_STATUS_EQUAL(status,
>-					NT_STATUS_OBJECT_NAME_NOT_FOUND)) {
>-				/*
>-				 * Someone deleted it in the meantime.
>-				 * Retry with O_EXCL.
>-				 */
>-				file_existed = false;
>-				DEBUG(10,("fd_open_atomic: file %s existed. "
>-					"Retry.\n",
>-					smb_fname_str_dbg(fsp->fsp_name)));
>-					continue;
>-			}
>-		} else {
>-			/* Try create exclusively, fail if it exists. */
>-			curr_flags |= O_EXCL;
>-			status = fd_open(conn, fsp, curr_flags, mode);
>-			if (NT_STATUS_EQUAL(status,
>-					NT_STATUS_OBJECT_NAME_COLLISION)) {
>-				/*
>-				 * Someone created it in the meantime.
>-				 * Retry without O_CREAT.
>-				 */
>-				file_existed = true;
>-				DEBUG(10,("fd_open_atomic: file %s "
>-					"did not exist. Retry.\n",
>-					smb_fname_str_dbg(fsp->fsp_name)));
>-				continue;
>-			}
>-			if (NT_STATUS_IS_OK(status)) {
>-				/*
>-				 * Here we've opened with O_CREAT|O_EXCL
>-				 * and got success. We *know* we created
>-				 * this file.
>-				 */
>-				*file_created = true;
>-			}
>+	if (file_existed) {
>+		curr_flags &= ~(O_CREAT);
>+		retry_status = NT_STATUS_OBJECT_NAME_NOT_FOUND;
>+	} else {
>+		curr_flags |= O_EXCL;
>+		retry_status = NT_STATUS_OBJECT_NAME_COLLISION;
>+	}
>+
>+	status = fd_open(conn, fsp, curr_flags, mode);
>+	if (NT_STATUS_IS_OK(status)) {
>+		if (!file_existed) {
>+			*file_created = true;
> 		}
>-		/* Create is done, or failed. */
>-		break;
>+		return NT_STATUS_OK;
> 	}
>+	if (!NT_STATUS_EQUAL(status, retry_status)) {
>+		return status;
>+	}
>+
>+	curr_flags = flags;
>+
>+	/*
>+	 * Keep file_existed up to date for clarity.
>+	 */
>+	if (NT_STATUS_EQUAL(status, NT_STATUS_OBJECT_NAME_NOT_FOUND)) {
>+		file_existed = false;
>+		curr_flags |= O_EXCL;
>+		DBG_DEBUG("file %s did not exist. Retry.\n",
>+			smb_fname_str_dbg(fsp->fsp_name));
>+	} else {
>+		file_existed = true;
>+		curr_flags &= ~(O_CREAT);
>+		DBG_DEBUG("file %s existed. Retry.\n",
>+			smb_fname_str_dbg(fsp->fsp_name));
>+	}
>+
>+	status = fd_open(conn, fsp, curr_flags, mode);
>+
>+	if (NT_STATUS_IS_OK(status) && (!file_existed)) {
>+		*file_created = true;
>+	}
>+
> 	return status;
> }
> 
>-- 
>2.11.0.483.g087da7b7c-goog
>

Flags:

slow: review+

Actions: View

Attachments on bug 12572: 12925 | 12928 | 12929 | 12942 | 12946 | 12948 | 12949