From 5919a9e9d6052ed073f9b00096005cad359de92c Mon Sep 17 00:00:00 2001 From: Jeremy Allison 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 Signed-off-by: Jeremy Allison Signed-off-by: Ralph Boehme Reviewed-by: Ralph Boehme (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