Merge ~ahasenack/ubuntu/+source/samba:cosmic-samba-rmdir-1795772 into ubuntu/+source/samba:ubuntu/devel

[Andreas Hasenack]

This MP is superseeding Karl's one at https://code.launchpad.net/~kstenerud/ubuntu/+source/samba/+git/samba/+merge/356153, since he won't be able to finish that work before final freeze.

Bileto ticket: https://bileto.ubuntu.com/#/ticket/3463
PPA (still building): ppa:ci-train-ppa-service/3463

This branch fixes the case where rmdir fails to remove a non-empty directory but returns successfully, indicating to the caller that all went well. It's a bug in the library and affects many client tools, like smbclient and GUI file browsers.

Testing instructions, taken from Karl's work:
# lxc launch ubuntu-daily:cosmic sambatest
# lxc exec sambatest bash

# apt update
# apt dist-upgrade -y
# apt install -y samba samba-client
# mkdir -p /tmp/shared/testdir/testsubdir
# chown -R nobody:nogroup /tmp/shared
# printf "%s" "[global]
    workgroup = WORKGROUP
    server role = standalone server
    security = user
    map to guest = Bad User
    guest ok = yes
    guest only = yes
    guest account = nobody
[shared]
    path = /tmp/shared
    writable = yes
    browsable = yes
    guest ok = yes
" | tee "/etc/samba/smb.conf" >/dev/null
# service smbd restart
# CLI_FORCE_INTERACTIVE=yes smbclient -U " "%" " //localhost/shared <<EOF
rmdir testdir
quit
EOF

* This will not report an error, but it should.

With the updated packages, the smbclient call will correctly report that the directory is not empty.

[Andreas Hasenack (ahasenack)]

Only gvfs failed on s390x so far, all other dep8 tests passed. I clicked retry, let's see tomorrow.

[Christian Ehrhardt  (paelzer)]

Don't care about s390x GVFS andreas
=> http://autopkgtest.ubuntu.com/packages/g/gvfs/cosmic/s390x

And
ubuntu-release:61:force-badtest gvfs/1.36.1-0ubuntu3/ppc64el gvfs/1.36.1-0ubuntu3/s390x gvfs/1.38.0-2ubuntu2/s390x

So consider the tests to be good.

[Christian Ehrhardt  (paelzer)]

Also the MP LGTM

[Christian Ehrhardt  (paelzer)]

Tested from the PPA:
- testcase is fine
- also works if the subelements are files instead of dirs
- rmdir woorks as expected if the dir is empty

Overall +1

review: Approve

[Andreas Hasenack (ahasenack)]

Thanks, tagged and uploaded

diff --git a/debian/changelog b/debian/changelog
index cbab972..2c638a4 100644
--- a/debian/changelog
+++ b/debian/changelog
@@ -1,3 +1,11 @@
+samba (2:4.8.4+dfsg-2ubuntu2) cosmic; urgency=high
+
+  [ Karl Stenerud ]
+  * d/p/fix-rmdir.patch: Fix to make the samba client library report
+    directory-not-empty errors (LP: #1795772)
+
+ -- Andreas Hasenack <[e-mail]>  Tue, 09 Oct 2018 14:32:16 -0300
+
 samba (2:4.8.4+dfsg-2ubuntu1) cosmic; urgency=medium
 
   * Merge with Debian unstable (LP: #1778125). Remaining changes:
diff --git a/debian/patches/fix-rmdir.patch b/debian/patches/fix-rmdir.patch
 new file mode 100644
index 0000000..2a03d5b
--- /dev/null
+++ b/debian/patches/fix-rmdir.patch
@@ -0,0 +1,48 @@
+Description: 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.
+
+Author: Anoop C S <[e-mail]>
+Origin: upstream, https://github.com/samba-team/samba/commit/6b68e3eca631c04d6d57c489daf60f64732fc86d
+Bug: https://bugzilla.samba.org/show_bug.cgi?id=13204
+Bug-Ubuntu: https://bugs.launchpad.net/ubuntu/+source/samba/+bug/1795772
+Last-Update: 2018-10-09
+---
+This patch header follows DEP-3: http://dep.debian.net/deps/dep3/
+diff --git a/source3/libsmb/cli_smb2_fnum.c b/source3/libsmb/cli_smb2_fnum.c
+index c397b29b3..7b2f39c02 100644
+--- a/source3/libsmb/cli_smb2_fnum.c
++++ b/source3/libsmb/cli_smb2_fnum.c
+@@ -682,7 +682,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);
+ 
+@@ -710,6 +710,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);
+ }
+ 
diff --git a/debian/patches/series b/debian/patches/series
index c3ca4c2..28d241e 100644
--- a/debian/patches/series
+++ b/debian/patches/series
@@ -9,3 +9,4 @@ heimdal-rfc3454.txt
 Fix-pidl-manpage-sections.patch
 Fix-spelling.patch
 Improve-vfs_linux_xfs_sgid-manpage.patch
+fix-rmdir.patch