The Samba-Bugzilla – Attachment 12948 Details for
Bug 12572
fd_open_atomic() can loop indefinitely when trying to open an invalid symlink with O_CREAT
Home
|
New
|
Browse
|
Search
|
[?]
|
Reports
|
Requests
|
Help
|
New Account
|
Log In
[x]
|
Forgot Password
Login:
[x]
[patch]
git-am fix for 4.6.next, 4.5.next.
bug-12572-v4.6,v4.5 (text/plain), 10.55 KB, created by
Jeremy Allison
on 2017-02-16 21:32:39 UTC
(
hide
)
Description:
git-am fix for 4.6.next, 4.5.next.
Filename:
MIME Type:
Creator:
Jeremy Allison
Created:
2017-02-16 21:32:39 UTC
Size:
10.55 KB
patch
obsolete
>From d40533311f1a281b361ca9aecfcbd46a2567a086 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 > >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 42db659d402..5f6f048cff9 100644 >--- a/source3/smbd/open.c >+++ b/source3/smbd/open.c >@@ -639,7 +639,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; > >@@ -671,59 +673,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 > > >From ab3561b83abd86decfde6e56c7142d4f91918625 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 > >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> > >Autobuild-User(master): Jeremy Allison <jra@samba.org> >Autobuild-Date(master): Thu Feb 16 22:06:51 CET 2017 on sn-devel-144 > >(cherry picked from commit 40d813cdb312fd8378db310543e0778193a1a684) >--- > 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 b72da85ec5e..c34924803da 100755 >--- a/source3/selftest/tests.py >+++ b/source3/selftest/tests.py >@@ -84,7 +84,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 95274b1bf1b..cafc1a8c3d4 100644 >--- a/source3/torture/torture.c >+++ b/source3/torture/torture.c >@@ -9497,6 +9497,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; >@@ -11059,6 +11159,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 >
You cannot view the attachment while viewing its details because your browser does not support IFRAMEs.
View the attachment on a separate page
.
View Attachment As Raw
Flags:
slow
:
review+
Actions:
View
Attachments on
bug 12572
:
12925
|
12928
|
12929
|
12942
|
12946
| 12948 |
12949