Attachment 16424 Details for Bug 13992 – git-am fix for 4.12.next, 4.13.next.

[patch] git-am fix for 4.12.next, 4.13.next.

bug-13992-4-13 (text/plain), 12.06 KB, created by Jeremy Allison on 2021-02-02 22:05:58 UTC

(hide)

Description:

Filename:

MIME Type:

Creator: Jeremy Allison

Created: 2021-02-02 22:05:58 UTC

Size: 12.06 KB

patch

obsolete

>From 2a75d84965be70690da084c818c0778fe0e9330f Mon Sep 17 00:00:00 2001
>From: Jeremy Allison <jra@samba.org>
>Date: Thu, 28 Jan 2021 14:07:23 -0800
>Subject: [PATCH 1/5] s3: tests: Add regression test for bug 13992.
>
>Subtle extra test. Mark as knownfail for now.
>
>'^ user1$' must appear MORE THAN ONCE, as it can read more than one
>share. The previous test found user1, but only once as the bug only
>allows reading the security descriptor for one share, and we were
>unlucky that the first share security descriptor returned allows
>user1 to read from it.
>
>BUG: https://bugzilla.samba.org/show_bug.cgi?id=13992
>
>Signed-off-by: Jeremy Allison <jra@samba.org>
>Reviewed-by: Andreas Schneider <asn@samba.org>
>(cherry picked from commit 068f4a977f0539f790809d580bf22d2362032e3d)
>---
> selftest/knownfail.d/bug-13992                |  2 ++
> .../tests/test_net_rpc_share_allowedusers.sh  | 20 +++++++++++++++++++
> 2 files changed, 22 insertions(+)
> create mode 100644 selftest/knownfail.d/bug-13992
>
>diff --git a/selftest/knownfail.d/bug-13992 b/selftest/knownfail.d/bug-13992
>new file mode 100644
>index 00000000000..76365f09303
>--- /dev/null
>+++ b/selftest/knownfail.d/bug-13992
>@@ -0,0 +1,2 @@
>+# bug 13992
>+^samba3.blackbox.net_rpc_share_allowedusers.net_rpc_share_allowedusers$nt4_dc$
>diff --git a/source3/script/tests/test_net_rpc_share_allowedusers.sh b/source3/script/tests/test_net_rpc_share_allowedusers.sh
>index 5dd382d4c51..d22c7580681 100755
>--- a/source3/script/tests/test_net_rpc_share_allowedusers.sh
>+++ b/source3/script/tests/test_net_rpc_share_allowedusers.sh
>@@ -26,5 +26,25 @@ testit_grep "net_rpc_share_allowedusers" '^print\$$' $net usersidlist | $VALGRIN
> testit_grep "net_rpc_share_allowedusers" '^print\$$' $net usersidlist | $VALGRIND $net rpc share allowedusers -S$SERVER -U$USERNAME%$PASSWORD $ADDARGS - 'print$' || failed=`expr $failed + 1`
> # Check user "user1" is allowed to read share "tmp".
> testit_grep "net_rpc_share_allowedusers" '^ user1$' $net usersidlist | $VALGRIND $net rpc share allowedusers -S$SERVER -U$USERNAME%$PASSWORD $ADDARGS || failed=`expr $failed + 1`
>+#
>+# Subtle extra test for bug https://bugzilla.samba.org/show_bug.cgi?id=13992
>+#
>+# '^ user1$' must appear MORE THAN ONCE, as it can read more than one
>+# share. The previous test found user1, but only once as the bug only
>+# allows reading the security descriptor for one share, and we were
>+# unlucky that the first share security descriptor returned allows
>+# user1 to read from it.
>+#
>+subunit_start_test "net_rpc_share_allowedusers"
>+multi_userout=`$net usersidlist | $VALGRIND $net rpc share allowedusers -S$SERVER -U$USERNAME%$PASSWORD $ADDARGS`
>+num_matches=`echo "$multi_userout" | grep -c '^ user1$'`
>+if [ "$num_matches" -gt "1" ]
>+then
>+	subunit_pass_test "net_rpc_share_allowedusers"
>+else
>+	echo "net_rpc_share_allowedusers only found $num_matches shares readable by user1. Should be greater than one.\n"
>+	failed=`expr $failed + 1`
>+	echo "$multi_userout" | subunit_fail_test "net_rpc_share_allowedusers"
>+fi
> 
> testok $0 $failed
>-- 
>2.27.0
>
>
>From 8e9db89642cba20c4393cd794508b1333cc106fb Mon Sep 17 00:00:00 2001
>From: Jeremy Allison <jra@samba.org>
>Date: Thu, 28 Jan 2021 10:46:33 -0800
>Subject: [PATCH 2/5] s3: smbtorture3: Ensure we *always* replace the saved
> saved_tcon even in an error condition.
>
>BUG: https://bugzilla.samba.org/show_bug.cgi?id=13992
>
>Signed-off-by: Jeremy Allison <jra@samba.org>
>Reviewed-by: Andreas Schneider <asn@samba.org>
>(cherry picked from commit dc701959cad7bf15aa47cad6451212606520f67f)
>---
> source3/torture/test_smb2.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
>diff --git a/source3/torture/test_smb2.c b/source3/torture/test_smb2.c
>index d5023d88cfd..3e25a562bd6 100644
>--- a/source3/torture/test_smb2.c
>+++ b/source3/torture/test_smb2.c
>@@ -188,11 +188,11 @@ bool run_smb2_basic(int dummy)
> 			      cli->timeout,
> 			      cli->smb2.session,
> 			      cli->smb2.tcon);
>+	cli_state_restore_tcon(cli, saved_tcon);
> 	if (!NT_STATUS_IS_OK(status)) {
> 		printf("smb2cli_tdis returned %s\n", nt_errstr(status));
> 		return false;
> 	}
>-	cli_state_restore_tcon(cli, saved_tcon);
> 
> 	status = smb2cli_tdis(cli->conn,
> 			      cli->timeout,
>-- 
>2.27.0
>
>
>From 78da9875cb84104a18cd2739b501b3527739bc9a Mon Sep 17 00:00:00 2001
>From: Jeremy Allison <jra@samba.org>
>Date: Thu, 28 Jan 2021 10:56:18 -0800
>Subject: [PATCH 3/5] s3: smbtorture3: Ensure run_tcon_test() always replaces
> any saved tcon and shuts down correctly even in error paths.
>
>BUG: https://bugzilla.samba.org/show_bug.cgi?id=13992
>
>Signed-off-by: Jeremy Allison <jra@samba.org>
>Reviewed-by: Andreas Schneider <asn@samba.org>
>(cherry picked from commit f9ca91bd293e9f2710c4449c5d4f5d016a066049)
>---
> source3/torture/torture.c | 5 +++++
> 1 file changed, 5 insertions(+)
>
>diff --git a/source3/torture/torture.c b/source3/torture/torture.c
>index 2a3133373e9..6160adb98cc 100644
>--- a/source3/torture/torture.c
>+++ b/source3/torture/torture.c
>@@ -1343,6 +1343,7 @@ static bool run_tcon_test(int dummy)
> 	if (!NT_STATUS_IS_OK(status)) {
> 		printf("%s refused 2nd tree connect (%s)\n", host,
> 		       nt_errstr(status));
>+		cli_state_restore_tcon(cli, orig_tcon);
> 		cli_shutdown(cli);
> 		return False;
> 	}
>@@ -1395,6 +1396,8 @@ static bool run_tcon_test(int dummy)
> 	status = cli_close(cli, fnum1);
> 	if (!NT_STATUS_IS_OK(status)) {
> 		printf("close failed (%s)\n", nt_errstr(status));
>+		cli_state_restore_tcon(cli, orig_tcon);
>+		cli_shutdown(cli);
> 		return False;
> 	}
> 
>@@ -1403,6 +1406,8 @@ static bool run_tcon_test(int dummy)
> 	status = cli_tdis(cli);
> 	if (!NT_STATUS_IS_OK(status)) {
> 		printf("secondary tdis failed (%s)\n", nt_errstr(status));
>+		cli_state_restore_tcon(cli, orig_tcon);
>+		cli_shutdown(cli);
> 		return False;
> 	}
> 
>-- 
>2.27.0
>
>
>From 06d5f2bbd536dab3e5d7395b6f84ccf037ba7457 Mon Sep 17 00:00:00 2001
>From: Jeremy Allison <jra@samba.org>
>Date: Thu, 28 Jan 2021 17:35:55 -0800
>Subject: [PATCH 4/5] s3: torture: Change the SMB1-only UID-REGRESSION-TEST to
> do an explicit copy of the tcon struct in use.
>
>For this test only, explicitly copy the SMB1 tcon struct,
>don't use cli_state_save_tcon()//cli_state_restore_tcon()
>as these calls will soon change to just manipulate the pointer
>to avoid TALLOC_FREE() on the tcon struct which calls
>destructors on child pipe data.
>
>In SMB1 this test calls cli_tdis() twice with an invalid
>vuid and expects the SMB1 tcon struct to be preserved
>across the calls.
>
>SMB1 cli_tdis() frees cli->smb1.tcon so we must put back
>a deep copy into cli->smb1.tcon to be able to safely call
>cli_tdis() again.
>
>This is a test-only hack. Real client code
>uses cli_state_save_tcon()/cli_state_restore_tcon()
>if it needs to temporarily swap out the active
>tcon on a client connection.
>
>BUG: https://bugzilla.samba.org/show_bug.cgi?id=13992
>
>Signed-off-by: Jeremy Allison <jra@samba.org>
>Reviewed-by: Andreas Schneider <asn@samba.org>
>(cherry picked from commit e93e6108837eff0cebad8dc26d055c0e1386093a)
>---
> source3/torture/torture.c | 22 +++++++++++++++++-----
> 1 file changed, 17 insertions(+), 5 deletions(-)
>
>diff --git a/source3/torture/torture.c b/source3/torture/torture.c
>index 6160adb98cc..5e263797730 100644
>--- a/source3/torture/torture.c
>+++ b/source3/torture/torture.c
>@@ -11719,7 +11719,7 @@ static bool run_uid_regression_test(int dummy)
> 	int16_t old_vuid;
> 	int32_t old_cnum;
> 	bool correct = True;
>-	struct smbXcli_tcon *orig_tcon = NULL;
>+	struct smbXcli_tcon *tcon_copy = NULL;
> 	NTSTATUS status;
> 
> 	printf("starting uid regression test\n");
>@@ -11760,8 +11760,20 @@ static bool run_uid_regression_test(int dummy)
> 	}
> 
> 	old_cnum = cli_state_get_tid(cli);
>-	orig_tcon = cli_state_save_tcon(cli);
>-	if (orig_tcon == NULL) {
>+	/*
>+	 * This is an SMB1-only test.
>+	 * Copy the tcon, not "save/restore".
>+	 *
>+	 * In SMB1 the cli_tdis() below frees
>+	 * cli->smb1.tcon so we need a copy
>+	 * of the struct to put back for the
>+	 * second tdis call with invalid vuid.
>+	 *
>+	 * This is a test-only hack. Real client code
>+	 * uses cli_state_save_tcon()/cli_state_restore_tcon().
>+	 */
>+	tcon_copy = smbXcli_tcon_copy(cli, cli->smb1.tcon);
>+	if (tcon_copy == NULL) {
> 		correct = false;
> 		goto out;
> 	}
>@@ -11777,11 +11789,11 @@ static bool run_uid_regression_test(int dummy)
> 	} else {
> 		d_printf("First tdis failed (%s)\n", nt_errstr(status));
> 		correct = false;
>-		cli_state_restore_tcon(cli, orig_tcon);
>+		cli->smb1.tcon = tcon_copy;
> 		goto out;
> 	}
> 
>-	cli_state_restore_tcon(cli, orig_tcon);
>+	cli->smb1.tcon = tcon_copy;
> 	cli_state_set_uid(cli, old_vuid);
> 	cli_state_set_tid(cli, old_cnum);
> 
>-- 
>2.27.0
>
>
>From 5bcf3d93c29b1d4d54c31167497db8de3a4a4ba7 Mon Sep 17 00:00:00 2001
>From: Jeremy Allison <jra@samba.org>
>Date: Thu, 28 Jan 2021 11:08:48 -0800
>Subject: [PATCH 5/5] s3: libsmb: cli_state_save_tcon(). Don't deepcopy tcon
> struct when temporarily swapping out a connection on a cli_state.
>
>This used to make a deep copy of either
>cli->smb2.tcon or cli->smb1.tcon, but this leaves
>the original tcon pointer in place which will then get
>TALLOC_FREE()'d when the new tree connection is made on
>this cli_state.
>
>As there may be pipes open on the old tree connection with
>talloc'ed state allocated using the original tcon pointer as a
>talloc parent we can't deep copy and then free this pointer
>as that will fire the destructors on the pipe memory and
>mark them as not connected.
>
>This call is used to temporarily swap out a tcon pointer
>(whilst keeping existing pipes open) to allow a new tcon
>on the same cli_state and all users correctly call
>cli_state_restore_tcon() once they are finished with
>the new tree connection.
>
>Just return the existing pointer and set the old value to NULL.
>We know we MUST be calling cli_state_restore_tcon() below
>to restore the original tcon tree connection pointer before
>closing the session.
>
>Remove the knownfail.d entry.
>
>BUG: https://bugzilla.samba.org/show_bug.cgi?id=13992
>
>Signed-off-by: Jeremy Allison <jra@samba.org>
>Reviewed-by: Andreas Schneider <asn@samba.org>
>
>Autobuild-User(master): Jeremy Allison <jra@samba.org>
>Autobuild-Date(master): Tue Feb  2 21:05:25 UTC 2021 on sn-devel-184
>
>(cherry picked from commit 4f80f5f9046b64a9e5e0503b1cb54f1492c4faec)
>---
> selftest/knownfail.d/bug-13992 |  2 --
> source3/libsmb/clientgen.c     | 30 ++++++++++++++++++++++++++++--
> 2 files changed, 28 insertions(+), 4 deletions(-)
> delete mode 100644 selftest/knownfail.d/bug-13992
>
>diff --git a/selftest/knownfail.d/bug-13992 b/selftest/knownfail.d/bug-13992
>deleted file mode 100644
>index 76365f09303..00000000000
>--- a/selftest/knownfail.d/bug-13992
>+++ /dev/null
>@@ -1,2 +0,0 @@
>-# bug 13992
>-^samba3.blackbox.net_rpc_share_allowedusers.net_rpc_share_allowedusers$nt4_dc$
>diff --git a/source3/libsmb/clientgen.c b/source3/libsmb/clientgen.c
>index 4412651f505..8ded372d0ba 100644
>--- a/source3/libsmb/clientgen.c
>+++ b/source3/libsmb/clientgen.c
>@@ -352,11 +352,37 @@ uint32_t cli_state_set_tid(struct cli_state *cli, uint32_t tid)
> 
> struct smbXcli_tcon *cli_state_save_tcon(struct cli_state *cli)
> {
>+	/*
>+	 * Note. This used to make a deep copy of either
>+	 * cli->smb2.tcon or cli->smb1.tcon, but this leaves
>+	 * the original pointer in place which will then get
>+	 * TALLOC_FREE()'d when the new connection is made on
>+	 * this cli_state.
>+	 *
>+	 * As there may be pipes open on the old connection with
>+	 * talloc'ed state allocated using the tcon pointer as a
>+	 * parent we can't deep copy and then free this as that
>+	 * closes the open pipes.
>+	 *
>+	 * This call is used to temporarily swap out a tcon pointer
>+	 * to allow a new tcon on the same cli_state.
>+	 *
>+	 * Just return the raw pointer and set the old value to NULL.
>+	 * We know we MUST be calling cli_state_restore_tcon() below
>+	 * to restore before closing the session.
>+	 *
>+	 * See BUG: https://bugzilla.samba.org/show_bug.cgi?id=13992
>+	 */
>+	struct smbXcli_tcon *tcon_ret = NULL;
>+
> 	if (smbXcli_conn_protocol(cli->conn) >= PROTOCOL_SMB2_02) {
>-		return smbXcli_tcon_copy(cli, cli->smb2.tcon);
>+		tcon_ret = cli->smb2.tcon;
>+		cli->smb2.tcon = NULL; /* *Not* TALLOC_FREE(). */
> 	} else {
>-		return smbXcli_tcon_copy(cli, cli->smb1.tcon);
>+		tcon_ret = cli->smb1.tcon;
>+		cli->smb1.tcon = NULL; /* *Not* TALLOC_FREE(). */
> 	}
>+	return tcon_ret;
> }
> 
> void cli_state_restore_tcon(struct cli_state *cli, struct smbXcli_tcon *tcon)
>-- 
>2.27.0
>

Flags:

asn: review+

Actions: View

Attachments on bug 13992: 15249 | 15266 | 15267 | 16412 | 16414 | 16415 | 16416 | 16422 | 16423 | 16424 | 16426