From a88f0e7f38670966bf1553fa5036d650b74c666a Mon Sep 17 00:00:00 2001 From: Jeremy Allison Date: Mon, 18 Sep 2023 14:43:23 -0700 Subject: [PATCH 1/5] s3: smbd: Add some DEVELOPER-only code to panic if the destructor for an aio_lnk is called and the associated fsp doesn't exist. Make this DEVELOPER-only as it walks the entire open file list on every file close (with associated aio). This helps catch really subtle problems with orphaned aio lnk structs. Reproducer test case to follow. BUG: https://bugzilla.samba.org/show_bug.cgi?id=15423 Signed-off-by: Jeremy Allison Reviewed-by: Ralph Boehme (cherry picked from commit 82e88f70f181300f6f98691f6680839a94470e13) --- source3/smbd/smb2_aio.c | 24 ++++++++++++++++++++++++ 1 file changed, 24 insertions(+) diff --git a/source3/smbd/smb2_aio.c b/source3/smbd/smb2_aio.c index 8c01c76a3e9..88aa68d218f 100644 --- a/source3/smbd/smb2_aio.c +++ b/source3/smbd/smb2_aio.c @@ -64,6 +64,9 @@ struct aio_extra *create_aio_extra(TALLOC_CTX *mem_ctx, } struct aio_req_fsp_link { +#ifdef DEVELOPER + struct smbd_server_connection *sconn; +#endif files_struct *fsp; struct tevent_req *req; }; @@ -74,6 +77,24 @@ static int aio_del_req_from_fsp(struct aio_req_fsp_link *lnk) files_struct *fsp = lnk->fsp; struct tevent_req *req = lnk->req; +#ifdef DEVELOPER + struct files_struct *ifsp = NULL; + bool found = false; + + /* + * When this is called, lnk->fsp must still exist + * on the files list for this connection. Panic if not. + */ + for (ifsp = lnk->sconn->files; ifsp; ifsp = ifsp->next) { + if (ifsp == fsp) { + found = true; + } + } + if (!found) { + smb_panic("orphaned lnk on fsp aio list.\n"); + } +#endif + for (i=0; inum_aio_requests; i++) { if (fsp->aio_requests[i] == req) { break; @@ -130,6 +151,9 @@ bool aio_add_req_to_fsp(files_struct *fsp, struct tevent_req *req) lnk->fsp = fsp; lnk->req = req; +#ifdef DEVELOPER + lnk->sconn = fsp->conn->sconn; +#endif talloc_set_destructor(lnk, aio_del_req_from_fsp); return true; -- 2.39.3 From f6e8e599a58d17dede3fc09a20258b0cd263df76 Mon Sep 17 00:00:00 2001 From: Jeremy Allison Date: Mon, 18 Sep 2023 17:09:00 -0700 Subject: [PATCH 2/5] s3: smbd: named pipe reads are async. Use the same logic as for named pipe transacts to avoid crashes on shutdown. Noticed by Metze. BUG: https://bugzilla.samba.org/show_bug.cgi?id=15423 Signed-off-by: Jeremy Allison Reviewed-by: Ralph Boehme (cherry picked from commit 3f32bf887d4425655e81da0b2234cbca3b1d56e6) --- source3/smbd/smb2_read.c | 13 +++++++++++++ 1 file changed, 13 insertions(+) diff --git a/source3/smbd/smb2_read.c b/source3/smbd/smb2_read.c index 0101c42cf76..bac78e4c77a 100644 --- a/source3/smbd/smb2_read.c +++ b/source3/smbd/smb2_read.c @@ -494,6 +494,7 @@ static struct tevent_req *smbd_smb2_read_send(TALLOC_CTX *mem_ctx, if (IS_IPC(smbreq->conn)) { struct tevent_req *subreq = NULL; + bool ok; state->out_data = data_blob_talloc(state, NULL, in_length); if (in_length > 0 && tevent_req_nomem(state->out_data.data, req)) { @@ -515,6 +516,18 @@ static struct tevent_req *smbd_smb2_read_send(TALLOC_CTX *mem_ctx, tevent_req_set_callback(subreq, smbd_smb2_read_pipe_done, req); + + /* + * Make sure we mark the fsp as having outstanding async + * activity so we don't crash on shutdown close. + */ + + ok = aio_add_req_to_fsp(fsp, req); + if (!ok) { + tevent_req_nterror(req, NT_STATUS_NO_MEMORY); + return tevent_req_post(req, ev); + } + return req; } -- 2.39.3 From b1c55e1432ad42af21e756787b3ec6a235ff7f47 Mon Sep 17 00:00:00 2001 From: Jeremy Allison Date: Mon, 18 Sep 2023 17:37:44 -0700 Subject: [PATCH 3/5] s3: smbd: named pipe writes are async. Use the same logic as for named pipe transacts to avoid crashes on shutdown. Noticed by Metze. BUG: https://bugzilla.samba.org/show_bug.cgi?id=15423 Signed-off-by: Jeremy Allison Reviewed-by: Ralph Boehme (cherry picked from commit ea062c3b0d4dbb1f0682f808ac893bf36a6fb194) --- source3/smbd/smb2_write.c | 13 +++++++++++++ 1 file changed, 13 insertions(+) diff --git a/source3/smbd/smb2_write.c b/source3/smbd/smb2_write.c index 269c489642e..a7af20173a1 100644 --- a/source3/smbd/smb2_write.c +++ b/source3/smbd/smb2_write.c @@ -307,6 +307,7 @@ static struct tevent_req *smbd_smb2_write_send(TALLOC_CTX *mem_ctx, if (IS_IPC(smbreq->conn)) { struct tevent_req *subreq = NULL; + bool ok; if (!fsp_is_np(fsp)) { tevent_req_nterror(req, NT_STATUS_FILE_CLOSED); @@ -323,6 +324,18 @@ static struct tevent_req *smbd_smb2_write_send(TALLOC_CTX *mem_ctx, tevent_req_set_callback(subreq, smbd_smb2_write_pipe_done, req); + + /* + * Make sure we mark the fsp as having outstanding async + * activity so we don't crash on shutdown close. + */ + + ok = aio_add_req_to_fsp(fsp, req); + if (!ok) { + tevent_req_nterror(req, NT_STATUS_NO_MEMORY); + return tevent_req_post(req, ev); + } + return req; } -- 2.39.3 From 5365769e354a90a4d17cfef96026094974c1296b Mon Sep 17 00:00:00 2001 From: Jeremy Allison Date: Tue, 19 Sep 2023 14:30:26 -0700 Subject: [PATCH 4/5] s3: torture: Add a new SMB2 test: SMB2-PIPE-READ-ASYNC-DISCONNECT Shows the server crashes if we open a named pipe, do an async read and then disconnect. Adds knownfail: BUG: https://bugzilla.samba.org/show_bug.cgi?id=15423 Signed-off-by: Jeremy Allison Reviewed-by: Ralph Boehme (cherry picked from commit 66398dd03c46633b474438dddb771caa2d245e64) --- .../smb2_pipe_read_async_disconnect | 1 + .../tests/test_smbtorture_nocrash_s3.sh | 38 ++++++ source3/selftest/tests.py | 16 +++ source3/torture/proto.h | 1 + source3/torture/test_smb2.c | 117 ++++++++++++++++++ source3/torture/torture.c | 4 + 6 files changed, 177 insertions(+) create mode 100644 selftest/knownfail.d/smb2_pipe_read_async_disconnect create mode 100755 source3/script/tests/test_smbtorture_nocrash_s3.sh diff --git a/selftest/knownfail.d/smb2_pipe_read_async_disconnect b/selftest/knownfail.d/smb2_pipe_read_async_disconnect new file mode 100644 index 00000000000..342a308adec --- /dev/null +++ b/selftest/knownfail.d/smb2_pipe_read_async_disconnect @@ -0,0 +1 @@ +^samba3.smbtorture_s3.plain.SMB2-PIPE-READ-ASYNC-DISCONNECT.check_panic$fileserver$ diff --git a/source3/script/tests/test_smbtorture_nocrash_s3.sh b/source3/script/tests/test_smbtorture_nocrash_s3.sh new file mode 100755 index 00000000000..b6ef1391262 --- /dev/null +++ b/source3/script/tests/test_smbtorture_nocrash_s3.sh @@ -0,0 +1,38 @@ +#!/bin/sh + +. $(dirname $0)/../../../testprogs/blackbox/subunit.sh + +# this runs the file serving tests that are expected to pass with samba3 + +if [ $# -lt 5 ]; then + cat < +EOF + exit 1 +fi + +t="$1" +unc="$2" +username="$3" +password="$4" +SMBTORTURE="$5" +shift 5 +ADDARGS="$*" + +incdir=$(dirname $0)/../../../testprogs/blackbox +. $incdir/subunit.sh + +panic_count_0=$(grep -c PANIC $SMBD_TEST_LOG) + +echo "$panic_count_0" >/tmp/look + +failed=0 +testit "smbtorture" $VALGRIND $SMBTORTURE $unc -U"$username"%"$password" $ADDARGS $t || failed=$(expr $failed + 1) + +panic_count_1=$(grep -c PANIC $SMBD_TEST_LOG) + +echo "$panic_count_1" >>/tmp/look + +testit "check_panic" test $panic_count_0 -eq $panic_count_1 || failed=$(expr $failed + 1) + +testok $0 $failed diff --git a/source3/selftest/tests.py b/source3/selftest/tests.py index 783cb486012..8986a73f03a 100755 --- a/source3/selftest/tests.py +++ b/source3/selftest/tests.py @@ -481,6 +481,22 @@ plantestsuite("samba3.smbtorture_s3.plain.%s" % "SMB2-DEL-ON-CLOSE-NONWRITE-DELE "", "-l $LOCAL_PATH"]) +# +# Test doing an async read + disconnect on a pipe doesn't crash the server. +# BUG: https://bugzilla.samba.org/show_bug.cgi?id=15423 +# +plantestsuite("samba3.smbtorture_s3.plain.%s" % "SMB2-PIPE-READ-ASYNC-DISCONNECT", + "fileserver", + [os.path.join(samba3srcdir, + "script/tests/test_smbtorture_nocrash_s3.sh"), + 'SMB2-PIPE-READ-ASYNC-DISCONNECT', + '//$SERVER_IP/tmp', + '$USERNAME', + '$PASSWORD', + smbtorture3, + "", + "-l $LOCAL_PATH"]) + shares = [ "vfs_aio_pthread_async_dosmode_default1", "vfs_aio_pthread_async_dosmode_default2" diff --git a/source3/torture/proto.h b/source3/torture/proto.h index 93d0727e936..0206cf15c9a 100644 --- a/source3/torture/proto.h +++ b/source3/torture/proto.h @@ -124,6 +124,7 @@ bool run_smb2_dfs_paths(int dummy); bool run_smb2_non_dfs_share(int dummy); bool run_smb2_dfs_share_non_dfs_path(int dummy); bool run_smb2_dfs_filename_leading_backslash(int dummy); +bool run_smb2_pipe_read_async_disconnect(int dummy); bool run_smb1_dfs_paths(int dummy); bool run_smb1_dfs_search_paths(int dummy); bool run_smb1_dfs_operations(int dummy); diff --git a/source3/torture/test_smb2.c b/source3/torture/test_smb2.c index 3cb6d13789a..672c38e2b55 100644 --- a/source3/torture/test_smb2.c +++ b/source3/torture/test_smb2.c @@ -5136,3 +5136,120 @@ bool run_smb2_dfs_filename_leading_backslash(int dummy) (void)smb2_dfs_delete(cli, dfs_filename_slash); return retval; } + +/* + * Ensure a named pipe async read followed by a disconnect + * doesn't crash the server (server crash checked for in + * containing test script: + * source3/script/tests/test_smbtorture_nocrash_s3.sh) + * BUG: https://bugzilla.samba.org/show_bug.cgi?id=15423 + */ + +bool run_smb2_pipe_read_async_disconnect(int dummy) +{ + struct cli_state *cli = NULL; + NTSTATUS status; + uint64_t fid_persistent = 0; + uint64_t fid_volatile = 0; + struct tevent_context *ev; + struct tevent_req *req; + bool retval = false; + + printf("Starting SMB2-PIPE-READ-ASYNC-DISCONNECT\n"); + + if (!torture_init_connection(&cli)) { + return false; + } + + status = smbXcli_negprot(cli->conn, + cli->timeout, + PROTOCOL_SMB2_02, + PROTOCOL_SMB3_11); + if (!NT_STATUS_IS_OK(status)) { + printf("smbXcli_negprot returned %s\n", nt_errstr(status)); + return false; + } + + status = cli_session_setup_creds(cli, torture_creds); + if (!NT_STATUS_IS_OK(status)) { + printf("cli_session_setup returned %s\n", nt_errstr(status)); + return false; + } + + status = cli_tree_connect_creds(cli, "IPC$", "IPC", torture_creds); + if (!NT_STATUS_IS_OK(status)) { + printf("cli_tree_connect to IPC$ returned %s\n", + nt_errstr(status)); + return false; + } + + /* Open the SAMR pipe. */ + status = smb2cli_create(cli->conn, + cli->timeout, + cli->smb2.session, + cli->smb2.tcon, + "SAMR", + SMB2_OPLOCK_LEVEL_NONE, /* oplock_level, */ + SMB2_IMPERSONATION_IMPERSONATION, /* impersonation_level, */ + SEC_STD_SYNCHRONIZE| + SEC_FILE_READ_DATA| + SEC_FILE_WRITE_DATA, /* desired_access, */ + FILE_ATTRIBUTE_NORMAL, /* file_attributes, */ + FILE_SHARE_READ|FILE_SHARE_WRITE|FILE_SHARE_DELETE, /* share_access, */ + FILE_OPEN, /* create_disposition, */ + 0, /* create_options, */ + NULL, /* smb2_create_blobs *blobs */ + &fid_persistent, + &fid_volatile, + NULL, /* struct smb_create_returns * */ + talloc_tos(), /* mem_ctx. */ + NULL, /* struct smb2_create_blobs * */ + NULL); /* psymlink */ + if (!NT_STATUS_IS_OK(status)) { + printf("%s:%d smb2cli_create on SAMR returned %s\n", + __FILE__, + __LINE__, + nt_errstr(status)); + goto err; + } + + ev = samba_tevent_context_init(talloc_tos()); + if (ev == NULL) { + goto err; + } + + /* Start an async read. */ + req = smb2cli_read_send(talloc_tos(), + ev, + cli->conn, + cli->timeout, + cli->smb2.session, + cli->smb2.tcon, + 16*1024, + 0, /* offset */ + fid_persistent, + fid_volatile, + 0, /* minimum_count */ + 0); /* remaining_bytes */ + if (req == NULL) { + goto err; + } + + /* Force disconnect. */ + smbXcli_conn_disconnect(cli->conn, NT_STATUS_LOCAL_DISCONNECT); + fid_volatile = 0; + retval = true; + + err: + + if (fid_volatile != 0) { + smb2cli_close(cli->conn, + cli->timeout, + cli->smb2.session, + cli->smb2.tcon, + 0, /* flags */ + fid_persistent, + fid_volatile); + } + return retval; +} diff --git a/source3/torture/torture.c b/source3/torture/torture.c index 2a8b8dec22f..9a1420c9e3a 100644 --- a/source3/torture/torture.c +++ b/source3/torture/torture.c @@ -15763,6 +15763,10 @@ static struct { .name = "SMB2-DFS-FILENAME-LEADING-BACKSLASH", .fn = run_smb2_dfs_filename_leading_backslash, }, + { + .name = "SMB2-PIPE-READ-ASYNC-DISCONNECT", + .fn = run_smb2_pipe_read_async_disconnect, + }, { .name = "SMB1-TRUNCATED-SESSSETUP", .fn = run_smb1_truncated_sesssetup, -- 2.39.3 From 9747b2e85da0c1613d3a98db23676d1df353eeaf Mon Sep 17 00:00:00 2001 From: Jeremy Allison Date: Tue, 19 Sep 2023 14:36:45 -0700 Subject: [PATCH 5/5] s3: smbd: Ensure we remove any pending aio values for named pipes on forced shutdown. Matches file and directory closes. Remove knownfail. BUG: https://bugzilla.samba.org/show_bug.cgi?id=15423 Signed-off-by: Jeremy Allison Reviewed-by: Ralph Boehme Autobuild-User(master): Jeremy Allison Autobuild-Date(master): Wed Sep 20 02:43:18 UTC 2023 on atb-devel-224 (cherry picked from commit 11280f1705c0faa1729f5aeaa1b6a1f79ab5a199) --- selftest/knownfail.d/smb2_pipe_read_async_disconnect | 1 - source3/smbd/close.c | 8 ++++++++ 2 files changed, 8 insertions(+), 1 deletion(-) delete mode 100644 selftest/knownfail.d/smb2_pipe_read_async_disconnect diff --git a/selftest/knownfail.d/smb2_pipe_read_async_disconnect b/selftest/knownfail.d/smb2_pipe_read_async_disconnect deleted file mode 100644 index 342a308adec..00000000000 --- a/selftest/knownfail.d/smb2_pipe_read_async_disconnect +++ /dev/null @@ -1 +0,0 @@ -^samba3.smbtorture_s3.plain.SMB2-PIPE-READ-ASYNC-DISCONNECT.check_panic$fileserver$ diff --git a/source3/smbd/close.c b/source3/smbd/close.c index 2b180e0c718..af5e78daa10 100644 --- a/source3/smbd/close.c +++ b/source3/smbd/close.c @@ -1630,6 +1630,14 @@ NTSTATUS close_file_smb(struct smb_request *req, SMB_ASSERT(fsp->stream_fsp == NULL); if (fsp->fake_file_handle != NULL) { + /* + * Named pipes are opened as fake files and + * can have pending aio requests. Ensure + * we clear out all pending aio on force + * shutdown of named pipes also. + * BUG: https://bugzilla.samba.org/show_bug.cgi?id=15423 + */ + assert_no_pending_aio(fsp, close_type); status = close_fake_file(req, fsp); } else if (fsp->print_file != NULL) { /* FIXME: return spool errors */ -- 2.39.3