[SCM] Samba Shared Repository - branch v4-14-test updated
Karolin Seeger
kseeger at samba.org
Wed Feb 3 22:33:01 UTC 2021
The branch, v4-14-test has been updated
via df0dd2ae007 s3: libsmb: cli_state_save_tcon(). Don't deepcopy tcon struct when temporarily swapping out a connection on a cli_state.
via b6a9277beae s3: torture: Change the SMB1-only UID-REGRESSION-TEST to do an explicit copy of the tcon struct in use.
via b6183a479ca s3: smbtorture3: Ensure run_tcon_test() always replaces any saved tcon and shuts down correctly even in error paths.
via 55294ccdeca s3: smbtorture3: Ensure we *always* replace the saved saved_tcon even in an error condition.
via 7125792f0e1 s3: libsmb: Ensure we disconnect the temporary SMB1 tcon pointer on failure to set up encryption.
via 42f41c5ca5e s3: tests: Add regression test for bug 13992.
via eac2d1504b7 s3:smbd: Fix invalid memory access in posix_sys_acl_blob_get_fd()
from cc1568be4d4 script/autobuild.py: let cleanup() ignore errors from rmdir_force() by default
https://git.samba.org/?p=samba.git;a=shortlog;h=v4-14-test
- Log -----------------------------------------------------------------
commit df0dd2ae007e96261fb98e3cf858543c116b81ab
Author: Jeremy Allison <jra at samba.org>
Date: Thu Jan 28 11:08:48 2021 -0800
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 at samba.org>
Reviewed-by: Andreas Schneider <asn at samba.org>
Autobuild-User(master): Jeremy Allison <jra at samba.org>
Autobuild-Date(master): Tue Feb 2 21:05:25 UTC 2021 on sn-devel-184
(cherry picked from commit 4f80f5f9046b64a9e5e0503b1cb54f1492c4faec)
Autobuild-User(v4-14-test): Karolin Seeger <kseeger at samba.org>
Autobuild-Date(v4-14-test): Wed Feb 3 22:32:58 UTC 2021 on sn-devel-184
commit b6a9277beaeb7dd113ee6eb95243af8701985216
Author: Jeremy Allison <jra at samba.org>
Date: Thu Jan 28 17:35:55 2021 -0800
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 at samba.org>
Reviewed-by: Andreas Schneider <asn at samba.org>
(cherry picked from commit e93e6108837eff0cebad8dc26d055c0e1386093a)
commit b6183a479ca2fdebba123aaa966c2d8041036a62
Author: Jeremy Allison <jra at samba.org>
Date: Thu Jan 28 10:56:18 2021 -0800
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 at samba.org>
Reviewed-by: Andreas Schneider <asn at samba.org>
(cherry picked from commit f9ca91bd293e9f2710c4449c5d4f5d016a066049)
commit 55294ccdeca3a5758256427324da6cfc1b87acd4
Author: Jeremy Allison <jra at samba.org>
Date: Thu Jan 28 10:46:33 2021 -0800
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 at samba.org>
Reviewed-by: Andreas Schneider <asn at samba.org>
(cherry picked from commit dc701959cad7bf15aa47cad6451212606520f67f)
commit 7125792f0e15852c09482c6035b176f92cec6741
Author: Jeremy Allison <jra at samba.org>
Date: Thu Jan 28 14:32:53 2021 -0800
s3: libsmb: Ensure we disconnect the temporary SMB1 tcon pointer on failure to set up encryption.
BUG: https://bugzilla.samba.org/show_bug.cgi?id=13992
Signed-off-by: Jeremy Allison <jra at samba.org>
Reviewed-by: Andreas Schneider <asn at samba.org>
(cherry picked from commit faba89ad59eaa189f325be17377645862080a965)
commit 42f41c5ca5e7138ef4eb9ad428e05e7c2760d528
Author: Jeremy Allison <jra at samba.org>
Date: Thu Jan 28 14:07:23 2021 -0800
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 at samba.org>
Reviewed-by: Andreas Schneider <asn at samba.org>
(cherry picked from commit 068f4a977f0539f790809d580bf22d2362032e3d)
commit eac2d1504b72d766762f2991c0acd1355835a2cd
Author: Andreas Schneider <asn at samba.org>
Date: Mon Feb 1 12:03:17 2021 +0100
s3:smbd: Fix invalid memory access in posix_sys_acl_blob_get_fd()
We are handing down an out of scope buffer.
Found by AddressSanitizer.
BUG: https://bugzilla.samba.org/show_bug.cgi?id=14627
Signed-off-by: Andreas Schneider <asn at samba.org>
Reviewed-by: Jeremy Allison <jra at samba.org>
(cherry picked from commit 0a93f5367bc55ee14f13da5bdb812333c9d9e9f3)
-----------------------------------------------------------------------
Summary of changes:
source3/libsmb/clidfs.c | 7 +++++
source3/libsmb/clientgen.c | 30 ++++++++++++++++++++--
.../tests/test_net_rpc_share_allowedusers.sh | 20 +++++++++++++++
source3/smbd/posix_acls.c | 2 +-
source3/torture/test_smb2.c | 2 +-
source3/torture/torture.c | 27 +++++++++++++++----
6 files changed, 79 insertions(+), 9 deletions(-)
Changeset truncated at 500 lines:
diff --git a/source3/libsmb/clidfs.c b/source3/libsmb/clidfs.c
index 26b5499cf73..040b957e6f8 100644
--- a/source3/libsmb/clidfs.c
+++ b/source3/libsmb/clidfs.c
@@ -1203,6 +1203,13 @@ bool cli_check_msdfs_proxy(TALLOC_CTX *ctx,
break;
case SMB_ENCRYPTION_REQUIRED:
default:
+ /*
+ * Failed to set up encryption.
+ * Disconnect the temporary IPC$
+ * tcon before restoring the original
+ * tcon so we don't leak it.
+ */
+ cli_tdis(cli);
cli_state_restore_tcon(cli, orig_tcon);
return false;
}
diff --git a/source3/libsmb/clientgen.c b/source3/libsmb/clientgen.c
index d117885b8f7..e86f52dac0d 100644
--- a/source3/libsmb/clientgen.c
+++ b/source3/libsmb/clientgen.c
@@ -348,11 +348,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)
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
diff --git a/source3/smbd/posix_acls.c b/source3/smbd/posix_acls.c
index 041305d4aec..c1d5b7cd047 100644
--- a/source3/smbd/posix_acls.c
+++ b/source3/smbd/posix_acls.c
@@ -4651,12 +4651,12 @@ int posix_sys_acl_blob_get_fd(vfs_handle_struct *handle,
TALLOC_CTX *frame;
struct smb_acl_wrapper acl_wrapper = { 0 };
int fd = fsp_get_pathref_fd(fsp);
+ char buf[PATH_MAX] = {0};
struct smb_filename fname;
int ret;
if (fsp->fsp_flags.have_proc_fds) {
const char *proc_fd_path = NULL;
- char buf[PATH_MAX];
proc_fd_path = sys_proc_fd_path(fd, buf, sizeof(buf));
if (proc_fd_path == NULL) {
diff --git a/source3/torture/test_smb2.c b/source3/torture/test_smb2.c
index 2d02db3b108..a81e40568e8 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,
diff --git a/source3/torture/torture.c b/source3/torture/torture.c
index cdf5d5ca3aa..d1ea9b85a72 100644
--- a/source3/torture/torture.c
+++ b/source3/torture/torture.c
@@ -1347,6 +1347,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;
}
@@ -1399,6 +1400,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;
}
@@ -1407,6 +1410,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;
}
@@ -11685,7 +11690,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");
@@ -11726,8 +11731,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;
}
@@ -11743,11 +11760,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);
--
Samba Shared Repository
More information about the samba-cvs
mailing list