[PATCH] Fix bug 13204
Jeremy Allison
jra at samba.org
Thu Aug 9 17:27:26 UTC 2018
On Thu, Aug 09, 2018 at 09:39:41PM +0530, Anoop C S wrote:
> On Thu, 2018-08-09 at 15:32 +0530, Anoop C S via samba-technical wrote:
> > On Thu, 2018-08-09 at 14:09 +0530, Anoop C S via samba-technical wrote:
> > > Hi,
> > >
> > > Please find the attached patch which fixes the following bug:
> > >
> > > https://bugzilla.samba.org/show_bug.cgi?id=13204 - rmdir on non-empty directory fails silently
> > >
> > > For discussion regarding this please refer the thread
> > > https://lists.samba.org/archive/samba-technical/2018-August/129491.html
> > >
> > > Reviews are appreciated.
> >
> > Attaching a slightly modified version of patch with changes only to commit message.
>
> Ah.. forgot to close the directory :-)
>
> Modified the patch to close fnum before returning NT_STATUS_DIRECTORY_NOT_EMPTY while trying to set
> DELETE_ON_CLOSE.
LGTM. I've also included your original base.deltest20c patch
in this set (with one deleted unused variable definition to
make it compile :-) but this also needs another test. Your
deltest20c checks that Windows does what we expect, but you
weren't testing that your new libsmb client code also worked :-).
The following set also includes an additional test inside
test_smbclient_s3.sh that exercises your new code (it works :-).
Passes local make test.
Can I get a second Team reviewer so I can push ?
Thanks,
Jeremy
-------------- next part --------------
From c8408abf0516536bfef414b31f8d99df8e4d30ca Mon Sep 17 00:00:00 2001
From: Anoop C S <anoopcs at redhat.com>
Date: Thu, 9 Aug 2018 12:28:41 +0530
Subject: [PATCH 1/3] s3/libsmb: Explicitly set delete_on_close token for rmdir
The current implementation of `rmdir` hopes to get the directory deleted
on closing last open handle when FILE_DELETE_ON_CLOSE is set on it. But
for non-empty directories Windows doesn't error out during an open call.
Following that we internally refuse to set initial delete_on_close while
opening a non-empty directory. This prevents us from trying to delete
the directory when last open handle is closed.
Instead of relying on FILE_DELETE_ON_CLOSE during an open we explicitly
set delete_on_close token on directory handle once it is available. This
ensures that NT_STATUS_DIRECTORY_NOT_EMPTY is returned for `rmdir` on
non-empty directories while closing open directory handle.
BUG: https://bugzilla.samba.org/show_bug.cgi?id=13204
Signed-off-by: Anoop C S <anoopcs at redhat.com>
Reviewed-by: Jeremy Allison <jra at samba.org>
---
source3/libsmb/cli_smb2_fnum.c | 9 ++++++++-
1 file changed, 8 insertions(+), 1 deletion(-)
diff --git a/source3/libsmb/cli_smb2_fnum.c b/source3/libsmb/cli_smb2_fnum.c
index 74f2f2ec4e4..3cf16b325dd 100644
--- a/source3/libsmb/cli_smb2_fnum.c
+++ b/source3/libsmb/cli_smb2_fnum.c
@@ -683,7 +683,7 @@ NTSTATUS cli_smb2_rmdir(struct cli_state *cli, const char *dname)
FILE_ATTRIBUTE_DIRECTORY, /* file attributes */
FILE_SHARE_READ|FILE_SHARE_WRITE|FILE_SHARE_DELETE, /* share_access */
FILE_OPEN, /* create_disposition */
- FILE_DIRECTORY_FILE|FILE_DELETE_ON_CLOSE, /* create_options */
+ FILE_DIRECTORY_FILE, /* create_options */
&fnum,
NULL);
@@ -711,6 +711,13 @@ NTSTATUS cli_smb2_rmdir(struct cli_state *cli, const char *dname)
if (!NT_STATUS_IS_OK(status)) {
return status;
}
+
+ status = cli_smb2_delete_on_close(cli, fnum, true);
+ if (!NT_STATUS_IS_OK(status)) {
+ cli_smb2_close_fnum(cli, fnum);
+ return status;
+ }
+
return cli_smb2_close_fnum(cli, fnum);
}
--
2.18.0.597.ga71716f1ad-goog
From fdb433379558a387702cf604e91e583cefeda1d3 Mon Sep 17 00:00:00 2001
From: Anoop C S <anoopcs at redhat.com>
Date: Thu, 9 Aug 2018 20:02:05 +0530
Subject: [PATCH 2/3] s4/torture: Add new test for DELETE_ON_CLOSE on non-empty
directories
BUG: https://bugzilla.samba.org/show_bug.cgi?id=13204
Signed-off-by: Anoop C S <anoopcs at redhat.com>
Reviewed-by: Jeremy Allison <jra at samba.org>
---
source4/torture/basic/delete.c | 87 ++++++++++++++++++++++++++++++++++
1 file changed, 87 insertions(+)
diff --git a/source4/torture/basic/delete.c b/source4/torture/basic/delete.c
index 6596985a4e7..a8c4e3fa3f1 100644
--- a/source4/torture/basic/delete.c
+++ b/source4/torture/basic/delete.c
@@ -2097,6 +2097,92 @@ static bool deltest20b(struct torture_context *tctx, struct smbcli_state *cli1,
return correct;
}
+/* Test 20c */
+/* Along the lines of deltest20 we try to open a non-empty directory with delete
+ * on close set and subsequent close to verify its presence in the tree.
+ */
+static bool deltest20c(struct torture_context *tctx, struct smbcli_state *cli1, struct smbcli_state *cli2)
+{
+ int fnum1 = -1;
+ int dnum1 = -1;
+ int ret;
+ char *fullname;
+
+ del_clean_area(cli1, cli2);
+
+ smbcli_deltree(cli1->tree, dname);
+
+ /* Firstly open and create with all access */
+ dnum1 = smbcli_nt_create_full(cli1->tree, dname, 0,
+ SEC_FILE_READ_DATA|
+ SEC_FILE_WRITE_DATA|
+ SEC_STD_DELETE,
+ FILE_ATTRIBUTE_DIRECTORY,
+ NTCREATEX_SHARE_ACCESS_READ|
+ NTCREATEX_SHARE_ACCESS_WRITE|
+ NTCREATEX_SHARE_ACCESS_DELETE,
+ NTCREATEX_DISP_CREATE,
+ NTCREATEX_OPTIONS_DIRECTORY, 0);
+ torture_assert(tctx, dnum1 != -1, talloc_asprintf(tctx, "open of %s failed: %s",
+ dname, smbcli_errstr(cli1->tree)));
+
+ /* And close - just to create the directory */
+ smbcli_close(cli1->tree, dnum1);
+
+ ret = asprintf(&fullname, "\\%s%s", dname, fname);
+ torture_assert(tctx, ret != -1, "asprintf failed");
+
+ /* Open and create with all access */
+ fnum1 = smbcli_nt_create_full(cli1->tree, fullname, 0,
+ SEC_RIGHTS_FILE_ALL,
+ FILE_ATTRIBUTE_NORMAL,
+ NTCREATEX_SHARE_ACCESS_READ|
+ NTCREATEX_SHARE_ACCESS_WRITE|
+ NTCREATEX_SHARE_ACCESS_DELETE,
+ NTCREATEX_DISP_CREATE,
+ 0, 0);
+ torture_assert(tctx, fnum1 != -1, talloc_asprintf(tctx, "open of %s failed: %s",
+ fname, smbcli_errstr(cli1->tree)));
+
+ /* And close - just to create the file. */
+ smbcli_close(cli1->tree, fnum1);
+
+ /* Open with all access, but add delete on close */
+ dnum1 = smbcli_nt_create_full(cli1->tree, dname, 0,
+ SEC_FILE_READ_DATA|
+ SEC_FILE_WRITE_DATA|
+ SEC_STD_DELETE,
+ FILE_ATTRIBUTE_DIRECTORY,
+ NTCREATEX_SHARE_ACCESS_READ|
+ NTCREATEX_SHARE_ACCESS_WRITE|
+ NTCREATEX_SHARE_ACCESS_DELETE,
+ NTCREATEX_DISP_OPEN,
+ NTCREATEX_OPTIONS_DIRECTORY|NTCREATEX_OPTIONS_DELETE_ON_CLOSE, 0);
+ /* Should work */
+ torture_assert(tctx, dnum1 != -1, talloc_asprintf(tctx, "open of %s failed: %s",
+ dname, smbcli_errstr(cli1->tree)));
+
+ smbcli_close(cli1->tree, dnum1);
+
+ /* Try to open again */
+ dnum1 = smbcli_nt_create_full(cli1->tree, dname, 0,
+ SEC_FILE_READ_DATA|
+ SEC_FILE_WRITE_DATA|
+ SEC_STD_DELETE,
+ FILE_ATTRIBUTE_DIRECTORY,
+ NTCREATEX_SHARE_ACCESS_READ|
+ NTCREATEX_SHARE_ACCESS_WRITE|
+ NTCREATEX_SHARE_ACCESS_DELETE,
+ NTCREATEX_DISP_OPEN,
+ NTCREATEX_OPTIONS_DIRECTORY, 0);
+ /* Directory should be still present*/
+ torture_assert(tctx, dnum1 != -1, talloc_asprintf(tctx, "open of %s failed: %s",
+ dname, smbcli_errstr(cli1->tree)));
+
+ smbcli_close(cli1->tree, dnum1);
+
+ return true;
+}
/* Test 21 ... */
static bool deltest21(struct torture_context *tctx)
@@ -2522,6 +2608,7 @@ struct torture_suite *torture_test_delete(TALLOC_CTX *ctx)
torture_suite_add_2smb_test(suite, "deltest20", deltest20);
torture_suite_add_2smb_test(suite, "deltest20a", deltest20a);
torture_suite_add_2smb_test(suite, "deltest20b", deltest20b);
+ torture_suite_add_2smb_test(suite, "deltest20c", deltest20c);
torture_suite_add_simple_test(suite, "deltest21", deltest21);
torture_suite_add_simple_test(suite, "deltest22", deltest22);
torture_suite_add_2smb_test(suite, "deltest23", deltest23);
--
2.18.0.597.ga71716f1ad-goog
From d6d12a81d4d1aaf5e527e4a253b19c6a8682ff71 Mon Sep 17 00:00:00 2001
From: Jeremy Allison <jra at samba.org>
Date: Thu, 9 Aug 2018 10:02:26 -0700
Subject: [PATCH 3/3] s3: tests: smbclient. Regression test to ensure we get
NT_STATUS_DIRECTORY_NOT_EMPTY on rmdir.
BUG: https://bugzilla.samba.org/show_bug.cgi?id=13204
Signed-off-by: Jeremy Allison <jra at samba.org>
---
source3/script/tests/test_smbclient_s3.sh | 42 +++++++++++++++++++++++
1 file changed, 42 insertions(+)
diff --git a/source3/script/tests/test_smbclient_s3.sh b/source3/script/tests/test_smbclient_s3.sh
index 9a5170ad664..bf033ccd2fb 100755
--- a/source3/script/tests/test_smbclient_s3.sh
+++ b/source3/script/tests/test_smbclient_s3.sh
@@ -1707,6 +1707,44 @@ EOF
fi
}
+# Test smbclient non-empty rmdir command
+test_del_nedir()
+{
+ tmpfile=$PREFIX/smbclient_interactive_prompt_commands
+ del_nedir="$LOCAL_PATH/del_nedir"
+
+ rm -rf $del_nedir
+ mkdir $del_nedir
+ touch $del_nedir/afile
+ cat > $tmpfile <<EOF
+rmdir del_nedir
+quit
+EOF
+ cmd='CLI_FORCE_INTERACTIVE=yes $SMBCLIENT "$@" -U$USERNAME%$PASSWORD //$SERVER/tmp -I $SERVER_IP $ADDARGS < $tmpfile 2>&1'
+ eval echo "$cmd"
+ out=`eval $cmd`
+ ret=$?
+ rm -rf $del_nedir
+
+ if [ $ret != 0 ] ; then
+ echo "$out"
+ echo "failed test_del_nedir test with output $ret"
+ false
+ return
+ fi
+
+# Should get NT_STATUS_DIRECTORY_NOT_EMPTY error from rmdir
+ echo "$out" | grep 'NT_STATUS_DIRECTORY_NOT_EMPTY'
+ ret=$?
+ if [ $ret -ne 0 ] ; then
+ echo "$out"
+ echo "test_del_nedir failed - should get an NT_STATUS_DIRECTORY_NOT_EMPTY error"
+ false
+ return
+ fi
+}
+
+#
#
LOGDIR_PREFIX=test_smbclient_s3
@@ -1851,4 +1889,8 @@ testit "rm -rf $LOGDIR" \
rm -rf $LOGDIR || \
failed=`expr $failed + 1`
+testit "delete a non empty directory" \
+ test_del_nedir || \
+ failed=`expr $failed + 1`
+
testok $0 $failed
--
2.18.0.597.ga71716f1ad-goog
More information about the samba-technical
mailing list