The Samba-Bugzilla – Attachment 17161 Details for
Bug 14975
Fix a crash in vfs_full_audit - CREATE_FILE can free a used fsp.
Home
|
New
|
Browse
|
Search
|
[?]
|
Reports
|
Requests
|
Help
|
New Account
|
Log In
[x]
|
Forgot Password
Login:
[x]
[patch]
git-am fix for 4.16rcNext.
bug-14975-4.16.patch (text/plain), 64.99 KB, created by
Jeremy Allison
on 2022-02-11 21:50:33 UTC
(
hide
)
Description:
git-am fix for 4.16rcNext.
Filename:
MIME Type:
Creator:
Jeremy Allison
Created:
2022-02-11 21:50:33 UTC
Size:
64.99 KB
patch
obsolete
>From 461471df49cba1d6bae6d22f89a8907dfc9bc5cf Mon Sep 17 00:00:00 2001 >From: Volker Lendecke <vl@samba.org> >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 <vl@samba.org> >Reviewed-by: Jeremy Allison <jra@samba.org> >(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 <vl@samba.org> >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 <vl@samba.org> >Reviewed-by: Jeremy Allison <jra@samba.org> >(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 <vl@samba.org> >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 <vl@samba.org> >Reviewed-by: Jeremy Allison <jra@samba.org> >(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 <vl@samba.org> >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 <vl@samba.org> >Reviewed-by: Jeremy Allison <jra@samba.org> >(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 <vl@samba.org> >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 <vl@samba.org> >Reviewed-by: Jeremy Allison <jra@samba.org> >(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 <vl@samba.org> >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 <vl@samba.org> >Reviewed-by: Jeremy Allison <jra@samba.org> >(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 <vl@samba.org> >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 <vl@samba.org> >Reviewed-by: Jeremy Allison <jra@samba.org> >(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 <vl@samba.org> >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 <vl@samba.org> >Reviewed-by: Jeremy Allison <jra@samba.org> >(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 <vl@samba.org> >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 <vl@samba.org> >Reviewed-by: Jeremy Allison <jra@samba.org> >(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 <vl@samba.org> >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 <vl@samba.org> >Reviewed-by: Jeremy Allison <jra@samba.org> >(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 <vl@samba.org> >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 <vl@samba.org> >Reviewed-by: Jeremy Allison <jra@samba.org> >(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 <<EOF >+Usage: run.sh VFSTEST >+EOF >+exit 1; >+fi >+ >+TALLOC_FILL_FREE=0; export TALLOC_FILL_FREE >+ >+TESTBASE="$(dirname $0)" >+VFSTEST="$VALGRIND $1"; shift 1; >+ADDARGS="$*" >+ >+incdir=`dirname $0`/../../../../testprogs/blackbox >+. $incdir/subunit.sh >+ >+failed=0 >+ >+testit "vfstest" "$VFSTEST" -f "$TESTBASE/vfstest.cmd" "$ADDARGS" || >+ failed=$(expr $failed + 1) >+ >+exit $failed >diff --git a/source3/script/tests/full_audit_segfault/vfstest.cmd b/source3/script/tests/full_audit_segfault/vfstest.cmd >new file mode 100644 >index 00000000000..84e93e2b157 >--- /dev/null >+++ b/source3/script/tests/full_audit_segfault/vfstest.cmd >@@ -0,0 +1,3 @@ >+load full_audit >+connect >+create_file . >diff --git a/source3/selftest/tests.py b/source3/selftest/tests.py >index acf5282b8ec..95192ae19ae 100755 >--- a/source3/selftest/tests.py >+++ b/source3/selftest/tests.py >@@ -359,6 +359,14 @@ plantestsuite("samba.vfstest.stream_depot", "nt4_dc:local", [os.path.join(samba3 > plantestsuite("samba.vfstest.xattr-tdb-1", "nt4_dc:local", [os.path.join(samba3srcdir, "script/tests/xattr-tdb-1/run.sh"), binpath("vfstest"), "$PREFIX", configuration]) > plantestsuite("samba.vfstest.acl", "nt4_dc:local", [os.path.join(samba3srcdir, "script/tests/vfstest-acl/run.sh"), binpath("vfstest"), "$PREFIX", configuration]) > plantestsuite("samba.vfstest.catia", "nt4_dc:local", [os.path.join(samba3srcdir, "script/tests/vfstest-catia/run.sh"), binpath("vfstest"), "$PREFIX", configuration]) >+plantestsuite( >+ "samba.vfstest.full_audit_segfault", >+ "nt4_dc:local", >+ [os.path.join(samba3srcdir, >+ "script/tests/full_audit_segfault/run.sh"), >+ binpath("vfstest"), >+ "$PREFIX", >+ configuration]) > > plantestsuite("samba3.blackbox.smbclient_basic.NT1", "nt4_dc_schannel", [os.path.join(samba3srcdir, "script/tests/test_smbclient_basic.sh"), '$SERVER', '$SERVER_IP', '$DC_USERNAME', '$DC_PASSWORD', smbclient3, configuration, "-mNT1"]) > plantestsuite("samba3.blackbox.smbclient_basic.NT1", "nt4_dc_smb1", [os.path.join(samba3srcdir, "script/tests/test_smbclient_basic.sh"), '$SERVER', '$SERVER_IP', '$DC_USERNAME', '$DC_PASSWORD', smbclient3, configuration, "-mNT1"]) >diff --git a/source3/torture/cmd_vfs.c b/source3/torture/cmd_vfs.c >index 72fa784d72b..4be5719e94e 100644 >--- a/source3/torture/cmd_vfs.c >+++ b/source3/torture/cmd_vfs.c >@@ -2169,6 +2169,86 @@ cleanup: > return status; > } > >+/* >+ * This is a quick hack to demonstrate a crash in the full_audit >+ * module when passing fsp->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 <filename>" >+ }, > {0} > }; >-- >2.32.0 > > >From e018b690a94217f245267dfebf5e45664479f941 Mon Sep 17 00:00:00 2001 >From: Volker Lendecke <vl@samba.org> >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 <vl@samba.org> >Reviewed-by: Jeremy Allison <jra@samba.org> >(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 <vl@samba.org> >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 <vl@samba.org> >Reviewed-by: Jeremy Allison <jra@samba.org> >(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 <vl@samba.org> >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 <vl@samba.org> >Reviewed-by: Jeremy Allison <jra@samba.org> > >Autobuild-User(master): Jeremy Allison <jra@samba.org> >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 >
<b>You cannot view the attachment while viewing its details because your browser does not support IFRAMEs. <a href="attachment.cgi?id=17161">View the attachment on a separate page</a>.</b>
View Attachment As Raw
Flags:
vl
:
review+
Actions:
View
Attachments on
bug 14975
: 17161