The Samba-Bugzilla – Attachment 12949 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.4.next
bug-12572-v4.4 (text/plain), 5.03 KB, created by
Jeremy Allison
on 2017-02-16 21:33:55 UTC
(
hide
)
Description:
git-am fix for 4.4.next
Filename:
MIME Type:
Creator:
Jeremy Allison
Created:
2017-02-16 21:33:55 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 >
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