Attachment 12946 Details for Bug 12572 – Pair programmed with Ralph.

[patch] Pair programmed with Ralph.

12572 (text/plain), 9.91 KB, created by Jeremy Allison on 2017-02-16 00:19:20 UTC

(hide)

Description:

Filename:

MIME Type:

Creator: Jeremy Allison

Created: 2017-02-16 00:19:20 UTC

Size: 9.91 KB

patch

obsolete

>From d606a1662e797a2788e7c5b1023dbff19b976422 Mon Sep 17 00:00:00 2001
>From: Jeremy Allison <jra@samba.org>
>Date: Wed, 15 Feb 2017 15:42:52 -0800
>Subject: [PATCH 1/2] 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
>
>Signed-off-by: Ralph Boehme <slow@samba.org>
>Signed-off-by: Jeremy Allison <jra@samba.org>
>---
> source3/smbd/open.c | 96 ++++++++++++++++++++++++++---------------------------
> 1 file changed, 47 insertions(+), 49 deletions(-)
>
>diff --git a/source3/smbd/open.c b/source3/smbd/open.c
>index 931d76df44f..19ad47fc752 100644
>--- a/source3/smbd/open.c
>+++ b/source3/smbd/open.c
>@@ -640,7 +640,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;
> 
>@@ -672,59 +674,55 @@ 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;
>-			}
>-		}
>-		/* Create is done, or failed. */
>-		break;
>+	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_EQUAL(status, retry_status)) {
>+		return status;
>+	}
>+
>+	curr_flags = flags;
>+
>+	/*
>+	 * Keep file_existed up to date for clarity, even
>+	 * though we don't use it again.
>+	 */
>+	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);
> 	return status;
> }
> 
>-- 
>2.11.0.483.g087da7b7c-goog
>
>
>From d265f0921727e9b7f059f7fe60532e1de5e3f4c7 Mon Sep 17 00:00:00 2001
>From: Jeremy Allison <jra@samba.org>
>Date: Tue, 14 Feb 2017 12:59:58 -0800
>Subject: [PATCH 2/2] s3: torture: Regression test for smbd trying to open an
> invalid symlink.
>
>BUG: https://bugzilla.samba.org/show_bug.cgi?id=12572
>
>Signed-off-by: Jeremy Allison <jra@samba.org>
>---
> selftest/skip             |   1 +
> source3/selftest/tests.py |   2 +-
> source3/torture/torture.c | 101 ++++++++++++++++++++++++++++++++++++++++++++++
> 3 files changed, 103 insertions(+), 1 deletion(-)
>
>diff --git a/selftest/skip b/selftest/skip
>index 1e6d311f3c1..fd9932a47c0 100644
>--- a/selftest/skip
>+++ b/selftest/skip
>@@ -48,6 +48,7 @@
> ^samba3.smbtorture_s3.plain$ad_dc_ntvfs$.POSIX-SYMLINK-EA # Fails against the s4 ntvfs server
> ^samba3.smbtorture_s3.plain$ad_dc_ntvfs$.POSIX-OFD-LOCK # Fails against the s4 ntvfs server
> ^samba3.smbtorture_s3.plain$ad_dc_ntvfs$.POSIX-STREAM-DELETE # Fails against the s4 ntvfs server
>+^samba3.smbtorture_s3.plain$ad_dc_ntvfs$.WINDOWS-BAD-SYMLINK # Fails against the s4 ntvfs server
> ^samba3.smbtorture_s3.plain$ad_dc_ntvfs$.RENAME-ACCESS # Fails against the s4 ntvfs server
> ^samba3.smbtorture_s3.plain$ad_dc_ntvfs$.OWNER-RIGHTS # Don't test against the s4 ntvfs server anymore
> ^samba3.smbtorture_s3.plain$ad_dc_ntvfs$.PIDHIGH # Fails against the s4 ntvfs server
>diff --git a/source3/selftest/tests.py b/source3/selftest/tests.py
>index 8e1c33d66f0..4215eb043bf 100755
>--- a/source3/selftest/tests.py
>+++ b/source3/selftest/tests.py
>@@ -89,7 +89,7 @@ for t in tests:
>     plantestsuite("samba3.smbtorture_s3.vfs_aio_fork(simpleserver).%s" % t, "simpleserver", [os.path.join(samba3srcdir, "script/tests/test_smbtorture_s3.sh"), t, '//$SERVER_IP/vfs_aio_fork', '$USERNAME', '$PASSWORD', smbtorture3, "", "-l $LOCAL_PATH"])
> 
> posix_tests = ["POSIX", "POSIX-APPEND", "POSIX-SYMLINK-ACL", "POSIX-SYMLINK-EA", "POSIX-OFD-LOCK",
>-              "POSIX-STREAM-DELETE" ]
>+              "POSIX-STREAM-DELETE", "WINDOWS-BAD-SYMLINK" ]
> 
> for t in posix_tests:
>     plantestsuite("samba3.smbtorture_s3.plain(nt4_dc).%s" % t, "nt4_dc", [os.path.join(samba3srcdir, "script/tests/test_smbtorture_s3.sh"), t, '//$SERVER_IP/posix_share', '$USERNAME', '$PASSWORD', smbtorture3, "", "-l $LOCAL_PATH"])
>diff --git a/source3/torture/torture.c b/source3/torture/torture.c
>index 1bd7d6e1393..7e5198dc117 100644
>--- a/source3/torture/torture.c
>+++ b/source3/torture/torture.c
>@@ -9643,6 +9643,106 @@ static bool run_pidhigh(int dummy)
> 	return success;
> }
> 
>+/*
>+  Test Windows open on a bad POSIX symlink.
>+ */
>+static bool run_symlink_open_test(int dummy)
>+{
>+	static struct cli_state *cli;
>+	const char *fname = "non_existant_file";
>+	const char *sname = "dangling_symlink";
>+	uint16_t fnum = (uint16_t)-1;
>+	bool correct = false;
>+	NTSTATUS status;
>+	TALLOC_CTX *frame = NULL;
>+
>+	frame = talloc_stackframe();
>+
>+	printf("Starting Windows bad symlink open test\n");
>+
>+	if (!torture_open_connection(&cli, 0)) {
>+		TALLOC_FREE(frame);
>+		return false;
>+	}
>+
>+	smbXcli_conn_set_sockopt(cli->conn, sockops);
>+
>+	status = torture_setup_unix_extensions(cli);
>+	if (!NT_STATUS_IS_OK(status)) {
>+		TALLOC_FREE(frame);
>+		return false;
>+	}
>+
>+	/* Ensure nothing exists. */
>+	cli_setatr(cli, fname, 0, 0);
>+	cli_posix_unlink(cli, fname);
>+	cli_setatr(cli, sname, 0, 0);
>+	cli_posix_unlink(cli, sname);
>+
>+	/* Create a symlink pointing nowhere. */
>+	status = cli_posix_symlink(cli, fname, sname);
>+	if (!NT_STATUS_IS_OK(status)) {
>+		printf("cli_posix_symlink of %s -> %s failed (%s)\n",
>+			sname,
>+			fname,
>+			nt_errstr(status));
>+		goto out;
>+	}
>+
>+	/* Now ensure that a Windows open doesn't hang. */
>+	status = cli_ntcreate(cli,
>+			sname,
>+			0,
>+			FILE_READ_DATA|FILE_WRITE_DATA,
>+			0,
>+			FILE_SHARE_READ|FILE_SHARE_WRITE|FILE_SHARE_DELETE,
>+			FILE_OPEN_IF,
>+			0x0,
>+			0x0,
>+			&fnum,
>+			NULL);
>+
>+	/*
>+	 * We get either NT_STATUS_OBJECT_NAME_NOT_FOUND or
>+	 * NT_STATUS_OBJECT_PATH_NOT_FOUND depending on if
>+	 * we use O_NOFOLLOW on the server or not.
>+	 */
>+	if (NT_STATUS_EQUAL(status, NT_STATUS_OBJECT_NAME_NOT_FOUND) ||
>+			NT_STATUS_EQUAL(status,
>+				NT_STATUS_OBJECT_PATH_NOT_FOUND)) {
>+		correct = true;
>+	} else {
>+		printf("cli_ntcreate of %s returned %s - should return"
>+				" either (%s) or (%s)\n",
>+			sname,
>+			nt_errstr(status),
>+			nt_errstr(NT_STATUS_OBJECT_NAME_NOT_FOUND),
>+			nt_errstr(NT_STATUS_OBJECT_PATH_NOT_FOUND));
>+		goto out;
>+	}
>+
>+	correct = true;
>+
>+  out:
>+
>+	if (fnum != (uint16_t)-1) {
>+		cli_close(cli, fnum);
>+		fnum = (uint16_t)-1;
>+	}
>+
>+	cli_setatr(cli, sname, 0, 0);
>+	cli_posix_unlink(cli, sname);
>+	cli_setatr(cli, fname, 0, 0);
>+	cli_posix_unlink(cli, fname);
>+
>+	if (!torture_close_connection(cli)) {
>+		correct = false;
>+	}
>+
>+	TALLOC_FREE(frame);
>+	return correct;
>+}
>+
> static bool run_local_substitute(int dummy)
> {
> 	bool ok = true;
>@@ -11205,6 +11305,7 @@ static struct {
> 	{"POSIX-SYMLINK-EA", run_ea_symlink_test, 0},
> 	{"POSIX-STREAM-DELETE", run_posix_stream_delete, 0},
> 	{"POSIX-OFD-LOCK", run_posix_ofd_lock_test, 0},
>+	{"WINDOWS-BAD-SYMLINK", run_symlink_open_test, 0},
> 	{"CASE-INSENSITIVE-CREATE", run_case_insensitive_create, 0},
> 	{"ASYNC-ECHO", run_async_echo, 0},
> 	{ "UID-REGRESSION-TEST", run_uid_regression_test, 0},
>-- 
>2.11.0.483.g087da7b7c-goog
>

Flags:

slow: review+

Actions: View

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