From 5e0f5a8a531f065cacc33f6331e09910e00ee952 Mon Sep 17 00:00:00 2001 From: Jeremy Allison Date: Thu, 28 Jan 2021 10:46:33 -0800 Subject: [PATCH 1/4] 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 --- 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 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, -- 2.27.0 From b5269170547a6073c4cb2ef6cebe1426bfbda35e Mon Sep 17 00:00:00 2001 From: Jeremy Allison Date: Thu, 28 Jan 2021 10:56:18 -0800 Subject: [PATCH 2/4] 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 --- source3/torture/torture.c | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/source3/torture/torture.c b/source3/torture/torture.c index cdf5d5ca3aa..4f572902494 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; } -- 2.27.0 From 2a6e6757ad040db952f4041f31b5e12665b12e27 Mon Sep 17 00:00:00 2001 From: Jeremy Allison Date: Thu, 28 Jan 2021 11:08:48 -0800 Subject: [PATCH 3/4] 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 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. BUG: https://bugzilla.samba.org/show_bug.cgi?id=13992 Signed-off-by: Jeremy Allison --- source3/libsmb/clientgen.c | 30 ++++++++++++++++++++++++++++-- 1 file changed, 28 insertions(+), 2 deletions(-) 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) -- 2.27.0 From caabbf6fd9f538c2a6c3f06ae67950ca06656363 Mon Sep 17 00:00:00 2001 From: Jeremy Allison Date: Thu, 28 Jan 2021 11:10:54 -0800 Subject: [PATCH 4/4] s3: libcli: smbXcli_tcon_copy() is now unused. The previous commit removed the only user. https://bugzilla.samba.org/show_bug.cgi?id=13992 Signed-off-by: Jeremy Allison --- libcli/smb/smbXcli_base.c | 32 -------------------------------- libcli/smb/smbXcli_base.h | 2 -- 2 files changed, 34 deletions(-) diff --git a/libcli/smb/smbXcli_base.c b/libcli/smb/smbXcli_base.c index e4d495f9622..3b760ea5718 100644 --- a/libcli/smb/smbXcli_base.c +++ b/libcli/smb/smbXcli_base.c @@ -6494,38 +6494,6 @@ struct smbXcli_tcon *smbXcli_tcon_create(TALLOC_CTX *mem_ctx) return tcon; } -/* - * Return a deep structure copy of a struct smbXcli_tcon * - */ - -struct smbXcli_tcon *smbXcli_tcon_copy(TALLOC_CTX *mem_ctx, - const struct smbXcli_tcon *tcon_in) -{ - struct smbXcli_tcon *tcon; - - tcon = talloc_memdup(mem_ctx, tcon_in, sizeof(struct smbXcli_tcon)); - if (tcon == NULL) { - return NULL; - } - - /* Deal with the SMB1 strings. */ - if (tcon_in->smb1.service != NULL) { - tcon->smb1.service = talloc_strdup(tcon, tcon_in->smb1.service); - if (tcon->smb1.service == NULL) { - TALLOC_FREE(tcon); - return NULL; - } - } - if (tcon->smb1.fs_type != NULL) { - tcon->smb1.fs_type = talloc_strdup(tcon, tcon_in->smb1.fs_type); - if (tcon->smb1.fs_type == NULL) { - TALLOC_FREE(tcon); - return NULL; - } - } - return tcon; -} - void smbXcli_tcon_set_fs_attributes(struct smbXcli_tcon *tcon, uint32_t fs_attributes) { diff --git a/libcli/smb/smbXcli_base.h b/libcli/smb/smbXcli_base.h index d9c3175bdf5..d908b4e31fd 100644 --- a/libcli/smb/smbXcli_base.h +++ b/libcli/smb/smbXcli_base.h @@ -522,8 +522,6 @@ NTSTATUS smb2cli_session_encryption_on(struct smbXcli_session *session); uint16_t smb2cli_session_get_encryption_cipher(struct smbXcli_session *session); struct smbXcli_tcon *smbXcli_tcon_create(TALLOC_CTX *mem_ctx); -struct smbXcli_tcon *smbXcli_tcon_copy(TALLOC_CTX *mem_ctx, - const struct smbXcli_tcon *tcon_in); void smbXcli_tcon_set_fs_attributes(struct smbXcli_tcon *tcon, uint32_t fs_attributes); uint32_t smbXcli_tcon_get_fs_attributes(struct smbXcli_tcon *tcon); -- 2.27.0