From f4622de3a6021f970054f58e310dabab600872a8 Mon Sep 17 00:00:00 2001 From: Jeremy Allison Date: Tue, 2 Nov 2021 10:44:44 -0700 Subject: [PATCH 1/3] s3: smbd: dirfsp is being used uninitialized inside rmdir_internals(). MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Not caught be the tests in bugs 14878, 14879 as can_delete_directory_fsp() doesn't have the same bug. BUG: https://bugzilla.samba.org/show_bug.cgi?id=14892 Signed-off-by: Jeremy Allison Reviewed-by: Ralph Boehme Autobuild-User(master): Ralph Böhme Autobuild-Date(master): Wed Nov 3 14:33:49 UTC 2021 on sn-devel-184 (cherry picked from commit bbdcd66c048fee39629aeff450b50d049806e2f7) --- source3/smbd/close.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/source3/smbd/close.c b/source3/smbd/close.c index 7178257efcc..89e18d979ed 100644 --- a/source3/smbd/close.c +++ b/source3/smbd/close.c @@ -1055,6 +1055,8 @@ static NTSTATUS rmdir_internals(TALLOC_CTX *ctx, struct files_struct *fsp) goto err; } + dirfsp = dir_hnd_fetch_fsp(dir_hnd); + while ((dname = ReadDirName(dir_hnd, &dirpos, &st, &talloced)) != NULL) { struct smb_filename *smb_dname_full = NULL; struct smb_filename *direntry_fname = NULL; @@ -1203,7 +1205,6 @@ static NTSTATUS rmdir_internals(TALLOC_CTX *ctx, struct files_struct *fsp) /* Do a recursive delete. */ RewindDir(dir_hnd,&dirpos); - dirfsp = dir_hnd_fetch_fsp(dir_hnd); while ((dname = ReadDirName(dir_hnd, &dirpos, &st, &talloced)) != NULL) { struct smb_filename *direntry_fname = NULL; -- 2.30.2 From 4ca5e2e900aa7e38c3678d2026df7cfbabe0c702 Mon Sep 17 00:00:00 2001 From: Jeremy Allison Date: Wed, 3 Nov 2021 16:50:10 -0700 Subject: [PATCH 2/3] s3: smbtorture3: Add test for setting delete on close on a directory, then creating a file within to see if delete succeeds. Exposes an existing problem where "ret" is overwritten in the directory scan. Add knownfail. BUG: https://bugzilla.samba.org/show_bug.cgi?id=14892 Signed-off-by: Jeremy Allison Reviewed-by: Ralph Boehme (cherry picked from commit adfad6390962022277cc6aacaa388af86e46b71c) --- selftest/knownfail.d/del_on_close_nonempty | 1 + source3/selftest/tests.py | 15 +++ source3/torture/proto.h | 1 + source3/torture/test_smb2.c | 136 +++++++++++++++++++++ source3/torture/torture.c | 4 + 5 files changed, 157 insertions(+) create mode 100644 selftest/knownfail.d/del_on_close_nonempty diff --git a/selftest/knownfail.d/del_on_close_nonempty b/selftest/knownfail.d/del_on_close_nonempty new file mode 100644 index 00000000000..7109b993f91 --- /dev/null +++ b/selftest/knownfail.d/del_on_close_nonempty @@ -0,0 +1 @@ +^samba3.smbtorture_s3.plain.SMB2-DEL-ON-CLOSE-NONEMPTY.smbtorture\(fileserver\) diff --git a/source3/selftest/tests.py b/source3/selftest/tests.py index e39009635a6..60b3f18eded 100755 --- a/source3/selftest/tests.py +++ b/source3/selftest/tests.py @@ -256,6 +256,21 @@ plantestsuite("samba3.smbtorture_s3.plain.%s" % "SMB2-LIST-DIR-ASYNC", smbtorture3, "", "-l $LOCAL_PATH"]) +# +# SMB2-DEL-ON-CLOSE-NONEMPTY needs to run against a special fileserver share veto_files_delete +# +plantestsuite("samba3.smbtorture_s3.plain.%s" % "SMB2-DEL-ON-CLOSE-NONEMPTY", + "fileserver", + [os.path.join(samba3srcdir, + "script/tests/test_smbtorture_s3.sh"), + 'SMB2-DEL-ON-CLOSE-NONEMPTY', + '//$SERVER_IP/veto_files_delete', + '$USERNAME', + '$PASSWORD', + smbtorture3, + "", + "-l $LOCAL_PATH"]) + shares = [ diff --git a/source3/torture/proto.h b/source3/torture/proto.h index 4db267c92b0..65fa17523d8 100644 --- a/source3/torture/proto.h +++ b/source3/torture/proto.h @@ -120,6 +120,7 @@ bool run_smb2_sacl(int dummy); bool run_smb2_quota1(int dummy); bool run_smb2_stream_acl(int dummy); bool run_list_dir_async_test(int dummy); +bool run_delete_on_close_non_empty(int dummy); bool run_chain3(int dummy); bool run_local_conv_auth_info(int dummy); bool run_local_sprintf_append(int dummy); diff --git a/source3/torture/test_smb2.c b/source3/torture/test_smb2.c index b186ea4edac..0fac5125c08 100644 --- a/source3/torture/test_smb2.c +++ b/source3/torture/test_smb2.c @@ -3228,3 +3228,139 @@ bool run_list_dir_async_test(int dummy) (void)cli_rmdir(cli, dname); return ret; } + +/* + * Test delete a directory fails if a file is created + * in a directory after the delete on close is set. + * BUG: https://bugzilla.samba.org/show_bug.cgi?id=14892 + */ + +bool run_delete_on_close_non_empty(int dummy) +{ + struct cli_state *cli = NULL; + NTSTATUS status; + const char *dname = "DEL_ON_CLOSE_DIR"; + const char *fname = "DEL_ON_CLOSE_DIR\\testfile"; + uint16_t fnum = (uint16_t)-1; + uint16_t fnum1 = (uint16_t)-1; + bool ret = false; + + printf("SMB2 delete on close nonempty\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(cli, share, "?????", NULL); + if (!NT_STATUS_IS_OK(status)) { + printf("cli_tree_connect returned %s\n", nt_errstr(status)); + return false; + } + + /* Ensure directory doesn't exist. */ + (void)cli_unlink(cli, + fname, + FILE_ATTRIBUTE_SYSTEM | FILE_ATTRIBUTE_HIDDEN); + (void)cli_rmdir(cli, dname); + + /* Create target directory. */ + status = cli_ntcreate(cli, + dname, + 0, + DELETE_ACCESS|FILE_READ_DATA, + FILE_ATTRIBUTE_DIRECTORY, + FILE_SHARE_READ| + FILE_SHARE_WRITE| + FILE_SHARE_DELETE, + FILE_CREATE, + FILE_DIRECTORY_FILE, + 0, + &fnum, + NULL); + if (!NT_STATUS_IS_OK(status)) { + printf("cli_ntcreate for directory %s returned %s\n", + dname, + nt_errstr(status)); + goto out; + } + + /* Now set the delete on close bit. */ + status = cli_nt_delete_on_close(cli, fnum, 1); + if (!NT_STATUS_IS_OK(status)) { + printf("cli_cli_nt_delete_on_close set for directory " + "%s returned %s\n", + dname, + nt_errstr(status)); + goto out; + } + + /* Create file inside target directory. */ + /* + * NB. On Windows this will return NT_STATUS_DELETE_PENDING. Only on + * Samba will this succeed by default (the option "check parent + * directory delete on close" configures behaviour), but we're using + * this to test a race condition. + */ + status = cli_ntcreate(cli, + fname, + 0, + FILE_READ_DATA, + FILE_ATTRIBUTE_NORMAL, + FILE_SHARE_READ| + FILE_SHARE_WRITE| + FILE_SHARE_DELETE, + FILE_CREATE, + 0, + 0, + &fnum1, + NULL); + if (!NT_STATUS_IS_OK(status)) { + printf("cli_ntcreate for file %s returned %s\n", + fname, + nt_errstr(status)); + goto out; + } + cli_close(cli, fnum1); + fnum1 = (uint16_t)-1; + + /* Now the close should fail. */ + status = cli_close(cli, fnum); + if (!NT_STATUS_EQUAL(status, NT_STATUS_DIRECTORY_NOT_EMPTY)) { + printf("cli_close for directory %s returned %s\n", + dname, + nt_errstr(status)); + goto out; + } + + ret = true; + + out: + + if (fnum1 != (uint16_t)-1) { + cli_close(cli, fnum1); + } + if (fnum != (uint16_t)-1) { + cli_nt_delete_on_close(cli, fnum, 0); + cli_close(cli, fnum); + } + (void)cli_unlink(cli, + fname, + FILE_ATTRIBUTE_SYSTEM | FILE_ATTRIBUTE_HIDDEN); + (void)cli_rmdir(cli, dname); + return ret; +} diff --git a/source3/torture/torture.c b/source3/torture/torture.c index 79a9c65073c..197e1990e16 100644 --- a/source3/torture/torture.c +++ b/source3/torture/torture.c @@ -15249,6 +15249,10 @@ static struct { .name = "SMB2-LIST-DIR-ASYNC", .fn = run_list_dir_async_test, }, + { + .name = "SMB2-DEL-ON-CLOSE-NONEMPTY", + .fn = run_delete_on_close_non_empty, + }, { .name = "CLEANUP1", .fn = run_cleanup1, -- 2.30.2 From b4b4675beef95ec57b28f62fc9de9b084d2bf5fb Mon Sep 17 00:00:00 2001 From: Jeremy Allison Date: Wed, 3 Nov 2021 19:02:36 -0700 Subject: [PATCH 3/3] s3: smbd: Ensure in the directory scanning loops inside rmdir_internals() we don't overwrite the 'ret' variable. MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit If we overwrite with ret=0, we return NT_STATUS_OK even when we goto err. This function should be restructured to use NT_STATUS internally, and make 'int ret' transitory, but that's a patch for another time. Remove knownfail. BUG: BUG: https://bugzilla.samba.org/show_bug.cgi?id=14892 Signed-off-by: Jeremy Allison Reviewed-by: Ralph Boehme Autobuild-User(master): Ralph Böhme Autobuild-Date(master): Thu Nov 4 09:10:27 UTC 2021 on sn-devel-184 (cherry picked from commit 141f3f5f9a5ef556cc7864b2afbf8ad48b7ebe77) --- selftest/knownfail.d/del_on_close_nonempty | 1 - source3/smbd/close.c | 13 +++++++------ 2 files changed, 7 insertions(+), 7 deletions(-) delete mode 100644 selftest/knownfail.d/del_on_close_nonempty diff --git a/selftest/knownfail.d/del_on_close_nonempty b/selftest/knownfail.d/del_on_close_nonempty deleted file mode 100644 index 7109b993f91..00000000000 --- a/selftest/knownfail.d/del_on_close_nonempty +++ /dev/null @@ -1 +0,0 @@ -^samba3.smbtorture_s3.plain.SMB2-DEL-ON-CLOSE-NONEMPTY.smbtorture\(fileserver\) diff --git a/source3/smbd/close.c b/source3/smbd/close.c index 89e18d979ed..0e5f1958fa1 100644 --- a/source3/smbd/close.c +++ b/source3/smbd/close.c @@ -1061,6 +1061,7 @@ static NTSTATUS rmdir_internals(TALLOC_CTX *ctx, struct files_struct *fsp) struct smb_filename *smb_dname_full = NULL; struct smb_filename *direntry_fname = NULL; char *fullname = NULL; + int retval; if (ISDOT(dname) || ISDOTDOT(dname)) { TALLOC_FREE(talloced); @@ -1095,8 +1096,8 @@ static NTSTATUS rmdir_internals(TALLOC_CTX *ctx, struct files_struct *fsp) goto err; } - ret = SMB_VFS_LSTAT(conn, smb_dname_full); - if (ret != 0) { + retval = SMB_VFS_LSTAT(conn, smb_dname_full); + if (retval != 0) { int saved_errno = errno; TALLOC_FREE(talloced); TALLOC_FREE(fullname); @@ -1139,8 +1140,8 @@ static NTSTATUS rmdir_internals(TALLOC_CTX *ctx, struct files_struct *fsp) } /* Not a DFS link - could it be a dangling symlink ? */ - ret = SMB_VFS_STAT(conn, smb_dname_full); - if (ret == -1 && (errno == ENOENT || errno == ELOOP)) { + retval = SMB_VFS_STAT(conn, smb_dname_full); + if (retval == -1 && (errno == ENOENT || errno == ELOOP)) { /* * Dangling symlink. * Allow delete as "delete veto files = yes" @@ -1243,8 +1244,8 @@ static NTSTATUS rmdir_internals(TALLOC_CTX *ctx, struct files_struct *fsp) * Todo: use SMB_VFS_STATX() once that's available. */ - ret = SMB_VFS_LSTAT(conn, smb_dname_full); - if (ret != 0) { + retval = SMB_VFS_LSTAT(conn, smb_dname_full); + if (retval != 0) { goto err_break; } -- 2.30.2