From 461471df49cba1d6bae6d22f89a8907dfc9bc5cf Mon Sep 17 00:00:00 2001 From: Volker Lendecke Date: Wed, 9 Feb 2022 10:02:46 +0100 Subject: [PATCH 01/14] smbd: Slightly simplify create_file_unixpath() Avoid the "needs_fsp_unlink" variable, describe the talloc hierarchy a bit differently in the comments. BUG: https://bugzilla.samba.org/show_bug.cgi?id=14975 Signed-off-by: Volker Lendecke Reviewed-by: Jeremy Allison (cherry picked from commit 1c1734974fcf1d060bc6bcdbe1858cba1b7e5a73) --- source3/smbd/open.c | 51 ++++++++++++++++----------------------------- 1 file changed, 18 insertions(+), 33 deletions(-) diff --git a/source3/smbd/open.c b/source3/smbd/open.c index 0427b0cef9d..6167fbc5775 100644 --- a/source3/smbd/open.c +++ b/source3/smbd/open.c @@ -5761,42 +5761,27 @@ static NTSTATUS create_file_unixpath(connection_struct *conn, } } - /* - * Now either reuse smb_fname->fsp or allocate a new fsp if - * smb_fname->fsp is NULL. The latter will be the case when processing a - * request to create a file that doesn't exist. - */ if (smb_fname->fsp != NULL) { - bool need_fsp_unlink = true; + + fsp = smb_fname->fsp; /* - * This is really subtle. If someone passes in an smb_fname - * where smb_fname actually is taken from fsp->fsp_name, then - * the lifetime of these objects is meant to be the same. + * We're about to use smb_fname->fsp for the fresh open. * - * This is commonly the case from an SMB1 path-based call, - * (call_trans2qfilepathinfo) where we use the pathref fsp - * (smb_fname->fsp) as the handle. In this case we must not - * unlink smb_fname->fsp from it's owner. - * - * The asserts below: - * - * SMB_ASSERT(fsp->fsp_name->fsp != NULL); - * SMB_ASSERT(fsp->fsp_name->fsp == fsp); - * - * ensure the required invarients are met. + * Every fsp passed in via smb_fname->fsp already + * holds a fsp->fsp_name. If it is already this + * fsp->fsp_name that we got passed in as our input + * argument smb_fname, these two are assumed to have + * the same lifetime: Every fsp hangs of "conn", and + * fsp->fsp_name is its talloc child. */ - if (smb_fname->fsp->fsp_name == smb_fname) { - need_fsp_unlink = false; - } - fsp = smb_fname->fsp; - - if (need_fsp_unlink) { + if (smb_fname != smb_fname->fsp->fsp_name) { /* - * Unlink the fsp from the smb_fname so the fsp is not - * autoclosed by the smb_fname pathref fsp talloc - * destructor. + * "smb_fname" is temporary in this case, but + * the destructor of smb_fname would also tear + * down the fsp we're about to use. Unlink + * them from each other. */ smb_fname_fsp_unlink(smb_fname); } @@ -5814,10 +5799,10 @@ static NTSTATUS create_file_unixpath(connection_struct *conn, fd_close(tmp_base_fsp); file_free(NULL, tmp_base_fsp); } - } - - if (fsp == NULL) { - /* Creating file */ + } else { + /* + * No fsp passed in that we can use, create one + */ status = file_new(req, conn, &fsp); if(!NT_STATUS_IS_OK(status)) { goto fail; -- 2.32.0 From 4714488ae68fbfe5ad032af78d25ddeba211aac0 Mon Sep 17 00:00:00 2001 From: Volker Lendecke Date: Tue, 1 Feb 2022 17:14:34 +0100 Subject: [PATCH 02/14] smbd: Move the call to file_free() out of close_directory() Call file_free() just once BUG: https://bugzilla.samba.org/show_bug.cgi?id=14975 Signed-off-by: Volker Lendecke Reviewed-by: Jeremy Allison (cherry picked from commit 9966b5e233ef2ff0368ba5860c824c7cd6420415) --- source3/smbd/close.c | 8 +------- 1 file changed, 1 insertion(+), 7 deletions(-) diff --git a/source3/smbd/close.c b/source3/smbd/close.c index 8abd3fb3861..cd406f08bee 100644 --- a/source3/smbd/close.c +++ b/source3/smbd/close.c @@ -1379,7 +1379,6 @@ static NTSTATUS close_directory(struct smb_request *req, files_struct *fsp, if (lck == NULL) { DEBUG(0, ("close_directory: Could not get share mode lock for " "%s\n", fsp_str_dbg(fsp))); - file_free(req, fsp); return NT_STATUS_INVALID_PARAMETER; } @@ -1429,7 +1428,6 @@ static NTSTATUS close_directory(struct smb_request *req, files_struct *fsp, if (!NT_STATUS_IS_OK(status)) { DEBUG(5, ("delete_all_streams failed: %s\n", nt_errstr(status))); - file_free(req, fsp); /* unbecome user. */ pop_sec_ctx(); return status; @@ -1472,11 +1470,6 @@ static NTSTATUS close_directory(struct smb_request *req, files_struct *fsp, strerror(errno))); } - /* - * Do the code common to files and directories. - */ - file_free(req, fsp); - if (NT_STATUS_IS_OK(status) && !NT_STATUS_IS_OK(status1)) { status = status1; } @@ -1562,6 +1555,7 @@ NTSTATUS close_file(struct smb_request *req, files_struct *fsp, status = NT_STATUS_OK; } else if (fsp->fsp_flags.is_directory) { status = close_directory(req, fsp, close_type); + file_free(req, fsp); } else { status = close_normal_file(req, fsp, close_type); } -- 2.32.0 From 56d91b7cace1b4eedb36aa3aac7f4eab8182f2a9 Mon Sep 17 00:00:00 2001 From: Volker Lendecke Date: Tue, 1 Feb 2022 17:17:36 +0100 Subject: [PATCH 03/14] smbd: Move the call to file_free() out of close_normal_file() Call file_free() just once BUG: https://bugzilla.samba.org/show_bug.cgi?id=14975 Signed-off-by: Volker Lendecke Reviewed-by: Jeremy Allison (cherry picked from commit 2293ca5b572178404273856f8d8989a5ee7de80c) --- source3/smbd/close.c | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/source3/smbd/close.c b/source3/smbd/close.c index cd406f08bee..bbcdae45429 100644 --- a/source3/smbd/close.c +++ b/source3/smbd/close.c @@ -782,7 +782,6 @@ static NTSTATUS close_normal_file(struct smb_request *req, files_struct *fsp, DEBUG(10, ("%s disconnected durable handle for file %s\n", conn->session_info->unix_info->unix_name, fsp_str_dbg(fsp))); - file_free(req, fsp); return NT_STATUS_OK; } @@ -833,7 +832,6 @@ static NTSTATUS close_normal_file(struct smb_request *req, files_struct *fsp, conn->num_files_open - 1, nt_errstr(status) )); - file_free(req, fsp); return status; } /**************************************************************************** @@ -1558,6 +1556,7 @@ NTSTATUS close_file(struct smb_request *req, files_struct *fsp, file_free(req, fsp); } else { status = close_normal_file(req, fsp, close_type); + file_free(req, fsp); } if (close_base_fsp) { -- 2.32.0 From a2045648e06d8c9f4e09b853c24b4342c247d408 Mon Sep 17 00:00:00 2001 From: Volker Lendecke Date: Tue, 1 Feb 2022 17:19:54 +0100 Subject: [PATCH 04/14] smbd: Move the call to file_free() out of close_fake_file() Centralize calling file_free(), but leave close_fake_file() in for API symmetry reasons. BUG: https://bugzilla.samba.org/show_bug.cgi?id=14975 Signed-off-by: Volker Lendecke Reviewed-by: Jeremy Allison (cherry picked from commit 244c5a7d31c3a37082b320680f2b71108d77bbd4) --- source3/smbd/close.c | 1 + source3/smbd/fake_file.c | 4 +++- 2 files changed, 4 insertions(+), 1 deletion(-) diff --git a/source3/smbd/close.c b/source3/smbd/close.c index bbcdae45429..dd4a4c3595f 100644 --- a/source3/smbd/close.c +++ b/source3/smbd/close.c @@ -1532,6 +1532,7 @@ NTSTATUS close_file(struct smb_request *req, files_struct *fsp, if (fsp->fake_file_handle != NULL) { status = close_fake_file(req, fsp); + file_free(req, fsp); } else if (fsp->print_file != NULL) { /* FIXME: return spool errors */ print_spool_end(fsp, close_type); diff --git a/source3/smbd/fake_file.c b/source3/smbd/fake_file.c index 92ea14a76da..5d669bfe8e5 100644 --- a/source3/smbd/fake_file.c +++ b/source3/smbd/fake_file.c @@ -206,6 +206,8 @@ NTSTATUS open_fake_file(struct smb_request *req, connection_struct *conn, NTSTATUS close_fake_file(struct smb_request *req, files_struct *fsp) { - file_free(req, fsp); + /* + * Nothing to do, fake files don't hold any resources + */ return NT_STATUS_OK; } -- 2.32.0 From 045de6cdfec95c660d333798d6cadac75a70af6c Mon Sep 17 00:00:00 2001 From: Volker Lendecke Date: Tue, 1 Feb 2022 17:21:24 +0100 Subject: [PATCH 05/14] smbd: Call file_free() just once in close_file() BUG: https://bugzilla.samba.org/show_bug.cgi?id=14975 Signed-off-by: Volker Lendecke Reviewed-by: Jeremy Allison (cherry picked from commit 363ac7533895fda786f56c4fe8346128753f38a5) --- source3/smbd/close.c | 7 ++----- 1 file changed, 2 insertions(+), 5 deletions(-) diff --git a/source3/smbd/close.c b/source3/smbd/close.c index dd4a4c3595f..af739abc4a3 100644 --- a/source3/smbd/close.c +++ b/source3/smbd/close.c @@ -1532,12 +1532,10 @@ NTSTATUS close_file(struct smb_request *req, files_struct *fsp, if (fsp->fake_file_handle != NULL) { status = close_fake_file(req, fsp); - file_free(req, fsp); } else if (fsp->print_file != NULL) { /* FIXME: return spool errors */ print_spool_end(fsp, close_type); fd_close(fsp); - file_free(req, fsp); status = NT_STATUS_OK; } else if (!fsp->fsp_flags.is_fsa) { if (close_type == NORMAL_CLOSE) { @@ -1550,16 +1548,15 @@ NTSTATUS close_file(struct smb_request *req, files_struct *fsp, } SMB_ASSERT(close_type != NORMAL_CLOSE); fd_close(fsp); - file_free(req, fsp); status = NT_STATUS_OK; } else if (fsp->fsp_flags.is_directory) { status = close_directory(req, fsp, close_type); - file_free(req, fsp); } else { status = close_normal_file(req, fsp, close_type); - file_free(req, fsp); } + file_free(req, fsp); + if (close_base_fsp) { /* -- 2.32.0 From bd2d1faf38872382ef8cb3e064d4cacace7cab7a Mon Sep 17 00:00:00 2001 From: Volker Lendecke Date: Tue, 1 Feb 2022 17:47:29 +0100 Subject: [PATCH 06/14] smbd: NULL out "fsp" in close_file() Quite a few places already had this in the caller, but not all. Rename close_file() to close_file_free() appropriately. We'll factor out close_file_smb() doing only parts of close_file_free() later. BUG: https://bugzilla.samba.org/show_bug.cgi?id=14975 Signed-off-by: Volker Lendecke Reviewed-by: Jeremy Allison (cherry picked from commit f5bc73a2ad97647f76143f7962c964f45aa6b1a0) --- source3/lib/adouble.c | 20 ++++---- source3/modules/vfs_fruit.c | 12 ++--- source3/modules/vfs_worm.c | 2 +- source3/printing/nt_printing.c | 10 ++-- source3/rpc_server/srvsvc/srv_srvsvc_nt.c | 4 +- source3/smbd/close.c | 12 +++-- source3/smbd/dir.c | 9 ++-- source3/smbd/files.c | 4 +- source3/smbd/nttrans.c | 6 +-- source3/smbd/open.c | 13 ++--- source3/smbd/proto.h | 5 +- source3/smbd/reply.c | 62 ++++++++++------------- source3/smbd/smb2_close.c | 2 +- source3/smbd/smb2_create.c | 3 +- source3/smbd/trans2.c | 48 ++++++++---------- source3/utils/net_vfs.c | 7 ++- 16 files changed, 103 insertions(+), 116 deletions(-) diff --git a/source3/lib/adouble.c b/source3/lib/adouble.c index 37fb686f17b..aa78007dadd 100644 --- a/source3/lib/adouble.c +++ b/source3/lib/adouble.c @@ -1254,13 +1254,13 @@ static bool ad_convert_xattr(vfs_handle_struct *handle, if (nwritten == -1) { DBG_ERR("SMB_VFS_PWRITE failed\n"); saved_errno = errno; - close_file(NULL, fsp, ERROR_CLOSE); + close_file_free(NULL, &fsp, ERROR_CLOSE); errno = saved_errno; ok = false; goto fail; } - status = close_file(NULL, fsp, NORMAL_CLOSE); + status = close_file_free(NULL, &fsp, NORMAL_CLOSE); if (!NT_STATUS_IS_OK(status)) { ok = false; goto fail; @@ -1395,12 +1395,12 @@ static bool ad_convert_finderinfo(vfs_handle_struct *handle, if (nwritten == -1) { DBG_ERR("SMB_VFS_PWRITE failed\n"); saved_errno = errno; - close_file(NULL, fsp, ERROR_CLOSE); + close_file_free(NULL, &fsp, ERROR_CLOSE); errno = saved_errno; return false; } - status = close_file(NULL, fsp, NORMAL_CLOSE); + status = close_file_free(NULL, &fsp, NORMAL_CLOSE); if (!NT_STATUS_IS_OK(status)) { return false; } @@ -1652,7 +1652,7 @@ static bool ad_unconvert_open_ad(TALLOC_CTX *mem_ctx, if (ret != 0) { DBG_ERR("SMB_VFS_FCHOWN [%s] failed: %s\n", fsp_str_dbg(fsp), nt_errstr(status)); - close_file(NULL, fsp, NORMAL_CLOSE); + close_file_free(NULL, &fsp, NORMAL_CLOSE); return false; } } @@ -1710,14 +1710,14 @@ static bool ad_unconvert_get_streams(struct vfs_handle_struct *handle, num_streams, streams); if (!NT_STATUS_IS_OK(status)) { - close_file(NULL, fsp, NORMAL_CLOSE); + close_file_free(NULL, &fsp, NORMAL_CLOSE); DBG_ERR("streaminfo on [%s] failed: %s\n", smb_fname_str_dbg(smb_fname), nt_errstr(status)); return false; } - status = close_file(NULL, fsp, NORMAL_CLOSE); + status = close_file_free(NULL, &fsp, NORMAL_CLOSE); if (!NT_STATUS_IS_OK(status)) { DBG_ERR("close_file [%s] failed: %s\n", smb_fname_str_dbg(smb_fname), @@ -1975,7 +1975,7 @@ static bool ad_collect_one_stream(struct vfs_handle_struct *handle, out: TALLOC_FREE(sname); if (fsp != NULL) { - status = close_file(NULL, fsp, NORMAL_CLOSE); + status = close_file_free(NULL, &fsp, NORMAL_CLOSE); if (!NT_STATUS_IS_OK(status)) { DBG_ERR("close_file [%s] failed: %s\n", smb_fname_str_dbg(smb_fname), @@ -2117,9 +2117,9 @@ bool ad_unconvert(TALLOC_CTX *mem_ctx, out: if (fsp != NULL) { - status = close_file(NULL, fsp, NORMAL_CLOSE); + status = close_file_free(NULL, &fsp, NORMAL_CLOSE); if (!NT_STATUS_IS_OK(status)) { - DBG_ERR("close_file [%s] failed: %s\n", + DBG_ERR("close_file_free() [%s] failed: %s\n", smb_fname_str_dbg(smb_fname), nt_errstr(status)); ok = false; diff --git a/source3/modules/vfs_fruit.c b/source3/modules/vfs_fruit.c index d6aa7e3644e..303df41258e 100644 --- a/source3/modules/vfs_fruit.c +++ b/source3/modules/vfs_fruit.c @@ -1002,7 +1002,7 @@ static bool readdir_attr_meta_finderi_stream( fail: if (fsp != NULL) { - close_file(NULL, fsp, NORMAL_CLOSE); + close_file_free(NULL, &fsp, NORMAL_CLOSE); } return ok; @@ -4247,8 +4247,8 @@ fail: DEBUG(10, ("fruit_create_file: %s\n", nt_errstr(status))); if (fsp) { - close_file(req, fsp, ERROR_CLOSE); - *result = fsp = NULL; + close_file_free(req, &fsp, ERROR_CLOSE); + *result = NULL; } return status; @@ -4993,8 +4993,7 @@ static bool fruit_get_bandsize(vfs_handle_struct *handle, } - status = close_file(NULL, fsp, NORMAL_CLOSE); - fsp = NULL; + status = close_file_free(NULL, &fsp, NORMAL_CLOSE); if (!NT_STATUS_IS_OK(status)) { DBG_ERR("close_file failed: %s\n", nt_errstr(status)); ok = false; @@ -5028,11 +5027,10 @@ static bool fruit_get_bandsize(vfs_handle_struct *handle, out: if (fsp != NULL) { - status = close_file(NULL, fsp, NORMAL_CLOSE); + status = close_file_free(NULL, &fsp, NORMAL_CLOSE); if (!NT_STATUS_IS_OK(status)) { DBG_ERR("close_file failed: %s\n", nt_errstr(status)); } - fsp = NULL; } TALLOC_FREE(plist); TALLOC_FREE(smb_fname); diff --git a/source3/modules/vfs_worm.c b/source3/modules/vfs_worm.c index 3ae1f9f39e6..76762e0a84f 100644 --- a/source3/modules/vfs_worm.c +++ b/source3/modules/vfs_worm.c @@ -75,7 +75,7 @@ static NTSTATUS vfs_worm_create_file(vfs_handle_struct *handle, * Access via MAXIMUM_ALLOWED_ACCESS? */ if (readonly && ((*result)->access_mask & write_access_flags)) { - close_file(req, *result, NORMAL_CLOSE); + close_file_free(req, result, NORMAL_CLOSE); return NT_STATUS_ACCESS_DENIED; } return NT_STATUS_OK; diff --git a/source3/printing/nt_printing.c b/source3/printing/nt_printing.c index a47afda4a84..1e35e017fb2 100644 --- a/source3/printing/nt_printing.c +++ b/source3/printing/nt_printing.c @@ -874,8 +874,7 @@ static int file_version_is_newer(connection_struct *conn, fstring new_file, fstr (long)old_create_time)); } - close_file(NULL, fsp, NORMAL_CLOSE); - fsp = NULL; + close_file_free(NULL, &fsp, NORMAL_CLOSE); /* Get file version info (if available) for new file */ status = driver_unix_convert(conn, new_file, &smb_fname); @@ -935,8 +934,7 @@ static int file_version_is_newer(connection_struct *conn, fstring new_file, fstr (long)new_create_time)); } - close_file(NULL, fsp, NORMAL_CLOSE); - fsp = NULL; + close_file_free(NULL, &fsp, NORMAL_CLOSE); if (use_version && (new_major != old_major || new_minor != old_minor)) { /* Compare versions and choose the larger version number */ @@ -969,7 +967,7 @@ static int file_version_is_newer(connection_struct *conn, fstring new_file, fstr error_exit: if(fsp) - close_file(NULL, fsp, NORMAL_CLOSE); + close_file_free(NULL, &fsp, NORMAL_CLOSE); ret = -1; done: TALLOC_FREE(smb_fname); @@ -1177,7 +1175,7 @@ static uint32_t get_correct_cversion(const struct auth_session_info *session_inf unbecome_user_without_service(); error_free_conn: if (fsp != NULL) { - close_file(NULL, fsp, NORMAL_CLOSE); + close_file_free(NULL, &fsp, NORMAL_CLOSE); } if (!W_ERROR_IS_OK(*perr)) { cversion = -1; diff --git a/source3/rpc_server/srvsvc/srv_srvsvc_nt.c b/source3/rpc_server/srvsvc/srv_srvsvc_nt.c index 770e5d368a8..ea296eaa6ab 100644 --- a/source3/rpc_server/srvsvc/srv_srvsvc_nt.c +++ b/source3/rpc_server/srvsvc/srv_srvsvc_nt.c @@ -2539,7 +2539,7 @@ WERROR _srvsvc_NetGetFileSecurity(struct pipes_struct *p, error_exit: if (fsp) { - close_file(NULL, fsp, NORMAL_CLOSE); + close_file_free(NULL, &fsp, NORMAL_CLOSE); } TALLOC_FREE(frame); @@ -2659,7 +2659,7 @@ WERROR _srvsvc_NetSetFileSecurity(struct pipes_struct *p, error_exit: if (fsp) { - close_file(NULL, fsp, NORMAL_CLOSE); + close_file_free(NULL, &fsp, NORMAL_CLOSE); } TALLOC_FREE(frame); diff --git a/source3/smbd/close.c b/source3/smbd/close.c index af739abc4a3..9160266c6fb 100644 --- a/source3/smbd/close.c +++ b/source3/smbd/close.c @@ -1478,10 +1478,12 @@ static NTSTATUS close_directory(struct smb_request *req, files_struct *fsp, Close a files_struct. ****************************************************************************/ -NTSTATUS close_file(struct smb_request *req, files_struct *fsp, - enum file_close_type close_type) +NTSTATUS close_file_free(struct smb_request *req, + struct files_struct **_fsp, + enum file_close_type close_type) { NTSTATUS status; + struct files_struct *fsp = *_fsp; struct files_struct *base_fsp = fsp->base_fsp; bool close_base_fsp = false; @@ -1569,9 +1571,11 @@ NTSTATUS close_file(struct smb_request *req, files_struct *fsp, * those loops will become confused. */ - close_file(req, base_fsp, close_type); + close_file_free(req, &base_fsp, close_type); } + *_fsp = NULL; + return status; } @@ -1610,5 +1614,5 @@ void msg_close_file(struct messaging_context *msg_ctx, DEBUG(10,("msg_close_file: failed to find file.\n")); return; } - close_file(NULL, fsp, NORMAL_CLOSE); + close_file_free(NULL, &fsp, NORMAL_CLOSE); } diff --git a/source3/smbd/dir.c b/source3/smbd/dir.c index 8e5ce66c961..581ce5202ed 100644 --- a/source3/smbd/dir.c +++ b/source3/smbd/dir.c @@ -185,9 +185,12 @@ void dptr_closecnum(connection_struct *conn) for(dptr = sconn->searches.dirptrs; dptr; dptr = next) { next = dptr->next; if (dptr->conn == conn) { - files_struct *fsp = dptr->dir_hnd->fsp; - close_file(NULL, fsp, NORMAL_CLOSE); - fsp = NULL; + /* + * Need to make a copy, "dptr" will be gone + * after close_file_free() returns + */ + struct files_struct *fsp = dptr->dir_hnd->fsp; + close_file_free(NULL, &fsp, NORMAL_CLOSE); } } } diff --git a/source3/smbd/files.c b/source3/smbd/files.c index 36d4497b3d8..50bb48b0d05 100644 --- a/source3/smbd/files.c +++ b/source3/smbd/files.c @@ -771,7 +771,7 @@ void file_close_conn(connection_struct *conn) */ fsp->op->global->durable = false; } - close_file(NULL, fsp, SHUTDOWN_CLOSE); + close_file_free(NULL, &fsp, SHUTDOWN_CLOSE); } } @@ -841,7 +841,7 @@ void file_close_user(struct smbd_server_connection *sconn, uint64_t vuid) for (fsp=sconn->files; fsp; fsp=next) { next=fsp->next; if (fsp->vuid == vuid) { - close_file(NULL, fsp, SHUTDOWN_CLOSE); + close_file_free(NULL, &fsp, SHUTDOWN_CLOSE); } } } diff --git a/source3/smbd/nttrans.c b/source3/smbd/nttrans.c index 6db17b8c685..bafe0d77806 100644 --- a/source3/smbd/nttrans.c +++ b/source3/smbd/nttrans.c @@ -1649,7 +1649,7 @@ NTSTATUS copy_internals(TALLOC_CTX *ctx, NULL, NULL); /* create context */ if (!NT_STATUS_IS_OK(status)) { - close_file(NULL, fsp1, ERROR_CLOSE); + close_file_free(NULL, &fsp1, ERROR_CLOSE); goto out; } @@ -1663,12 +1663,12 @@ NTSTATUS copy_internals(TALLOC_CTX *ctx, * Thus we don't look at the error return from the * close of fsp1. */ - close_file(NULL, fsp1, NORMAL_CLOSE); + close_file_free(NULL, &fsp1, NORMAL_CLOSE); /* Ensure the modtime is set correctly on the destination file. */ set_close_write_time(fsp2, smb_fname_src->st.st_ex_mtime); - status = close_file(NULL, fsp2, NORMAL_CLOSE); + status = close_file_free(NULL, &fsp2, NORMAL_CLOSE); /* Grrr. We have to do this as open_file_ntcreate adds FILE_ATTRIBUTE_ARCHIVE when it creates the file. This isn't the correct thing to do in the copy diff --git a/source3/smbd/open.c b/source3/smbd/open.c index 6167fbc5775..5d8251dcef5 100644 --- a/source3/smbd/open.c +++ b/source3/smbd/open.c @@ -4760,7 +4760,7 @@ NTSTATUS create_directory(connection_struct *conn, struct smb_request *req, NULL, NULL); /* create context */ if (NT_STATUS_IS_OK(status)) { - close_file(req, fsp, NORMAL_CLOSE); + close_file_free(req, &fsp, NORMAL_CLOSE); } return status; @@ -5010,7 +5010,7 @@ static NTSTATUS open_streams_for_delete(connection_struct *conn, DEBUG(10, ("Closing stream # %d, %s\n", j, fsp_str_dbg(streams[j]))); - close_file(NULL, streams[j], NORMAL_CLOSE); + close_file_free(NULL, &streams[j], NORMAL_CLOSE); } fail: @@ -6053,12 +6053,10 @@ static NTSTATUS create_file_unixpath(connection_struct *conn, * fsp->base_fsp. */ base_fsp = NULL; - close_file(req, fsp, ERROR_CLOSE); - fsp = NULL; + close_file_free(req, &fsp, ERROR_CLOSE); } if (base_fsp != NULL) { - close_file(req, base_fsp, ERROR_CLOSE); - base_fsp = NULL; + close_file_free(req, &base_fsp, ERROR_CLOSE); } TALLOC_FREE(parent_dir_fname); @@ -6236,8 +6234,7 @@ NTSTATUS create_file_default(connection_struct *conn, DEBUG(10, ("create_file: %s\n", nt_errstr(status))); if (fsp != NULL) { - close_file(req, fsp, ERROR_CLOSE); - fsp = NULL; + close_file_free(req, &fsp, ERROR_CLOSE); } return status; } diff --git a/source3/smbd/proto.h b/source3/smbd/proto.h index c802b05a78f..67a4edb4f88 100644 --- a/source3/smbd/proto.h +++ b/source3/smbd/proto.h @@ -130,8 +130,9 @@ bool smbd_smb1_brl_finish_by_mid( /* The following definitions come from smbd/close.c */ void set_close_write_time(struct files_struct *fsp, struct timespec ts); -NTSTATUS close_file(struct smb_request *req, files_struct *fsp, - enum file_close_type close_type); +NTSTATUS close_file_free(struct smb_request *req, + struct files_struct **_fsp, + enum file_close_type close_type); void msg_close_file(struct messaging_context *msg_ctx, void *private_data, uint32_t msg_type, diff --git a/source3/smbd/reply.c b/source3/smbd/reply.c index ffe6ec45153..50730baecc7 100644 --- a/source3/smbd/reply.c +++ b/source3/smbd/reply.c @@ -1821,8 +1821,7 @@ void reply_search(struct smb_request *req) * as this is not a client visible handle so * can'tbe part of an SMB1 chain. */ - close_file(NULL, fsp, NORMAL_CLOSE); - fsp = NULL; + close_file_free(NULL, &fsp, NORMAL_CLOSE); reply_nterror(req, nt_status); goto out; } @@ -1953,15 +1952,13 @@ void reply_search(struct smb_request *req) if (numentries == 0) { dptr_num = -1; if (fsp != NULL) { - close_file(NULL, fsp, NORMAL_CLOSE); - fsp = NULL; + close_file_free(NULL, &fsp, NORMAL_CLOSE); } } else if(expect_close && status_len == 0) { /* Close the dptr - we know it's gone */ dptr_num = -1; if (fsp != NULL) { - close_file(NULL, fsp, NORMAL_CLOSE); - fsp = NULL; + close_file_free(NULL, &fsp, NORMAL_CLOSE); } } @@ -1970,8 +1967,7 @@ void reply_search(struct smb_request *req) dptr_num = -1; /* fsp may have been closed above. */ if (fsp != NULL) { - close_file(NULL, fsp, NORMAL_CLOSE); - fsp = NULL; + close_file_free(NULL, &fsp, NORMAL_CLOSE); } } @@ -2073,8 +2069,7 @@ void reply_fclose(struct smb_request *req) fsp = dptr_fetch_fsp(sconn, status+12,&dptr_num); if(fsp != NULL) { /* Close the file - we know it's gone */ - close_file(NULL, fsp, NORMAL_CLOSE); - fsp = NULL; + close_file_free(NULL, &fsp, NORMAL_CLOSE); dptr_num = -1; } @@ -2216,7 +2211,7 @@ void reply_open(struct smb_request *req) if (fattr & FILE_ATTRIBUTE_DIRECTORY) { DEBUG(3,("attempt to open a directory %s\n", fsp_str_dbg(fsp))); - close_file(req, fsp, ERROR_CLOSE); + close_file_free(req, &fsp, ERROR_CLOSE); reply_botherror(req, NT_STATUS_ACCESS_DENIED, ERRDOS, ERRnoaccess); goto out; @@ -2401,19 +2396,19 @@ void reply_open_and_X(struct smb_request *req) if (((smb_action == FILE_WAS_CREATED) || (smb_action == FILE_WAS_OVERWRITTEN)) && allocation_size) { fsp->initial_allocation_size = smb_roundup(fsp->conn, allocation_size); if (vfs_allocate_file_space(fsp, fsp->initial_allocation_size) == -1) { - close_file(req, fsp, ERROR_CLOSE); + close_file_free(req, &fsp, ERROR_CLOSE); reply_nterror(req, NT_STATUS_DISK_FULL); goto out; } retval = vfs_set_filelen(fsp, (off_t)allocation_size); if (retval < 0) { - close_file(req, fsp, ERROR_CLOSE); + close_file_free(req, &fsp, ERROR_CLOSE); reply_nterror(req, NT_STATUS_DISK_FULL); goto out; } status = vfs_stat_fsp(fsp); if (!NT_STATUS_IS_OK(status)) { - close_file(req, fsp, ERROR_CLOSE); + close_file_free(req, &fsp, ERROR_CLOSE); reply_nterror(req, status); goto out; } @@ -2421,7 +2416,7 @@ void reply_open_and_X(struct smb_request *req) fattr = fdos_mode(fsp); if (fattr & FILE_ATTRIBUTE_DIRECTORY) { - close_file(req, fsp, ERROR_CLOSE); + close_file_free(req, &fsp, ERROR_CLOSE); reply_nterror(req, NT_STATUS_ACCESS_DENIED); goto out; } @@ -3189,7 +3184,7 @@ NTSTATUS unlink_internals(connection_struct *conn, "(%s)\n", smb_fname_str_dbg(smb_fname), nt_errstr(status)); - close_file(req, fsp, NORMAL_CLOSE); + close_file_free(req, &fsp, NORMAL_CLOSE); return status; } @@ -3197,11 +3192,11 @@ NTSTATUS unlink_internals(connection_struct *conn, if (!set_delete_on_close(fsp, True, conn->session_info->security_token, conn->session_info->unix_token)) { - close_file(req, fsp, NORMAL_CLOSE); + close_file_free(req, &fsp, NORMAL_CLOSE); return NT_STATUS_ACCESS_DENIED; } - return close_file(req, fsp, NORMAL_CLOSE); + return close_file_free(req, &fsp, NORMAL_CLOSE); } /**************************************************************************** @@ -5661,7 +5656,7 @@ static void reply_exit_done(struct tevent_req *req) smb_request_done(smb1req); END_PROFILE(SMBexit); } - close_file(NULL, fsp, SHUTDOWN_CLOSE); + close_file_free(NULL, &fsp, SHUTDOWN_CLOSE); } reply_outbuf(smb1req, 0, 0); @@ -5737,12 +5732,12 @@ void reply_close(struct smb_request *smb1req) } /* - * close_file() returns the unix errno if an error was detected on + * close_file_free() returns the unix errno if an error was detected on * close - normally this is due to a disk full error. If not then it * was probably an I/O error. */ - status = close_file(smb1req, fsp, NORMAL_CLOSE); + status = close_file_free(smb1req, &fsp, NORMAL_CLOSE); done: if (!NT_STATUS_IS_OK(status)) { reply_nterror(smb1req, status); @@ -5868,7 +5863,7 @@ static void reply_close_done(struct tevent_req *req) return; } - status = close_file(smb1req, state->fsp, NORMAL_CLOSE); + status = close_file_free(smb1req, &state->fsp, NORMAL_CLOSE); if (NT_STATUS_IS_OK(status)) { reply_outbuf(smb1req, 0, 0); } else { @@ -5967,8 +5962,7 @@ void reply_writeclose(struct smb_request *req) if (numtowrite) { DEBUG(3,("reply_writeclose: zero length write doesn't close " "file %s\n", fsp_str_dbg(fsp))); - close_status = close_file(req, fsp, NORMAL_CLOSE); - fsp = NULL; + close_status = close_file_free(req, &fsp, NORMAL_CLOSE); } if(((nwritten == 0) && (numtowrite != 0))||(nwritten < 0)) { @@ -6516,7 +6510,7 @@ void reply_printclose(struct smb_request *req) DEBUG(3,("printclose fd=%d %s\n", fsp_get_io_fd(fsp), fsp_fnum_dbg(fsp))); - status = close_file(req, fsp, NORMAL_CLOSE); + status = close_file_free(req, &fsp, NORMAL_CLOSE); if(!NT_STATUS_IS_OK(status)) { reply_nterror(req, status); @@ -6919,7 +6913,7 @@ void reply_rmdir(struct smb_request *req) status = can_set_delete_on_close(fsp, FILE_ATTRIBUTE_DIRECTORY); if (!NT_STATUS_IS_OK(status)) { - close_file(req, fsp, ERROR_CLOSE); + close_file_free(req, &fsp, ERROR_CLOSE); reply_nterror(req, status); goto out; } @@ -6927,12 +6921,12 @@ void reply_rmdir(struct smb_request *req) if (!set_delete_on_close(fsp, true, conn->session_info->security_token, conn->session_info->unix_token)) { - close_file(req, fsp, ERROR_CLOSE); + close_file_free(req, &fsp, ERROR_CLOSE); reply_nterror(req, NT_STATUS_ACCESS_DENIED); goto out; } - status = close_file(req, fsp, NORMAL_CLOSE); + status = close_file_free(req, &fsp, NORMAL_CLOSE); if (!NT_STATUS_IS_OK(status)) { reply_nterror(req, status); } else { @@ -7680,7 +7674,7 @@ NTSTATUS rename_internals(TALLOC_CTX *ctx, attrs, replace_if_exists); - close_file(req, fsp, NORMAL_CLOSE); + close_file_free(req, &fsp, NORMAL_CLOSE); DBG_NOTICE("Error %s rename %s -> %s\n", nt_errstr(status), smb_fname_str_dbg(smb_fname_src), @@ -7974,7 +7968,7 @@ NTSTATUS copy_file(TALLOC_CTX *ctx, NULL, NULL); /* create context */ if (!NT_STATUS_IS_OK(status)) { - close_file(NULL, fsp1, ERROR_CLOSE); + close_file_free(NULL, &fsp1, ERROR_CLOSE); goto out; } @@ -7984,8 +7978,8 @@ NTSTATUS copy_file(TALLOC_CTX *ctx, DEBUG(0, ("error - vfs lseek returned error %s\n", strerror(errno))); status = map_nt_error_from_unix(errno); - close_file(NULL, fsp1, ERROR_CLOSE); - close_file(NULL, fsp2, ERROR_CLOSE); + close_file_free(NULL, &fsp1, ERROR_CLOSE); + close_file_free(NULL, &fsp2, ERROR_CLOSE); goto out; } } @@ -7997,7 +7991,7 @@ NTSTATUS copy_file(TALLOC_CTX *ctx, ret = 0; } - close_file(NULL, fsp1, NORMAL_CLOSE); + close_file_free(NULL, &fsp1, NORMAL_CLOSE); /* Ensure the modtime is set correctly on the destination file. */ set_close_write_time(fsp2, smb_fname_src->st.st_ex_mtime); @@ -8008,7 +8002,7 @@ NTSTATUS copy_file(TALLOC_CTX *ctx, * Thus we don't look at the error return from the * close of fsp1. */ - status = close_file(NULL, fsp2, NORMAL_CLOSE); + status = close_file_free(NULL, &fsp2, NORMAL_CLOSE); if (!NT_STATUS_IS_OK(status)) { goto out; diff --git a/source3/smbd/smb2_close.c b/source3/smbd/smb2_close.c index 648080f1a8c..b434d696c3f 100644 --- a/source3/smbd/smb2_close.c +++ b/source3/smbd/smb2_close.c @@ -260,7 +260,7 @@ static NTSTATUS smbd_smb2_close(struct smbd_smb2_request *req, &dos_attrs); } - status = close_file(smbreq, fsp, NORMAL_CLOSE); + status = close_file_free(smbreq, &fsp, NORMAL_CLOSE); if (!NT_STATUS_IS_OK(status)) { DEBUG(5,("smbd_smb2_close: close_file[%s]: %s\n", smb_fname_str_dbg(smb_fname), nt_errstr(status))); diff --git a/source3/smbd/smb2_create.c b/source3/smbd/smb2_create.c index ad1ddc9a65e..932ec31a18a 100644 --- a/source3/smbd/smb2_create.c +++ b/source3/smbd/smb2_create.c @@ -915,7 +915,8 @@ static struct tevent_req *smbd_smb2_create_send(TALLOC_CTX *mem_ctx, status = smbd_smb2_create_durable_lease_check( smb1req, state->fname, state->result, state->lease_ptr); if (!NT_STATUS_IS_OK(status)) { - close_file(smb1req, state->result, SHUTDOWN_CLOSE); + close_file_free( + smb1req, &state->result, SHUTDOWN_CLOSE); tevent_req_nterror(req, status); return tevent_req_post(req, state->ev); } diff --git a/source3/smbd/trans2.c b/source3/smbd/trans2.c index 0798243b976..7e075ce21c0 100644 --- a/source3/smbd/trans2.c +++ b/source3/smbd/trans2.c @@ -1396,7 +1396,7 @@ static void call_trans2open(connection_struct *conn, mtime = convert_timespec_to_time_t(smb_fname->st.st_ex_mtime); inode = smb_fname->st.st_ex_ino; if (fattr & FILE_ATTRIBUTE_DIRECTORY) { - close_file(req, fsp, ERROR_CLOSE); + close_file_free(req, &fsp, ERROR_CLOSE); reply_nterror(req, NT_STATUS_ACCESS_DENIED); goto out; } @@ -2896,8 +2896,7 @@ total_data=%u (should be %u)\n", (unsigned int)total_data, (unsigned int)IVAL(pd * as this is not a client visible handle so * can'tbe part of an SMB1 chain. */ - close_file(NULL, fsp, NORMAL_CLOSE); - fsp = NULL; + close_file_free(NULL, &fsp, NORMAL_CLOSE); reply_nterror(req, ntstatus); goto out; } @@ -2987,8 +2986,7 @@ total_data=%u (should be %u)\n", (unsigned int)total_data, (unsigned int)IVAL(pd if(close_after_first || (finished && close_if_end)) { DEBUG(5,("call_trans2findfirst - (2) closing dptr_num %d\n", dptr_num)); dptr_num = -1; - close_file(NULL, fsp, NORMAL_CLOSE); - fsp = NULL; + close_file_free(NULL, &fsp, NORMAL_CLOSE); } /* @@ -3005,8 +3003,7 @@ total_data=%u (should be %u)\n", (unsigned int)total_data, (unsigned int)IVAL(pd * close_after_first or finished case above. */ if (fsp != NULL) { - close_file(NULL, fsp, NORMAL_CLOSE); - fsp = NULL; + close_file_free(NULL, &fsp, NORMAL_CLOSE); } if (get_Protocol() < PROTOCOL_NT1) { reply_force_doserror(req, ERRDOS, ERRnofiles); @@ -3409,8 +3406,7 @@ total_data=%u (should be %u)\n", (unsigned int)total_data, (unsigned int)IVAL(pd if(close_after_request || (finished && close_if_end)) { DEBUG(5,("call_trans2findnext: closing dptr_num = %d\n", dptr_num)); dptr_num = -1; - close_file(NULL, fsp, NORMAL_CLOSE); - fsp = NULL; + close_file_free(NULL, &fsp, NORMAL_CLOSE); } if (as_root) { @@ -5043,8 +5039,7 @@ static NTSTATUS smb_query_posix_acl(connection_struct *conn, * date. Structure copy. */ smb_fname->st = fsp->fsp_name->st; - (void)close_file(req, fsp, NORMAL_CLOSE); - fsp = NULL; + (void)close_file_free(req, &fsp, NORMAL_CLOSE); } TALLOC_FREE(file_acl); @@ -6687,18 +6682,18 @@ static NTSTATUS smb_set_file_size(connection_struct *conn, /* See RAW-SFILEINFO-END-OF-FILE */ if (fail_after_createfile) { - close_file(req, new_fsp,NORMAL_CLOSE); + close_file_free(req, &new_fsp, NORMAL_CLOSE); return NT_STATUS_INVALID_LEVEL; } if (vfs_set_filelen(new_fsp, size) == -1) { status = map_nt_error_from_unix(errno); - close_file(req, new_fsp,NORMAL_CLOSE); + close_file_free(req, &new_fsp, NORMAL_CLOSE); return status; } trigger_write_time_update_immediate(new_fsp); - close_file(req, new_fsp,NORMAL_CLOSE); + close_file_free(req, &new_fsp, NORMAL_CLOSE); return NT_STATUS_OK; } @@ -7583,8 +7578,7 @@ static NTSTATUS smb_set_posix_acl(connection_struct *conn, out: if (close_fsp) { - (void)close_file(req, fsp, NORMAL_CLOSE); - fsp = NULL; + (void)close_file_free(req, &fsp, NORMAL_CLOSE); } return status; } @@ -7960,7 +7954,7 @@ static NTSTATUS smb_set_file_allocation_info(connection_struct *conn, if (allocation_size != get_file_size_stat(&smb_fname->st)) { if (vfs_allocate_file_space(new_fsp, allocation_size) == -1) { status = map_nt_error_from_unix(errno); - close_file(req, new_fsp, NORMAL_CLOSE); + close_file_free(req, &new_fsp, NORMAL_CLOSE); return status; } } @@ -7971,7 +7965,7 @@ static NTSTATUS smb_set_file_allocation_info(connection_struct *conn, * if there are no pending writes. */ trigger_write_time_update_immediate(new_fsp); - close_file(req, new_fsp, NORMAL_CLOSE); + close_file_free(req, &new_fsp, NORMAL_CLOSE); return NT_STATUS_OK; } @@ -8476,7 +8470,7 @@ static NTSTATUS smb_posix_mkdir(connection_struct *conn, TALLOC_FREE(posx); if (NT_STATUS_IS_OK(status)) { - close_file(req, fsp, NORMAL_CLOSE); + close_file_free(req, &fsp, NORMAL_CLOSE); } info_level_return = SVAL(pdata,16); @@ -8744,7 +8738,7 @@ static NTSTATUS smb_posix_open(connection_struct *conn, /* Realloc the data size */ *ppdata = (char *)SMB_REALLOC(*ppdata,*pdata_return_size); if (*ppdata == NULL) { - close_file(req, fsp, ERROR_CLOSE); + close_file_free(req, &fsp, ERROR_CLOSE); *pdata_return_size = 0; return NT_STATUS_NO_MEMORY; } @@ -8872,7 +8866,7 @@ static NTSTATUS smb_posix_unlink(connection_struct *conn, if (lck == NULL) { DEBUG(0, ("smb_posix_unlink: Could not get share mode " "lock for file %s\n", fsp_str_dbg(fsp))); - close_file(req, fsp, NORMAL_CLOSE); + close_file_free(req, &fsp, NORMAL_CLOSE); return NT_STATUS_INVALID_PARAMETER; } @@ -8880,7 +8874,7 @@ static NTSTATUS smb_posix_unlink(connection_struct *conn, if (other_nonposix_opens) { /* Fail with sharing violation. */ TALLOC_FREE(lck); - close_file(req, fsp, NORMAL_CLOSE); + close_file_free(req, &fsp, NORMAL_CLOSE); return NT_STATUS_SHARING_VIOLATION; } @@ -8896,10 +8890,10 @@ static NTSTATUS smb_posix_unlink(connection_struct *conn, TALLOC_FREE(lck); if (!NT_STATUS_IS_OK(status)) { - close_file(req, fsp, NORMAL_CLOSE); + close_file_free(req, &fsp, NORMAL_CLOSE); return status; } - return close_file(req, fsp, NORMAL_CLOSE); + return close_file_free(req, &fsp, NORMAL_CLOSE); } static NTSTATUS smbd_do_posix_setfilepathinfo(struct connection_struct *conn, @@ -9666,8 +9660,7 @@ static void call_trans2mkdir(connection_struct *conn, struct smb_request *req, out: if (fsp != NULL) { - close_file(NULL, fsp, NORMAL_CLOSE); - fsp = NULL; + close_file_free(NULL, &fsp, NORMAL_CLOSE); } TALLOC_FREE(smb_dname); return; @@ -9904,8 +9897,7 @@ void reply_findclose(struct smb_request *req) fsp = dptr_fetch_lanman2_fsp(sconn, dptr_num); dptr_num = -1; if (fsp != NULL) { - close_file(NULL, fsp, NORMAL_CLOSE); - fsp = NULL; + close_file_free(NULL, &fsp, NORMAL_CLOSE); } } diff --git a/source3/utils/net_vfs.c b/source3/utils/net_vfs.c index 73eda3e5cda..53e4583af39 100644 --- a/source3/utils/net_vfs.c +++ b/source3/utils/net_vfs.c @@ -283,23 +283,22 @@ static int net_vfs_get_ntacl(struct net_context *net, goto done; } - status = close_file(NULL, fsp, NORMAL_CLOSE); + status = close_file_free(NULL, &fsp, NORMAL_CLOSE); if (!NT_STATUS_IS_OK(status)) { DBG_ERR("close_file [%s] failed: %s\n", smb_fname_str_dbg(smb_fname), nt_errstr(status)); goto done; } - fsp = NULL; sec_desc_print(NULL, stdout, sd, true); rc = 0; done: if (fsp != NULL) { - status = close_file(NULL, fsp, NORMAL_CLOSE); + status = close_file_free(NULL, &fsp, NORMAL_CLOSE); if (!NT_STATUS_IS_OK(status)) { - DBG_ERR("close_file [%s] failed: %s\n", + DBG_ERR("close_file_free() [%s] failed: %s\n", smb_fname_str_dbg(smb_fname), nt_errstr(status)); rc = 1; -- 2.32.0 From 6ec389df42ca93bb30c075fe9d4023b16b24d4f4 Mon Sep 17 00:00:00 2001 From: Volker Lendecke Date: Wed, 2 Feb 2022 08:58:15 +0100 Subject: [PATCH 07/14] smbd: No base fsps to close_file_free() from file_close_conn() close_file_free() needs to handle base fsps specially. This can be simplified a lot if we pass the the open files a second time in case we encountered base_fsps that we could not immediately delete. file_close_conn() is not our hot code path, and also we don't expect many thousand open files that we need to walk a second time. A subsequent patch will simplify close_file_free(), the complicated logic is now in files.c, where it IMHO belongs because file_set_base_fsp() are here as well. BUG: https://bugzilla.samba.org/show_bug.cgi?id=14975 Signed-off-by: Volker Lendecke Reviewed-by: Jeremy Allison (cherry picked from commit d1341d666af12965b4318f89b1d0e1e8769e861e) --- source3/smbd/files.c | 87 +++++++++++++++++++++++++++++++++++++------- 1 file changed, 74 insertions(+), 13 deletions(-) diff --git a/source3/smbd/files.c b/source3/smbd/files.c index 50bb48b0d05..7a2123f3fae 100644 --- a/source3/smbd/files.c +++ b/source3/smbd/files.c @@ -756,22 +756,83 @@ NTSTATUS parent_pathref(TALLOC_CTX *mem_ctx, Close all open files for a connection. ****************************************************************************/ -void file_close_conn(connection_struct *conn) +struct file_close_conn_state { + struct connection_struct *conn; + bool fsp_left_behind; +}; + +static struct files_struct *file_close_conn_fn( + struct files_struct *fsp, + void *private_data) { - files_struct *fsp, *next; + struct file_close_conn_state *state = private_data; + + if (fsp->conn != state->conn) { + return NULL; + } + + if (fsp->op != NULL && fsp->op->global->durable) { + /* + * A tree disconnect closes a durable handle + */ + fsp->op->global->durable = false; + } + + if (fsp->base_fsp != NULL) { + /* + * This is a stream, it can't be a base + */ + SMB_ASSERT(fsp->stream_fsp == NULL); + SMB_ASSERT(fsp->base_fsp->stream_fsp == fsp); + + /* + * Remove the base<->stream link so that + * close_file_free() does not close fsp->base_fsp as + * well. This would destroy walking the linked list of + * fsps. + */ + fsp->base_fsp->stream_fsp = NULL; + fsp->base_fsp = NULL; - for (fsp=conn->sconn->files; fsp; fsp=next) { - next = fsp->next; - if (fsp->conn != conn) { - continue; - } - if (fsp->op != NULL && fsp->op->global->durable) { - /* - * A tree disconnect closes a durable handle - */ - fsp->op->global->durable = false; - } close_file_free(NULL, &fsp, SHUTDOWN_CLOSE); + return NULL; + } + + if (fsp->stream_fsp != NULL) { + /* + * This is the base of a stream. + */ + SMB_ASSERT(fsp->stream_fsp->base_fsp == fsp); + + /* + * Remove the base<->stream link. This will make fsp + * look like a normal fsp for the next round. + */ + fsp->stream_fsp->base_fsp = NULL; + fsp->stream_fsp = NULL; + + /* + * Have us called back a second time. In the second + * round, "fsp" now looks like a normal fsp. + */ + state->fsp_left_behind = true; + return NULL; + } + + close_file_free(NULL, &fsp, SHUTDOWN_CLOSE); + return NULL; +} + +void file_close_conn(connection_struct *conn) +{ + struct file_close_conn_state state = { .conn = conn }; + + files_forall(conn->sconn, file_close_conn_fn, &state); + + if (state.fsp_left_behind) { + state.fsp_left_behind = false; + files_forall(conn->sconn, file_close_conn_fn, &state); + SMB_ASSERT(!state.fsp_left_behind); } } -- 2.32.0 From d847a0dc32e5469328132b562fcf8dd4d2a8451a Mon Sep 17 00:00:00 2001 From: Volker Lendecke Date: Wed, 2 Feb 2022 12:27:50 +0100 Subject: [PATCH 08/14] smbd: Factor out close_file_in_loop() from file_close_conn_fn() To be reused in file_close_user(). Deliberately a separate commit to make the previous commit easier to understand. BUG: https://bugzilla.samba.org/show_bug.cgi?id=14975 Signed-off-by: Volker Lendecke Reviewed-by: Jeremy Allison (cherry picked from commit 61f57ba24ee2e54abf224118f93bd0ccda44ec41) --- source3/smbd/files.c | 64 +++++++++++++++++++++++++------------------- 1 file changed, 37 insertions(+), 27 deletions(-) diff --git a/source3/smbd/files.c b/source3/smbd/files.c index 7a2123f3fae..1d03fa82076 100644 --- a/source3/smbd/files.c +++ b/source3/smbd/files.c @@ -752,32 +752,8 @@ NTSTATUS parent_pathref(TALLOC_CTX *mem_ctx, return NT_STATUS_OK; } -/**************************************************************************** - Close all open files for a connection. -****************************************************************************/ - -struct file_close_conn_state { - struct connection_struct *conn; - bool fsp_left_behind; -}; - -static struct files_struct *file_close_conn_fn( - struct files_struct *fsp, - void *private_data) +static bool close_file_in_loop(struct files_struct *fsp) { - struct file_close_conn_state *state = private_data; - - if (fsp->conn != state->conn) { - return NULL; - } - - if (fsp->op != NULL && fsp->op->global->durable) { - /* - * A tree disconnect closes a durable handle - */ - fsp->op->global->durable = false; - } - if (fsp->base_fsp != NULL) { /* * This is a stream, it can't be a base @@ -815,11 +791,45 @@ static struct files_struct *file_close_conn_fn( * Have us called back a second time. In the second * round, "fsp" now looks like a normal fsp. */ - state->fsp_left_behind = true; - return NULL; + return false; } close_file_free(NULL, &fsp, SHUTDOWN_CLOSE); + return true; +} + +/**************************************************************************** + Close all open files for a connection. +****************************************************************************/ + +struct file_close_conn_state { + struct connection_struct *conn; + bool fsp_left_behind; +}; + +static struct files_struct *file_close_conn_fn( + struct files_struct *fsp, + void *private_data) +{ + struct file_close_conn_state *state = private_data; + bool did_close; + + if (fsp->conn != state->conn) { + return NULL; + } + + if (fsp->op != NULL && fsp->op->global->durable) { + /* + * A tree disconnect closes a durable handle + */ + fsp->op->global->durable = false; + } + + did_close = close_file_in_loop(fsp); + if (!did_close) { + state->fsp_left_behind = true; + } + return NULL; } -- 2.32.0 From 0eb41618c0b399f75b6e8ce3f5861b751fc7bd0c Mon Sep 17 00:00:00 2001 From: Volker Lendecke Date: Wed, 2 Feb 2022 08:58:15 +0100 Subject: [PATCH 09/14] smbd: No base fsps to close_file_free() from file_close_user() Same logic as the change for file_close_conn() BUG: https://bugzilla.samba.org/show_bug.cgi?id=14975 Signed-off-by: Volker Lendecke Reviewed-by: Jeremy Allison (cherry picked from commit 1fbd9877fead466a17d697c143cd370c0b27f610) --- source3/smbd/files.c | 37 +++++++++++++++++++++++++++++++------ 1 file changed, 31 insertions(+), 6 deletions(-) diff --git a/source3/smbd/files.c b/source3/smbd/files.c index 1d03fa82076..be93d0e884e 100644 --- a/source3/smbd/files.c +++ b/source3/smbd/files.c @@ -905,15 +905,40 @@ bool file_init(struct smbd_server_connection *sconn) Close files open by a specified vuid. ****************************************************************************/ +struct file_close_user_state { + uint64_t vuid; + bool fsp_left_behind; +}; + +static struct files_struct *file_close_user_fn( + struct files_struct *fsp, + void *private_data) +{ + struct file_close_user_state *state = private_data; + bool did_close; + + if (fsp->vuid != state->vuid) { + return NULL; + } + + did_close = close_file_in_loop(fsp); + if (!did_close) { + state->fsp_left_behind = true; + } + + return NULL; +} + void file_close_user(struct smbd_server_connection *sconn, uint64_t vuid) { - files_struct *fsp, *next; + struct file_close_user_state state = { .vuid = vuid }; - for (fsp=sconn->files; fsp; fsp=next) { - next=fsp->next; - if (fsp->vuid == vuid) { - close_file_free(NULL, &fsp, SHUTDOWN_CLOSE); - } + files_forall(sconn, file_close_user_fn, &state); + + if (state.fsp_left_behind) { + state.fsp_left_behind = false; + files_forall(sconn, file_close_user_fn, &state); + SMB_ASSERT(!state.fsp_left_behind); } } -- 2.32.0 From 4fb1d12738661b26cf3f0d6577e569fd568808df Mon Sep 17 00:00:00 2001 From: Volker Lendecke Date: Wed, 2 Feb 2022 12:42:08 +0100 Subject: [PATCH 10/14] smbd: Simplify the flow in close_file_free() We are no longer called on base_fsp's in SHUTDOWN_CLOSE. That simplifies the logic in the common case, we now have a linear flow for the very often-called close_file() BUG: https://bugzilla.samba.org/show_bug.cgi?id=14975 Signed-off-by: Volker Lendecke Reviewed-by: Jeremy Allison (cherry picked from commit 93fe9c83145d31ea11a9cd25049ac527ad4a000d) --- source3/smbd/close.c | 69 ++++++++++++-------------------------------- 1 file changed, 19 insertions(+), 50 deletions(-) diff --git a/source3/smbd/close.c b/source3/smbd/close.c index 9160266c6fb..9e7f89a304f 100644 --- a/source3/smbd/close.c +++ b/source3/smbd/close.c @@ -1484,8 +1484,6 @@ NTSTATUS close_file_free(struct smb_request *req, { NTSTATUS status; struct files_struct *fsp = *_fsp; - struct files_struct *base_fsp = fsp->base_fsp; - bool close_base_fsp = false; /* * This fsp can never be an internal dirfsp. They must @@ -1493,44 +1491,10 @@ NTSTATUS close_file_free(struct smb_request *req, */ SMB_ASSERT(!fsp->fsp_flags.is_dirfsp); - if (fsp->stream_fsp != NULL) { - /* - * fsp is the base for a stream. - * - * We're called with SHUTDOWN_CLOSE from files.c which walks the - * complete list of files. - * - * We need to wait until the stream is closed. - */ - SMB_ASSERT(close_type == SHUTDOWN_CLOSE); - return NT_STATUS_OK; - } - - if (base_fsp != NULL) { - /* - * We need to remove the link in order to - * recurse for the base fsp below. - */ - SMB_ASSERT(base_fsp->base_fsp == NULL); - SMB_ASSERT(base_fsp->stream_fsp == fsp); - base_fsp->stream_fsp = NULL; - - if (close_type == SHUTDOWN_CLOSE) { - /* - * We're called with SHUTDOWN_CLOSE from files.c - * which walks the complete list of files. - * - * We may need to defer the SHUTDOWN_CLOSE - * if it's the next in the linked list. - * - * So we only close if the base is *not* the - * next in the list. - */ - close_base_fsp = (fsp->next != base_fsp); - } else { - close_base_fsp = true; - } - } + /* + * Never call directly on a base fsp + */ + SMB_ASSERT(fsp->stream_fsp == NULL); if (fsp->fake_file_handle != NULL) { status = close_fake_file(req, fsp); @@ -1557,23 +1521,28 @@ NTSTATUS close_file_free(struct smb_request *req, status = close_normal_file(req, fsp, close_type); } - file_free(req, fsp); + if (fsp->base_fsp != NULL) { + /* + * fsp was a stream, its base_fsp can't be a stream + * as well + */ + SMB_ASSERT(fsp->base_fsp->base_fsp == NULL); - if (close_base_fsp) { + /* + * There's a 1:1 relationship between fsp and a base_fsp + */ + SMB_ASSERT(fsp->base_fsp->stream_fsp == fsp); /* - * fsp was a stream, the base fsp can't be a stream as well - * - * For SHUTDOWN_CLOSE this is not possible here - * (if the base_fsp was the next in the linked list), because - * SHUTDOWN_CLOSE only happens from files.c which walks the - * complete list of files. If we mess with more than one fsp - * those loops will become confused. + * Make base_fsp look standalone now */ + fsp->base_fsp->stream_fsp = NULL; - close_file_free(req, &base_fsp, close_type); + close_file_free(req, &fsp->base_fsp, close_type); } + file_free(req, fsp); + *_fsp = NULL; return status; -- 2.32.0 From e8e07e2e4eedcd7ab42718dd9b3a18b27e3a8889 Mon Sep 17 00:00:00 2001 From: Volker Lendecke Date: Thu, 3 Feb 2022 15:25:11 +0100 Subject: [PATCH 11/14] torture: Add a test to show that full_audit uses a ptr after free Run vfstest with this vfstest.cmd under valgrind and you'll see what happens. Exact explanation a few patches further down... BUG: https://bugzilla.samba.org/show_bug.cgi?id=14975 Signed-off-by: Volker Lendecke Reviewed-by: Jeremy Allison (cherry picked from commit 5f1ceead7094aefc6ad1f209468e9ea8f009716c) --- selftest/knownfail.d/full_audit_crash | 1 + .../script/tests/full_audit_segfault/run.sh | 23 +++++ .../tests/full_audit_segfault/vfstest.cmd | 3 + source3/selftest/tests.py | 8 ++ source3/torture/cmd_vfs.c | 85 +++++++++++++++++++ 5 files changed, 120 insertions(+) create mode 100644 selftest/knownfail.d/full_audit_crash create mode 100755 source3/script/tests/full_audit_segfault/run.sh create mode 100644 source3/script/tests/full_audit_segfault/vfstest.cmd diff --git a/selftest/knownfail.d/full_audit_crash b/selftest/knownfail.d/full_audit_crash new file mode 100644 index 00000000000..9154ea334f2 --- /dev/null +++ b/selftest/knownfail.d/full_audit_crash @@ -0,0 +1 @@ +^samba.vfstest.full_audit_segfault.vfstest\(nt4_dc:local\) \ No newline at end of file diff --git a/source3/script/tests/full_audit_segfault/run.sh b/source3/script/tests/full_audit_segfault/run.sh new file mode 100755 index 00000000000..752b27125c8 --- /dev/null +++ b/source3/script/tests/full_audit_segfault/run.sh @@ -0,0 +1,23 @@ +#!/bin/sh +if [ $# -lt 1 ]; then +cat <smb_fname into SMB_VFS_CREATE_FILE leading + * to an error. + * + * Feel free to expand with more options as needed + */ +static NTSTATUS cmd_create_file( + struct vfs_state *vfs, + TALLOC_CTX *mem_ctx, + int argc, + const char **argv) +{ + struct smb_filename *fname = NULL; + struct files_struct *fsp = NULL; + int info, ret; + NTSTATUS status; + + if (argc != 2) { + DBG_ERR("Usage: create_file filename\n"); + return NT_STATUS_UNSUCCESSFUL; + } + + fname = synthetic_smb_fname( + talloc_tos(), argv[1], NULL, NULL, 0, 0); + if (fname == NULL) { + return NT_STATUS_NO_MEMORY; + } + + ret = vfs_stat(vfs->conn, fname); + if (ret != 0) { + status = map_nt_error_from_unix(errno); + DBG_DEBUG("vfs_stat() failed: %s\n", strerror(errno)); + TALLOC_FREE(fname); + return status; + } + + status = openat_pathref_fsp(vfs->conn->cwd_fsp, fname); + if (!NT_STATUS_IS_OK(status)) { + DBG_DEBUG("Could not open %s: %s\n", + fname->base_name, + nt_errstr(status)); + TALLOC_FREE(fname); + return status; + } + + status = SMB_VFS_CREATE_FILE( + vfs->conn, + NULL, + + /* + * Using fname->fsp->fsp_name seems to be legal, + * there's code to handle this in + * create_file_unixpath(). And it is actually very + * worthwhile re-using the fsp_name, we can save quite + * a few copies of smb_filename with that. + */ + fname->fsp->fsp_name, + SEC_FILE_ALL, + FILE_SHARE_NONE, + FILE_OPEN, + FILE_NON_DIRECTORY_FILE, + 0, + 0, + NULL, + 0, + 0, + NULL, + NULL, + &fsp, + &info, + NULL, + NULL + ); + DBG_DEBUG("create_file returned %s\n", nt_errstr(status)); + + TALLOC_FREE(fname); + + return NT_STATUS_OK; +} struct cmd_set vfs_commands[] = { @@ -2237,5 +2317,10 @@ struct cmd_set vfs_commands[] = { { "test_chain", cmd_test_chain, "test chain code", "test_chain" }, { "translate_name", cmd_translate_name, "VFS translate_name()", "translate_name unix_filename" }, + { "create_file", + cmd_create_file, + "VFS create_file()", + "create_file " + }, {0} }; -- 2.32.0 From e018b690a94217f245267dfebf5e45664479f941 Mon Sep 17 00:00:00 2001 From: Volker Lendecke Date: Wed, 9 Feb 2022 17:23:03 +0100 Subject: [PATCH 12/14] smbd: Factor out fsp_unbind_smb() from file_free() For example, remove our entry from smbXsrv_open_global.tdb BUG: https://bugzilla.samba.org/show_bug.cgi?id=14975 Signed-off-by: Volker Lendecke Reviewed-by: Jeremy Allison (cherry picked from commit e751c6237b750adb4cb59df4a42bb9f39354e7e4) --- source3/smbd/files.c | 16 ++++++++++++---- source3/smbd/proto.h | 1 + 2 files changed, 13 insertions(+), 4 deletions(-) diff --git a/source3/smbd/files.c b/source3/smbd/files.c index be93d0e884e..677b600d0a6 100644 --- a/source3/smbd/files.c +++ b/source3/smbd/files.c @@ -1217,11 +1217,11 @@ static void fsp_free(files_struct *fsp) TALLOC_FREE(fsp); } -void file_free(struct smb_request *req, files_struct *fsp) +/* + * Rundown of all smb-related sub-structures of an fsp + */ +void fsp_unbind_smb(struct smb_request *req, files_struct *fsp) { - struct smbd_server_connection *sconn = fsp->conn->sconn; - uint64_t fnum = fsp->fnum; - if (fsp == fsp->conn->cwd_fsp) { return; } @@ -1262,6 +1262,14 @@ void file_free(struct smb_request *req, files_struct *fsp) * pointers in the SMB2 request queue. */ remove_smb2_chained_fsp(fsp); +} + +void file_free(struct smb_request *req, files_struct *fsp) +{ + struct smbd_server_connection *sconn = fsp->conn->sconn; + uint64_t fnum = fsp->fnum; + + fsp_unbind_smb(req, fsp); /* Drop all remaining extensions. */ vfs_remove_all_fsp_extensions(fsp); diff --git a/source3/smbd/proto.h b/source3/smbd/proto.h index 67a4edb4f88..45519d87ef7 100644 --- a/source3/smbd/proto.h +++ b/source3/smbd/proto.h @@ -412,6 +412,7 @@ struct files_struct *file_find_one_fsp_from_lease_key( struct smbd_server_connection *sconn, const struct smb2_lease_key *lease_key); bool file_find_subpath(files_struct *dir_fsp); +void fsp_unbind_smb(struct smb_request *req, files_struct *fsp); void file_free(struct smb_request *req, files_struct *fsp); files_struct *file_fsp(struct smb_request *req, uint16_t fid); struct files_struct *file_fsp_get(struct smbd_smb2_request *smb2req, -- 2.32.0 From e773e609de055a35c614f50c0bb0e00ca87dc4db Mon Sep 17 00:00:00 2001 From: Volker Lendecke Date: Wed, 9 Feb 2022 18:03:33 +0100 Subject: [PATCH 13/14] smbd: Introduce close_file_smb() This does almost everything that close_file_free() does, but it leaves the fsp around. A normal close_file() now calls fsp_unbind_smb() twice. Functionally this is not a problem, fsp_unbind_smb() is idempotent. The only potential performance penalty might come from the loops in remove_smb2_chained_fsp(), but those only are potentially large with deeply queued smb2 requests. If that turns out to be a problem, we'll cope with it later. The alternative would be to split up file_free() into even more routines and make it more difficult to figure out which of the "rundown/unbind/free" routines to call in any particular situation. BUG: https://bugzilla.samba.org/show_bug.cgi?id=14975 Signed-off-by: Volker Lendecke Reviewed-by: Jeremy Allison (cherry picked from commit e91b59c4dfb2b35661dbecbc5769584109e23571) --- source3/smbd/close.c | 26 +++++++++++++++++++------- source3/smbd/proto.h | 3 +++ 2 files changed, 22 insertions(+), 7 deletions(-) diff --git a/source3/smbd/close.c b/source3/smbd/close.c index 9e7f89a304f..206515202e0 100644 --- a/source3/smbd/close.c +++ b/source3/smbd/close.c @@ -1475,15 +1475,14 @@ static NTSTATUS close_directory(struct smb_request *req, files_struct *fsp, } /**************************************************************************** - Close a files_struct. + Rundown all SMB-related dependencies of a files struct ****************************************************************************/ -NTSTATUS close_file_free(struct smb_request *req, - struct files_struct **_fsp, - enum file_close_type close_type) +NTSTATUS close_file_smb(struct smb_request *req, + struct files_struct *fsp, + enum file_close_type close_type) { NTSTATUS status; - struct files_struct *fsp = *_fsp; /* * This fsp can never be an internal dirfsp. They must @@ -1541,9 +1540,22 @@ NTSTATUS close_file_free(struct smb_request *req, close_file_free(req, &fsp->base_fsp, close_type); } - file_free(req, fsp); + fsp_unbind_smb(req, fsp); - *_fsp = NULL; + return status; +} + +NTSTATUS close_file_free(struct smb_request *req, + struct files_struct **_fsp, + enum file_close_type close_type) +{ + struct files_struct *fsp = *_fsp; + NTSTATUS status; + + status = close_file_smb(req, fsp, close_type); + + file_free(req, fsp); + *_fsp = NULL; return status; } diff --git a/source3/smbd/proto.h b/source3/smbd/proto.h index 45519d87ef7..598ca1de2e2 100644 --- a/source3/smbd/proto.h +++ b/source3/smbd/proto.h @@ -130,6 +130,9 @@ bool smbd_smb1_brl_finish_by_mid( /* The following definitions come from smbd/close.c */ void set_close_write_time(struct files_struct *fsp, struct timespec ts); +NTSTATUS close_file_smb(struct smb_request *req, + struct files_struct *fsp, + enum file_close_type close_type); NTSTATUS close_file_free(struct smb_request *req, struct files_struct **_fsp, enum file_close_type close_type); -- 2.32.0 From 7f2f15b0b6e741be3a6fc3b7d05cab4b984dc048 Mon Sep 17 00:00:00 2001 From: Volker Lendecke Date: Thu, 3 Feb 2022 17:17:07 +0100 Subject: [PATCH 14/14] smbd: Only file_free() a self-created fsp in create_file_unixpath() This fixes a use-after-free in smb_full_audit_create_file() when calling SMB_VFS_CREATE_FILE with fsp->fsp_name as smb_fname. create_file_unixpath() has this comment: * This is really subtle. If someone passes in an smb_fname * where smb_fname actually is taken from fsp->fsp_name, then * the lifetime of these objects is meant to be the same. so it seems legitimate to call CREATE_FILE this way. When CREATE_FILE runs into an error, create_file_unixpath() does a file_free, which also takes fsp->fsp_name with it. smb_full_audit_create_file() wants to log the failure including the smb_fname after NEXT_CREATE_FILE has exited, but this will then use the already free'ed data. Fix by only doing the file_free() on an fsp that create_file_unixpath() created itself. BUG: https://bugzilla.samba.org/show_bug.cgi?id=14975 Signed-off-by: Volker Lendecke Reviewed-by: Jeremy Allison Autobuild-User(master): Jeremy Allison Autobuild-Date(master): Thu Feb 10 19:11:33 UTC 2022 on sn-devel-184 (cherry picked from commit 434e6d4b4b45757878642d229d26d146792a3878) --- selftest/knownfail.d/full_audit_crash | 1 - source3/smbd/open.c | 13 ++++++++++++- 2 files changed, 12 insertions(+), 2 deletions(-) delete mode 100644 selftest/knownfail.d/full_audit_crash diff --git a/selftest/knownfail.d/full_audit_crash b/selftest/knownfail.d/full_audit_crash deleted file mode 100644 index 9154ea334f2..00000000000 --- a/selftest/knownfail.d/full_audit_crash +++ /dev/null @@ -1 +0,0 @@ -^samba.vfstest.full_audit_segfault.vfstest\(nt4_dc:local\) \ No newline at end of file diff --git a/source3/smbd/open.c b/source3/smbd/open.c index 5d8251dcef5..a5664b319ad 100644 --- a/source3/smbd/open.c +++ b/source3/smbd/open.c @@ -5533,6 +5533,7 @@ static NTSTATUS create_file_unixpath(connection_struct *conn, int info = FILE_WAS_OPENED; files_struct *base_fsp = NULL; files_struct *fsp = NULL; + bool free_fsp_on_error = false; NTSTATUS status; int ret; struct smb_filename *parent_dir_fname = NULL; @@ -5784,6 +5785,11 @@ static NTSTATUS create_file_unixpath(connection_struct *conn, * them from each other. */ smb_fname_fsp_unlink(smb_fname); + + /* + * "fsp" is ours now + */ + free_fsp_on_error = true; } status = fsp_bind_smb(fsp, req); @@ -5807,6 +5813,7 @@ static NTSTATUS create_file_unixpath(connection_struct *conn, if(!NT_STATUS_IS_OK(status)) { goto fail; } + free_fsp_on_error = true; status = fsp_set_smb_fname(fsp, smb_fname); if (!NT_STATUS_IS_OK(status)) { @@ -6053,7 +6060,11 @@ static NTSTATUS create_file_unixpath(connection_struct *conn, * fsp->base_fsp. */ base_fsp = NULL; - close_file_free(req, &fsp, ERROR_CLOSE); + close_file_smb(req, fsp, ERROR_CLOSE); + if (free_fsp_on_error) { + file_free(req, fsp); + fsp = NULL; + } } if (base_fsp != NULL) { close_file_free(req, &base_fsp, ERROR_CLOSE); -- 2.32.0