The Samba-Bugzilla – Attachment 17253 Details for
Bug 15038
SMB2_CLOSE_FLAGS_FULL_INFORMATION fails to return information on renamed file handle.
Home
|
New
|
Browse
|
Search
|
[?]
|
Reports
|
Requests
|
Help
|
New Account
|
Log In
[x]
|
Forgot Password
Login:
[x]
[patch]
git-am fix for master.
bug-15038-master (text/plain), 12.69 KB, created by
Jeremy Allison
on 2022-03-29 01:44:51 UTC
(
hide
)
Description:
git-am fix for master.
Filename:
MIME Type:
Creator:
Jeremy Allison
Created:
2022-03-29 01:44:51 UTC
Size:
12.69 KB
patch
obsolete
>From 77ef1e9b25043fe01385d52c1aa48e8c0a320db9 Mon Sep 17 00:00:00 2001 >From: Jeremy Allison <jra@samba.org> >Date: Mon, 28 Mar 2022 18:09:20 -0700 >Subject: [PATCH 1/6] s3: tests.py: Only run smb2.rename against fileserver. > >No need to run this against nt4_dc or ad_dc. > >BUG: https://bugzilla.samba.org/show_bug.cgi?id=15038 > >Signed-off-by: Jeremy Allison <jra@samba.org> >--- > source3/selftest/tests.py | 2 ++ > 1 file changed, 2 insertions(+) > >diff --git a/source3/selftest/tests.py b/source3/selftest/tests.py >index 2bfb38fdfff..cae09571fe1 100755 >--- a/source3/selftest/tests.py >+++ b/source3/selftest/tests.py >@@ -1017,6 +1017,8 @@ for t in tests: > plansmbtorture4testsuite("smb2.async_dosmode", > "simpleserver", > "//$SERVER_IP/async_dosmode_shadow_copy2 -U$USERNAME%$PASSWORD") >+ elif t == "smb2.rename": >+ plansmbtorture4testsuite(t, "fileserver", '//$SERVER_IP/tmp -U$USERNAME%$PASSWORD') > elif t == "rpc.wkssvc": > plansmbtorture4testsuite(t, "ad_member", '//$SERVER/tmp -U$DC_USERNAME%$DC_PASSWORD') > elif t == "rpc.srvsvc": >-- >2.32.0 > > >From cae37eaf61467ff943ac5e39b694a6559ac6fea1 Mon Sep 17 00:00:00 2001 >From: Jeremy Allison <jra@samba.org> >Date: Mon, 28 Mar 2022 18:23:05 -0700 >Subject: [PATCH 2/6] s4: torture: Add CHECK_VAL macro to smb2/rename.c. Not > yet used. > >BUG: https://bugzilla.samba.org/show_bug.cgi?id=15038 > >Signed-off-by: Jeremy Allison <jra@samba.org> >--- > source4/torture/smb2/rename.c | 14 ++++++++++++++ > 1 file changed, 14 insertions(+) > >diff --git a/source4/torture/smb2/rename.c b/source4/torture/smb2/rename.c >index 5055cf24344..60234769797 100644 >--- a/source4/torture/smb2/rename.c >+++ b/source4/torture/smb2/rename.c >@@ -30,6 +30,20 @@ > > #include "librpc/gen_ndr/security.h" > >+#define CHECK_VAL(v, correct) \ >+ do { \ >+ if ((v) != (correct)) { \ >+ torture_result(torture, \ >+ TORTURE_FAIL, \ >+ "(%s): wrong value for %s got " \ >+ "0x%llx - should be 0x%llx\n", \ >+ __location__, #v, \ >+ (unsigned long long)v, \ >+ (unsigned long long)correct); \ >+ ret = false; \ >+ goto done; \ >+ }} while (0) >+ > #define CHECK_STATUS(status, correct) do { \ > if (!NT_STATUS_EQUAL(status, correct)) { \ > torture_result(torture, TORTURE_FAIL, \ >-- >2.32.0 > > >From 4a4a14dccac2e719f54e6adc4b78249f9e18782a Mon Sep 17 00:00:00 2001 >From: Jeremy Allison <jra@samba.org> >Date: Mon, 28 Mar 2022 18:24:27 -0700 >Subject: [PATCH 3/6] s4: torture: Add CHECK_CREATED macro to smb2/rename.c. > Not yet used. > >BUG: https://bugzilla.samba.org/show_bug.cgi?id=15038 > >Signed-off-by: Jeremy Allison <jra@samba.org> >--- > source4/torture/smb2/rename.c | 8 ++++++++ > 1 file changed, 8 insertions(+) > >diff --git a/source4/torture/smb2/rename.c b/source4/torture/smb2/rename.c >index 60234769797..c8d3c95d166 100644 >--- a/source4/torture/smb2/rename.c >+++ b/source4/torture/smb2/rename.c >@@ -44,6 +44,14 @@ > goto done; \ > }} while (0) > >+#define CHECK_CREATED(__io, __created, __attribute) \ >+ do { \ >+ CHECK_VAL((__io)->out.create_action, NTCREATEX_ACTION_ ## __created); \ >+ CHECK_VAL((__io)->out.size, 0); \ >+ CHECK_VAL((__io)->out.file_attr, (__attribute)); \ >+ CHECK_VAL((__io)->out.reserved2, 0); \ >+ } while(0) >+ > #define CHECK_STATUS(status, correct) do { \ > if (!NT_STATUS_EQUAL(status, correct)) { \ > torture_result(torture, TORTURE_FAIL, \ >-- >2.32.0 > > >From 1ca76bce4acf730a5704e62372412c5fa80f335f Mon Sep 17 00:00:00 2001 >From: Jeremy Allison <jra@samba.org> >Date: Mon, 28 Mar 2022 18:25:54 -0700 >Subject: [PATCH 4/6] s4: torture: Add test_smb2_close_full_information() test > to smb2.rename. > >Creates a file, opens it again on two different connections >and then renames it. When we close and ask for SMB2_CLOSE_FLAGS_FULL_INFORMATION >we expect this to succeed and return valid data on the handles that did not do >the rename request. > >This currently succeeds by accident on master, so we are not >adding a knownfail.d/ file here. When we back-port this test >to 4.16.next, 4.15.next we will add a knownfail.d file. > >The rename request zeros out the fsp->fsp_name->st field on the handles >that are open but are not being renamed, marking them as INVALID_STAT. > >This should not happen on any open handle. Fix to follow will >preserve the field on rename in both the local connection and >different connection case. > >Master gets away with this as in this branch, openat_pathref_fsp(), >which we use in the setup_close_full_information() call to fetch >the SMB2_CLOSE_FLAGS_FULL_INFORMATION data doesn't require an >existing VALID_STAT struct in order to open the file. This >hides the fact the rename zeroed out fsp->fsp_name->st. > >4.16.x and 4.15.x don't have this fix, so expose the bug. >Regardless, even in master we should not zero out any >fsp->fsp_name->st values on rename. > >BUG: https://bugzilla.samba.org/show_bug.cgi?id=15038 > >Signed-off-by: Jeremy Allison <jra@samba.org> >--- > source4/torture/smb2/rename.c | 125 ++++++++++++++++++++++++++++++++++ > 1 file changed, 125 insertions(+) > >diff --git a/source4/torture/smb2/rename.c b/source4/torture/smb2/rename.c >index c8d3c95d166..1b3dca8bcba 100644 >--- a/source4/torture/smb2/rename.c >+++ b/source4/torture/smb2/rename.c >@@ -1437,6 +1437,127 @@ static bool torture_smb2_rename_dir_bench(struct torture_context *tctx, > return true; > } > >+static bool test_smb2_close_full_information(struct torture_context *torture, >+ struct smb2_tree *tree1, >+ struct smb2_tree *tree2) >+{ >+ union smb_close cl; >+ struct smb2_create io = {0}; >+ struct smb2_handle h1 = {{0}}; >+ struct smb2_handle h2 = {{0}}; >+ struct smb2_handle h3 = {{0}}; >+ union smb_setfileinfo sinfo; >+ NTSTATUS status; >+ const char *fname_src = "request.dat"; >+ const char *fname_dst = "renamed.dat"; >+ bool ret = true; >+ >+ /* Start with a tidy share. */ >+ smb2_util_unlink(tree1, fname_src); >+ smb2_util_unlink(tree1, fname_dst); >+ >+ /* Create the test file, and leave it open. */ >+ io.in.fname = fname_src; >+ io.in.desired_access = SEC_FILE_READ_DATA | SEC_FILE_READ_ATTRIBUTE; >+ io.in.create_disposition = NTCREATEX_DISP_CREATE; >+ io.in.share_access = NTCREATEX_SHARE_ACCESS_READ | >+ NTCREATEX_SHARE_ACCESS_WRITE | >+ NTCREATEX_SHARE_ACCESS_DELETE; >+ status = smb2_create(tree1, tree1, &io); >+ CHECK_STATUS(status, NT_STATUS_OK); >+ h1 = io.out.file.handle; >+ CHECK_CREATED(&io, CREATED, FILE_ATTRIBUTE_ARCHIVE); >+ >+ /* Open the test file on the second connection. */ >+ ZERO_STRUCT(io); >+ io.in.fname = fname_src; >+ io.in.desired_access = SEC_FILE_READ_DATA | SEC_FILE_READ_ATTRIBUTE; >+ io.in.create_disposition = NTCREATEX_DISP_OPEN; >+ io.in.share_access = NTCREATEX_SHARE_ACCESS_READ | >+ NTCREATEX_SHARE_ACCESS_WRITE | >+ NTCREATEX_SHARE_ACCESS_DELETE; >+ status = smb2_create(tree2, tree2, &io); >+ CHECK_STATUS(status, NT_STATUS_OK); >+ h2 = io.out.file.handle; >+ >+ /* Now open for rename on the first connection. */ >+ ZERO_STRUCT(io); >+ io.in.fname = fname_src; >+ io.in.desired_access = SEC_STD_DELETE | SEC_FILE_READ_ATTRIBUTE; >+ io.in.create_disposition = NTCREATEX_DISP_OPEN; >+ io.in.share_access = NTCREATEX_SHARE_ACCESS_READ | >+ NTCREATEX_SHARE_ACCESS_WRITE | >+ NTCREATEX_SHARE_ACCESS_DELETE; >+ status = smb2_create(tree1, tree1, &io); >+ CHECK_STATUS(status, NT_STATUS_OK); >+ h3 = io.out.file.handle; >+ >+ /* Do the rename. */ >+ ZERO_STRUCT(sinfo); >+ sinfo.rename_information.level = RAW_SFILEINFO_RENAME_INFORMATION; >+ sinfo.rename_information.in.file.handle = h3; >+ sinfo.rename_information.in.new_name = fname_dst; >+ status = smb2_setinfo_file(tree1, &sinfo); >+ CHECK_STATUS(status, NT_STATUS_OK); >+ >+ /* And close h3. */ >+ ZERO_STRUCT(cl.smb2); >+ cl.smb2.level = RAW_CLOSE_SMB2; >+ cl.smb2.in.file.handle = h3; >+ status = smb2_close(tree1, &cl.smb2); >+ CHECK_STATUS(status, NT_STATUS_OK); >+ ZERO_STRUCT(h3); >+ >+ /* >+ * Close h1 with SMB2_CLOSE_FLAGS_FULL_INFORMATION. >+ * Ensure we get data. >+ */ >+ ZERO_STRUCT(cl.smb2); >+ cl.smb2.level = RAW_CLOSE_SMB2; >+ cl.smb2.in.file.handle = h1; >+ cl.smb2.in.flags = SMB2_CLOSE_FLAGS_FULL_INFORMATION; >+ status = smb2_close(tree1, &cl.smb2); >+ CHECK_STATUS(status, NT_STATUS_OK); >+ ZERO_STRUCT(h1); >+ CHECK_VAL(cl.smb2.out.file_attr, 0x20); >+ >+ /* >+ * Wait 3 seconds for name change to propagate >+ * to the other connection. >+ */ >+ sleep(3); >+ >+ /* >+ * Close h2 with SMB2_CLOSE_FLAGS_FULL_INFORMATION. >+ * This is on connection2. >+ * Ensure we get data. >+ */ >+ ZERO_STRUCT(cl.smb2); >+ cl.smb2.level = RAW_CLOSE_SMB2; >+ cl.smb2.in.file.handle = h2; >+ cl.smb2.in.flags = SMB2_CLOSE_FLAGS_FULL_INFORMATION; >+ status = smb2_close(tree2, &cl.smb2); >+ CHECK_STATUS(status, NT_STATUS_OK); >+ ZERO_STRUCT(h2); >+ CHECK_VAL(cl.smb2.out.file_attr, 0x20); >+ >+ done: >+ >+ if (h1.data[0] != 0 || h1.data[1] != 0) { >+ smb2_util_close(tree1, h1); >+ } >+ if (h2.data[0] != 0 || h2.data[1] != 0) { >+ smb2_util_close(tree2, h2); >+ } >+ if (h3.data[0] != 0 || h3.data[1] != 0) { >+ smb2_util_close(tree1, h3); >+ } >+ >+ smb2_util_unlink(tree1, fname_src); >+ smb2_util_unlink(tree1, fname_dst); >+ >+ return ret; >+} > > /* > basic testing of SMB2 rename >@@ -1483,6 +1604,10 @@ struct torture_suite *torture_smb2_rename_init(TALLOC_CTX *ctx) > "rename_dir_bench", > torture_smb2_rename_dir_bench); > >+ torture_suite_add_2smb2_test(suite, >+ "close-full-information", >+ test_smb2_close_full_information); >+ > suite->description = talloc_strdup(suite, "smb2.rename tests"); > > return suite; >-- >2.32.0 > > >From f6f9300f13edc2c2c800bbc13d032144b2572ee9 Mon Sep 17 00:00:00 2001 >From: Jeremy Allison <jra@samba.org> >Date: Mon, 28 Mar 2022 18:42:18 -0700 >Subject: [PATCH 5/6] s3: smbd: Preserve the fsp->fsp_name->st bufs across > rename_open_files() > >BUG: https://bugzilla.samba.org/show_bug.cgi?id=15038 > >Signed-off-by: Jeremy Allison <jra@samba.org> >--- > source3/smbd/reply.c | 15 +++++++++++++++ > 1 file changed, 15 insertions(+) > >diff --git a/source3/smbd/reply.c b/source3/smbd/reply.c >index 1677d997fe9..31f2d4d7a08 100644 >--- a/source3/smbd/reply.c >+++ b/source3/smbd/reply.c >@@ -6960,6 +6960,7 @@ static void rename_open_files(connection_struct *conn, > > for(fsp = file_find_di_first(conn->sconn, id, false); fsp; > fsp = file_find_di_next(fsp, false)) { >+ SMB_STRUCT_STAT fsp_orig_sbuf; > struct file_id_buf idbuf; > /* fsp_name is a relative path under the fsp. To change this for other > sharepaths we need to manipulate relative paths. */ >@@ -6978,10 +6979,24 @@ static void rename_open_files(connection_struct *conn, > fsp_str_dbg(fsp), > smb_fname_str_dbg(smb_fname_dst)); > >+ /* >+ * The incoming smb_fname_dst here has an >+ * invalid stat struct (it must not have >+ * existed for the rename to succeed). >+ * Preserve the existing stat from the >+ * open fsp after fsp_set_smb_fname() >+ * overwrites with the invalid stat. >+ * >+ * We will do an fstat before returning >+ * any of this metadata to the client anyway. >+ */ >+ fsp_orig_sbuf = fsp->fsp_name->st; > status = fsp_set_smb_fname(fsp, smb_fname_dst); > if (NT_STATUS_IS_OK(status)) { > did_rename = True; > new_name_hash = fsp->name_hash; >+ /* Restore existing stat. */ >+ fsp->fsp_name->st = fsp_orig_sbuf; > } > } > >-- >2.32.0 > > >From 580740c4105e07fcfcb3d34287845845989ccbbb Mon Sep 17 00:00:00 2001 >From: Jeremy Allison <jra@samba.org> >Date: Mon, 28 Mar 2022 18:39:55 -0700 >Subject: [PATCH 6/6] s3: smbd: Preserve the fsp->fsp_name->st buf across a > MSG_SMB_FILE_RENAME message. > >BUG: https://bugzilla.samba.org/show_bug.cgi?id=15038 > >Signed-off-by: Jeremy Allison <jra@samba.org> >--- > source3/smbd/open.c | 20 ++++++++++++++++++++ > 1 file changed, 20 insertions(+) > >diff --git a/source3/smbd/open.c b/source3/smbd/open.c >index 0d64e534149..51419d4272f 100644 >--- a/source3/smbd/open.c >+++ b/source3/smbd/open.c >@@ -4887,16 +4887,36 @@ void msg_file_was_renamed(struct messaging_context *msg_ctx, > } > > if (strcmp(fsp->conn->connectpath, msg->servicepath) == 0) { >+ SMB_STRUCT_STAT fsp_orig_sbuf; > NTSTATUS status; > DBG_DEBUG("renaming file %s from %s -> %s\n", > fsp_fnum_dbg(fsp), > fsp_str_dbg(fsp), > smb_fname_str_dbg(smb_fname)); >+ >+ /* >+ * The incoming smb_fname here has an >+ * invalid stat struct from synthetic_smb_fname() >+ * above. >+ * Preserve the existing stat from the >+ * open fsp after fsp_set_smb_fname() >+ * overwrites with the invalid stat. >+ * >+ * (We could just copy this into >+ * smb_fname->st, but keep this code >+ * identical to the fix in rename_open_files() >+ * for clarity. >+ * >+ * We will do an fstat before returning >+ * any of this metadata to the client anyway. >+ */ >+ fsp_orig_sbuf = fsp->fsp_name->st; > status = fsp_set_smb_fname(fsp, smb_fname); > if (!NT_STATUS_IS_OK(status)) { > DBG_DEBUG("fsp_set_smb_fname failed: %s\n", > nt_errstr(status)); > } >+ fsp->fsp_name->st = fsp_orig_sbuf; > } else { > /* TODO. JRA. */ > /* >-- >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=17253">View the attachment on a separate page</a>.</b>
View Attachment As Raw
Actions:
View
Attachments on
bug 15038
:
17253
|
17257