Attachment 17024 Details for Bug 14879 – patch for v4-14

[patch] patch for v4-14

samba-4.14-14879.patch (text/plain), 28.95 KB, created by Pavel Filipenský on 2021-11-24 11:56:12 UTC

(hide)

Description:

Filename:

MIME Type:

Creator: Pavel Filipenský

Created: 2021-11-24 11:56:12 UTC

Size: 28.95 KB

patch

obsolete

>From a0618e0eda69d31d090f776a9217a6a3cf76e5da Mon Sep 17 00:00:00 2001
>From: Jeremy Allison <jra@samba.org>
>Date: Thu, 21 Oct 2021 16:37:27 -0700
>Subject: [PATCH 1/7] s3: smbd: Add two tests showing the ability to delete a
> directory containing a dangling symlink over SMB2 depends on "delete veto
> files" setting.
>
>Add knownfail.
>
>BUG: https://bugzilla.samba.org/show_bug.cgi?id=14879
>
>Signed-off-by: Jeremy Allison <jra@samba.org>
>Reviewed-by: Ralph Boehme <slow@samba.org>
>(cherry picked from commit 942123b95923f35a32df4196a072a3ed3468396a)
>(cherry picked from commit 359517877d6462ff4398401748f921c8b79357a6)
>---
> selftest/knownfail.d/rmdir_dangle_symlink     |   1 +
> selftest/target/Samba3.pm                     |   4 +
> .../test_delete_veto_files_only_rmdir.sh      | 183 ++++++++++++++++++
> source3/selftest/tests.py                     |   3 +
> 4 files changed, 191 insertions(+)
> create mode 100644 selftest/knownfail.d/rmdir_dangle_symlink
> create mode 100755 source3/script/tests/test_delete_veto_files_only_rmdir.sh
>
>diff --git a/selftest/knownfail.d/rmdir_dangle_symlink b/selftest/knownfail.d/rmdir_dangle_symlink
>new file mode 100644
>index 00000000000..c775dc5fe15
>--- /dev/null
>+++ b/selftest/knownfail.d/rmdir_dangle_symlink
>@@ -0,0 +1 @@
>+^samba3.blackbox.test_dangle_rmdir.rmdir can delete directory containing dangling symlink$fileserver$
>diff --git a/selftest/target/Samba3.pm b/selftest/target/Samba3.pm
>index bbff9d74817..588d7779dd4 100755
>--- a/selftest/target/Samba3.pm
>+++ b/selftest/target/Samba3.pm
>@@ -1584,6 +1584,10 @@ sub setup_fileserver
> 	veto files = /veto_name*/
> 	delete veto files = yes
> 
>+[delete_veto_files_only]
>+	path = $veto_sharedir
>+	delete veto files = yes
>+
> [homes]
> 	comment = Home directories
> 	browseable = No
>diff --git a/source3/script/tests/test_delete_veto_files_only_rmdir.sh b/source3/script/tests/test_delete_veto_files_only_rmdir.sh
>new file mode 100755
>index 00000000000..d2c3b2198f7
>--- /dev/null
>+++ b/source3/script/tests/test_delete_veto_files_only_rmdir.sh
>@@ -0,0 +1,183 @@
>+#!/bin/sh
>+#
>+# Check smbclient can (or cannot) delete a directory containing dangling symlinks.
>+# BUG: https://bugzilla.samba.org/show_bug.cgi?id=14879
>+#
>+
>+if [ $# -lt 6 ]; then
>+cat <<EOF
>+Usage: $0 SERVER SERVER_IP USERNAME PASSWORD SHAREPATH SMBCLIENT
>+EOF
>+exit 1;
>+fi
>+
>+SERVER=${1}
>+SERVER_IP=${2}
>+USERNAME=${3}
>+PASSWORD=${4}
>+SHAREPATH=${5}
>+SMBCLIENT=${6}
>+shift 6
>+SMBCLIENT="$VALGRIND ${SMBCLIENT}"
>+ADDARGS="$@"
>+
>+incdir=$(dirname "$0")/../../../testprogs/blackbox
>+. $incdir/subunit.sh
>+
>+failed=0
>+
>+rmdir_path="$SHAREPATH/dir"
>+
>+#
>+# Using the share "[delete_veto_files_only]" we CAN delete
>+# a directory containing only a dangling symlink.
>+#
>+test_dangle_symlink_delete_veto_rmdir()
>+{
>+    local dangle_symlink_path="$rmdir_path/bad_link"
>+    local tmpfile=$PREFIX/smbclient.in.$$
>+
>+    # Create rmdir directory.
>+    mkdir -p "$rmdir_path"
>+    # Create dangling symlink underneath.
>+    ln -s "nowhere-foo" "$dangle_symlink_path"
>+
>+    cat > "$tmpfile" <<EOF
>+cd dir
>+ls
>+quit
>+EOF
>+
>+    local cmd='CLI_FORCE_INTERACTIVE=yes $SMBCLIENT //$SERVER/delete_veto_files_only -U$USERNAME%$PASSWORD $ADDARGS < $tmpfile 2>&1'
>+    eval echo "$cmd"
>+    out=$(eval "$cmd")
>+    ret=$?
>+
>+    # Check for smbclient error.
>+    if [ $ret != 0 ] ; then
>+        echo "Failed accessing share delete_veto_files_only - $ret"
>+        echo "$out"
>+        return 1
>+    fi
>+
>+    # We should NOT see the dangling symlink file.
>+    echo "$out" | grep bad_link
>+    ret=$?
>+    if [ $ret -eq 0 ] ; then
>+       echo "Saw dangling symlink bad_link in share delete_veto_files_only"
>+       echo "$out"
>+       return 1
>+    fi
>+
>+    # Try and remove the directory, should succeed.
>+    cat > "$tmpfile" <<EOF
>+rd dir
>+quit
>+EOF
>+
>+    local cmd='CLI_FORCE_INTERACTIVE=yes $SMBCLIENT //$SERVER/delete_veto_files_only -U$USERNAME%$PASSWORD $ADDARGS < $tmpfile 2>&1'
>+    eval echo "$cmd"
>+    out=$(eval "$cmd")
>+    ret=$?
>+
>+    # Check for smbclient error.
>+    if [ $ret != 0 ] ; then
>+        echo "Failed accessing share delete_veto_files_only - $ret"
>+        echo "$out"
>+        return 1
>+    fi
>+
>+    # We should get no NT_STATUS_ errors.
>+    echo "$out" | grep NT_STATUS_
>+    ret=$?
>+    if [ $ret -eq 0 ] ; then
>+       echo "Got error NT_STATUS_ in share delete_veto_files_only"
>+       echo "$out"
>+       return 1
>+    fi
>+
>+    return 0
>+}
>+
>+#
>+# Using the share "[veto_files_nodelete]" we CANNOT delete
>+# a directory containing only a dangling symlink.
>+#
>+test_dangle_symlink_veto_files_nodelete()
>+{
>+    local dangle_symlink_path="$rmdir_path/bad_link"
>+    local tmpfile=$PREFIX/smbclient.in.$$
>+
>+    # Create rmdir directory.
>+    mkdir -p "$rmdir_path"
>+    # Create dangling symlink underneath.
>+    ln -s "nowhere-foo" "$dangle_symlink_path"
>+
>+    cat > "$tmpfile" <<EOF
>+cd dir
>+ls
>+quit
>+EOF
>+
>+    local cmd='CLI_FORCE_INTERACTIVE=yes $SMBCLIENT //$SERVER/veto_files_nodelete -U$USERNAME%$PASSWORD $ADDARGS < $tmpfile 2>&1'
>+    eval echo "$cmd"
>+    out=$(eval "$cmd")
>+    ret=$?
>+
>+    # Check for smbclient error.
>+    if [ $ret != 0 ] ; then
>+        echo "Failed accessing share veto_files_nodelete - $ret"
>+        echo "$out"
>+        return 1
>+    fi
>+
>+    # We should NOT see the dangling symlink file.
>+    echo "$out" | grep bad_link
>+    ret=$?
>+    if [ $ret -eq 0 ] ; then
>+       echo "Saw dangling symlink bad_link in share veto_files_nodelete"
>+       echo "$out"
>+       return 1
>+    fi
>+
>+    # Try and remove the directory, should fail with DIRECTORY_NOT_EMPTY.
>+    cat > "$tmpfile" <<EOF
>+rd dir
>+quit
>+EOF
>+
>+    local cmd='CLI_FORCE_INTERACTIVE=yes $SMBCLIENT //$SERVER/veto_files_nodelete -U$USERNAME%$PASSWORD $ADDARGS < $tmpfile 2>&1'
>+    eval echo "$cmd"
>+    out=$(eval "$cmd")
>+    ret=$?
>+
>+    # Check for smbclient error.
>+    if [ $ret != 0 ] ; then
>+        echo "Failed accessing share veto_files_nodelete - $ret"
>+        echo "$out"
>+        return 1
>+    fi
>+
>+    # We should get NT_STATUS_DIRECTORY_NOT_EMPTY errors.
>+    echo "$out" | grep NT_STATUS_DIRECTORY_NOT_EMPTY
>+    ret=$?
>+    if [ $ret -ne 0 ] ; then
>+       echo "Should get NT_STATUS_DIRECTORY_NOT_EMPTY in share veto_files_nodelete"
>+       echo "$out"
>+       return 1
>+    fi
>+
>+    return 0
>+}
>+
>+
>+testit "rmdir can delete directory containing dangling symlink" \
>+   test_dangle_symlink_delete_veto_rmdir || failed=$(expr "$failed" + 1)
>+
>+rm -rf "$rmdir_path"
>+
>+testit "rmdir cannot delete directory delete_veto_files_no containing dangling symlink" \
>+   test_dangle_symlink_veto_files_nodelete || failed=$(expr "$failed" + 1)
>+
>+rm -rf "$rmdir_path"
>+exit "$failed"
>diff --git a/source3/selftest/tests.py b/source3/selftest/tests.py
>index 82f32ec4232..330024cf77c 100755
>--- a/source3/selftest/tests.py
>+++ b/source3/selftest/tests.py
>@@ -501,6 +501,9 @@ for env in ["fileserver"]:
>     plantestsuite("samba3.blackbox.test_veto_rmdir", env,
>                   [os.path.join(samba3srcdir, "script/tests/test_veto_rmdir.sh"),
>                   '$SERVER', '$SERVER_IP', '$USERNAME', '$PASSWORD', '$LOCAL_PATH/veto', smbclient3])
>+    plantestsuite("samba3.blackbox.test_dangle_rmdir", env,
>+                  [os.path.join(samba3srcdir, "script/tests/test_delete_veto_files_only_rmdir.sh"),
>+                  '$SERVER', '$SERVER_IP', '$USERNAME', '$PASSWORD', '$LOCAL_PATH/veto', smbclient3])
> 
>     #
>     # tar command tests
>-- 
>2.33.1
>
>
>From 47c98fe40101b60d5b5a34eb8ef02106c1da66c9 Mon Sep 17 00:00:00 2001
>From: Jeremy Allison <jra@samba.org>
>Date: Mon, 25 Oct 2021 12:01:58 -0700
>Subject: [PATCH 2/7] s3: VFS: streams_depot. Allow unlinkat to cope with
> dangling symlinks.
>
>BUG: https://bugzilla.samba.org/show_bug.cgi?id=14879
>
>Signed-off-by: Jeremy Allison <jra@samba.org>
>Reviewed-by: Ralph Boehme <slow@samba.org>
>(cherry picked from commit 295d7d026babe3cd5123d0f53adcb16868907f05)
>(backported from commit 7a4173809a87350bc3580240232978042ec2ceca)
>[pfilipen@redhat.com: code in 4.15 uses different variable name]
>---
> source3/modules/vfs_streams_depot.c | 10 ++++++++++
> 1 file changed, 10 insertions(+)
>
>diff --git a/source3/modules/vfs_streams_depot.c b/source3/modules/vfs_streams_depot.c
>index a5e02d5a069..dd6376e5fd0 100644
>--- a/source3/modules/vfs_streams_depot.c
>+++ b/source3/modules/vfs_streams_depot.c
>@@ -739,6 +739,16 @@ static int streams_depot_unlink_internal(vfs_handle_struct *handle,
> 		ret = SMB_VFS_NEXT_LSTAT(handle, smb_fname_base);
> 	} else {
> 		ret = SMB_VFS_NEXT_STAT(handle, smb_fname_base);
>+		if (ret == -1 && (errno == ENOENT || errno == ELOOP)) {
>+			if (VALID_STAT(smb_fname->st) &&
>+					S_ISLNK(smb_fname->st.st_ex_mode)) {
>+				/*
>+				 * Original name was a link - Could be
>+				 * trying to remove a dangling symlink.
>+				 */
>+				ret = SMB_VFS_NEXT_LSTAT(handle, smb_fname_base);
>+			}
>+		}
> 	}
> 
> 	if (ret == -1) {
>-- 
>2.33.1
>
>
>From 474a91d03527a15f7655be3866a9e5eaa405118f Mon Sep 17 00:00:00 2001
>From: Jeremy Allison <jra@samba.org>
>Date: Mon, 25 Oct 2021 12:02:43 -0700
>Subject: [PATCH 3/7] s3: VFS: xattr_tdb. Allow unlinkat to cope with dangling
> symlinks.
>
>BUG: https://bugzilla.samba.org/show_bug.cgi?id=14879
>
>Signed-off-by: Jeremy Allison <jra@samba.org>
>Reviewed-by: Ralph Boehme <slow@samba.org>
>(cherry picked from commit f254be19d6501a4f573843af97963e350a9ee2ed)
>(backported from commit 0dba0917fd97e975d1daab5b0828644d026c2bc5)
>[pfilipen@redhat.com: code in 4.15 uses different variable name]
>---
> source3/modules/vfs_xattr_tdb.c | 10 ++++++++++
> 1 file changed, 10 insertions(+)
>
>diff --git a/source3/modules/vfs_xattr_tdb.c b/source3/modules/vfs_xattr_tdb.c
>index d89a1dd0d7d..e06ff9639f1 100644
>--- a/source3/modules/vfs_xattr_tdb.c
>+++ b/source3/modules/vfs_xattr_tdb.c
>@@ -639,6 +639,16 @@ static int xattr_tdb_unlinkat(vfs_handle_struct *handle,
> 		ret = SMB_VFS_NEXT_LSTAT(handle, smb_fname_tmp);
> 	} else {
> 		ret = SMB_VFS_NEXT_STAT(handle, smb_fname_tmp);
>+		if (ret == -1 && (errno == ENOENT || errno == ELOOP)) {
>+			if (VALID_STAT(smb_fname->st) &&
>+					S_ISLNK(smb_fname->st.st_ex_mode)) {
>+				/*
>+				 * Original name was a link - Could be
>+				 * trying to remove a dangling symlink.
>+				 */
>+				ret = SMB_VFS_NEXT_LSTAT(handle, smb_fname_tmp);
>+			}
>+		}
> 	}
> 	if (ret == -1) {
> 		goto out;
>-- 
>2.33.1
>
>
>From 298a8dac1160ebba56cd84b7e25908e160b88f85 Mon Sep 17 00:00:00 2001
>From: Jeremy Allison <jra@samba.org>
>Date: Mon, 25 Oct 2021 12:21:37 -0700
>Subject: [PATCH 4/7] s3: smbd: Fix rmdir_internals() to do an early return if
> lp_delete_veto_files() is not set.
>
>Fix the comments to match what the code actually does. The
>exit at the end of the scan directory loop if we find a client
>visible filename is a change in behavior, but the previous
>behavior (not exist on visible filename, but delete it) was
>a bug and in non-tested code. Now it's testd.
>
>BUG: https://bugzilla.samba.org/show_bug.cgi?id=14879
>
>Signed-off-by: Jeremy Allison <jra@samba.org>
>Reviewed-by: Ralph Boehme <slow@samba.org>
>(cherry picked from commit a37d16e7c55f85e3f2c9c8614755ea6307092d5f)
>(backported from commit e00fe095e8cf7ab54bc82870b913762d2fdddbad)
>[pfilipen@redhat.com: rmdir_internals() got refactored in 4.15]
>---
> source3/smbd/close.c | 247 ++++++++++++++++++++++---------------------
> 1 file changed, 128 insertions(+), 119 deletions(-)
>
>diff --git a/source3/smbd/close.c b/source3/smbd/close.c
>index f05619d1886..0c102b9533b 100644
>--- a/source3/smbd/close.c
>+++ b/source3/smbd/close.c
>@@ -938,8 +938,11 @@ static NTSTATUS rmdir_internals(TALLOC_CTX *ctx, files_struct *fsp)
> {
> 	connection_struct *conn = fsp->conn;
> 	struct smb_filename *smb_dname = fsp->fsp_name;
>-	const struct loadparm_substitution *lp_sub =
>-		loadparm_s3_global_substitution();
>+	SMB_STRUCT_STAT st;
>+	const char *dname = NULL;
>+	char *talloced = NULL;
>+	long dirpos = 0;
>+	struct smb_Dir *dir_hnd = NULL;
> 	int ret;
> 
> 	SMB_ASSERT(!is_ntfs_stream_smb_fname(smb_dname));
>@@ -974,143 +977,149 @@ static NTSTATUS rmdir_internals(TALLOC_CTX *ctx, files_struct *fsp)
> 		return NT_STATUS_OK;
> 	}
> 
>-	if(((errno == ENOTEMPTY)||(errno == EEXIST)) && *lp_veto_files(talloc_tos(), lp_sub, SNUM(conn))) {
>-		/*
>-		 * Check to see if the only thing in this directory are
>-		 * vetoed files/directories. If so then delete them and
>-		 * retry. If we fail to delete any of them (and we *don't*
>-		 * do a recursive delete) then fail the rmdir.
>-		 */
>-		SMB_STRUCT_STAT st;
>-		const char *dname = NULL;
>-		char *talloced = NULL;
>-		long dirpos = 0;
>-		struct smb_Dir *dir_hnd = OpenDir(talloc_tos(), conn,
>-						  smb_dname, NULL,
>-						  0);
>-
>-		if(dir_hnd == NULL) {
>+	if (!((errno == ENOTEMPTY) || (errno == EEXIST))) {
>+		DEBUG(3,("rmdir_internals: couldn't remove directory %s : "
>+			 "%s\n", smb_fname_str_dbg(smb_dname),
>+			 strerror(errno)));
>+		return map_nt_error_from_unix(errno);
>+	}
>+
>+	/*
>+	 * Here we know the initial directory unlink failed with
>+	 * ENOTEMPTY or EEXIST so we know there are objects within.
>+	 * If we don't have permission to delete files non
>+	 * visible to the client just fail the directory delete.
>+	 */
>+
>+	if (!lp_delete_veto_files(SNUM(conn))) {
>+		errno = ENOTEMPTY;
>+		goto err;
>+	}
>+
>+	/*
>+	 * Check to see if the only thing in this directory are
>+	 * files non-visible to the client. If not, fail the delete.
>+	 */
>+
>+	dir_hnd = OpenDir(talloc_tos(), conn, smb_dname, NULL, 0);
>+	if (dir_hnd == NULL) {
>+		errno = ENOTEMPTY;
>+		goto err;
>+	}
>+
>+	while ((dname = ReadDirName(dir_hnd, &dirpos, &st, &talloced)) != NULL) {
>+		if((strcmp(dname, ".") == 0) || (strcmp(dname, "..")==0)) {
>+			TALLOC_FREE(talloced);
>+			continue;
>+		}
>+		if (!is_visible_file(conn,
>+					dir_hnd,
>+					dname,
>+					&st,
>+					false)) {
>+			TALLOC_FREE(talloced);
>+			continue;
>+		}
>+		if(!IS_VETO_PATH(conn, dname)) {
>+			/*
>+			 * We found a client visible name.
>+			 * We cannot delete this directory.
>+			 */
>+			DBG_DEBUG("got name %s - "
>+				"can't delete directory %s\n",
>+				dname,
>+				fsp_str_dbg(fsp));
>+			TALLOC_FREE(dir_hnd);
>+			TALLOC_FREE(talloced);
> 			errno = ENOTEMPTY;
> 			goto err;
> 		}
>+		TALLOC_FREE(talloced);
>+	}
> 
>-		while ((dname = ReadDirName(dir_hnd, &dirpos, &st,
>-					    &talloced)) != NULL) {
>-			if((strcmp(dname, ".") == 0) || (strcmp(dname, "..")==0)) {
>-				TALLOC_FREE(talloced);
>-				continue;
>-			}
>-			if (!is_visible_file(conn,
>-						dir_hnd,
>-						dname,
>-						&st,
>-						false)) {
>-				TALLOC_FREE(talloced);
>-				continue;
>-			}
>-			if(!IS_VETO_PATH(conn, dname)) {
>-				TALLOC_FREE(dir_hnd);
>-				TALLOC_FREE(talloced);
>-				errno = ENOTEMPTY;
>-				goto err;
>-			}
>+	/* Do a recursive delete. */
>+	RewindDir(dir_hnd,&dirpos);
>+	while ((dname = ReadDirName(dir_hnd, &dirpos, &st, &talloced)) != NULL) {
>+		struct smb_filename *smb_dname_full = NULL;
>+		char *fullname = NULL;
>+		bool do_break = true;
>+
>+		if (ISDOT(dname) || ISDOTDOT(dname)) {
>+			TALLOC_FREE(talloced);
>+			continue;
>+		}
>+		if (!is_visible_file(conn,
>+					dir_hnd,
>+					dname,
>+					&st,
>+					false)) {
> 			TALLOC_FREE(talloced);
>+			continue;
> 		}
> 
>-		/* We only have veto files/directories.
>-		 * Are we allowed to delete them ? */
>+		fullname = talloc_asprintf(ctx,
>+				"%s/%s",
>+				smb_dname->base_name,
>+				dname);
> 
>-		if(!lp_delete_veto_files(SNUM(conn))) {
>-			TALLOC_FREE(dir_hnd);
>-			errno = ENOTEMPTY;
>-			goto err;
>+		if (fullname == NULL) {
>+			errno = ENOMEM;
>+			goto err_break;
> 		}
> 
>-		/* Do a recursive delete. */
>-		RewindDir(dir_hnd,&dirpos);
>-		while ((dname = ReadDirName(dir_hnd, &dirpos, &st,
>-					    &talloced)) != NULL) {
>-			struct smb_filename *smb_dname_full = NULL;
>-			char *fullname = NULL;
>-			bool do_break = true;
>-
>-			if (ISDOT(dname) || ISDOTDOT(dname)) {
>-				TALLOC_FREE(talloced);
>-				continue;
>-			}
>-			if (!is_visible_file(conn,
>-						dir_hnd,
>-						dname,
>-						&st,
>-						false)) {
>-				TALLOC_FREE(talloced);
>-				continue;
>-			}
>-
>-			fullname = talloc_asprintf(ctx,
>-					"%s/%s",
>-					smb_dname->base_name,
>-					dname);
>+		smb_dname_full = synthetic_smb_fname(talloc_tos(),
>+						fullname,
>+						NULL,
>+						NULL,
>+						smb_dname->twrp,
>+						smb_dname->flags);
>+		if (smb_dname_full == NULL) {
>+			errno = ENOMEM;
>+			goto err_break;
>+		}
> 
>-			if(!fullname) {
>-				errno = ENOMEM;
>+		if(SMB_VFS_LSTAT(conn, smb_dname_full) != 0) {
>+			goto err_break;
>+		}
>+		if(smb_dname_full->st.st_ex_mode & S_IFDIR) {
>+			int retval;
>+			if(!recursive_rmdir(ctx, conn,
>+					    smb_dname_full)) {
> 				goto err_break;
> 			}
>-
>-			smb_dname_full = synthetic_smb_fname(talloc_tos(),
>-							fullname,
>-							NULL,
>-							NULL,
>-							smb_dname->twrp,
>-							smb_dname->flags);
>-			if (smb_dname_full == NULL) {
>-				errno = ENOMEM;
>+			retval = SMB_VFS_UNLINKAT(conn,
>+					conn->cwd_fsp,
>+					smb_dname_full,
>+					AT_REMOVEDIR);
>+			if(retval != 0) {
> 				goto err_break;
> 			}
>-
>-			if(SMB_VFS_LSTAT(conn, smb_dname_full) != 0) {
>+		} else {
>+			int retval = SMB_VFS_UNLINKAT(conn,
>+					conn->cwd_fsp,
>+					smb_dname_full,
>+					0);
>+			if(retval != 0) {
> 				goto err_break;
> 			}
>-			if(smb_dname_full->st.st_ex_mode & S_IFDIR) {
>-				int retval;
>-				if(!recursive_rmdir(ctx, conn,
>-						    smb_dname_full)) {
>-					goto err_break;
>-				}
>-				retval = SMB_VFS_UNLINKAT(conn,
>-						conn->cwd_fsp,
>-						smb_dname_full,
>-						AT_REMOVEDIR);
>-				if(retval != 0) {
>-					goto err_break;
>-				}
>-			} else {
>-				int retval = SMB_VFS_UNLINKAT(conn,
>-						conn->cwd_fsp,
>-						smb_dname_full,
>-						0);
>-				if(retval != 0) {
>-					goto err_break;
>-				}
>-			}
>+		}
> 
>-			/* Successful iteration. */
>-			do_break = false;
>+		/* Successful iteration. */
>+		do_break = false;
> 
>-		 err_break:
>-			TALLOC_FREE(fullname);
>-			TALLOC_FREE(smb_dname_full);
>-			TALLOC_FREE(talloced);
>-			if (do_break)
>-				break;
>-		}
>-		TALLOC_FREE(dir_hnd);
>-		/* Retry the rmdir */
>-		ret = SMB_VFS_UNLINKAT(conn,
>-				conn->cwd_fsp,
>-				smb_dname,
>-				AT_REMOVEDIR);
>+	err_break:
>+		TALLOC_FREE(fullname);
>+		TALLOC_FREE(smb_dname_full);
>+		TALLOC_FREE(talloced);
>+		if (do_break)
>+			break;
> 	}
>+	TALLOC_FREE(dir_hnd);
>+	/* Retry the rmdir */
>+	ret = SMB_VFS_UNLINKAT(conn,
>+			conn->cwd_fsp,
>+			smb_dname,
>+			AT_REMOVEDIR);
> 
>   err:
> 
>-- 
>2.33.1
>
>
>From a7075aeedd078c68b57556678fa40907cd66cd08 Mon Sep 17 00:00:00 2001
>From: Jeremy Allison <jra@samba.org>
>Date: Mon, 25 Oct 2021 12:32:29 -0700
>Subject: [PATCH 5/7] s3: smbd: Fix logic in rmdir_internals() to cope with
> dangling symlinks.
>
>Still need to add the same logic in can_delete_directory_fsp()
>before we can delete the knownfail.
>
>BUG: https://bugzilla.samba.org/show_bug.cgi?id=14879
>
>Signed-off-by: Jeremy Allison <jra@samba.org>
>Reviewed-by: Ralph Boehme <slow@samba.org>
>(cherry picked from commit 26fecad2e66e91a3913d88ee2e0889f266e91d89)
>(backported from commit 4793c4d5307472f0eb72f70f7dbf7324744e3f91)
>[pfilipen@redhat.com: rmdir_internals() got refactored in 4.15]
>---
> source3/smbd/close.c | 103 ++++++++++++++++++++++++++++++++++++++++++-
> 1 file changed, 101 insertions(+), 2 deletions(-)
>
>diff --git a/source3/smbd/close.c b/source3/smbd/close.c
>index 0c102b9533b..81811f703b0 100644
>--- a/source3/smbd/close.c
>+++ b/source3/smbd/close.c
>@@ -1008,10 +1008,14 @@ static NTSTATUS rmdir_internals(TALLOC_CTX *ctx, files_struct *fsp)
> 	}
> 
> 	while ((dname = ReadDirName(dir_hnd, &dirpos, &st, &talloced)) != NULL) {
>+		struct smb_filename *smb_dname_full = NULL;
>+		char *fullname = NULL;
>+
> 		if((strcmp(dname, ".") == 0) || (strcmp(dname, "..")==0)) {
> 			TALLOC_FREE(talloced);
> 			continue;
> 		}
>+
> 		if (!is_visible_file(conn,
> 					dir_hnd,
> 					dname,
>@@ -1020,6 +1024,98 @@ static NTSTATUS rmdir_internals(TALLOC_CTX *ctx, files_struct *fsp)
> 			TALLOC_FREE(talloced);
> 			continue;
> 		}
>+
>+		fullname = talloc_asprintf(talloc_tos(),
>+					   "%s/%s",
>+					   smb_dname->base_name,
>+					   dname);
>+
>+		if (fullname == NULL) {
>+			TALLOC_FREE(talloced);
>+			errno = ENOMEM;
>+			goto err;
>+		}
>+
>+		smb_dname_full = synthetic_smb_fname(talloc_tos(),
>+						     fullname,
>+						     NULL,
>+						     NULL,
>+						     smb_dname->twrp,
>+						     smb_dname->flags);
>+		if (smb_dname_full == NULL) {
>+			TALLOC_FREE(talloced);
>+			TALLOC_FREE(fullname);
>+			errno = ENOMEM;
>+			goto err;
>+		}
>+
>+		ret = SMB_VFS_LSTAT(conn, smb_dname_full);
>+		if (ret != 0) {
>+			int saved_errno = errno;
>+			TALLOC_FREE(talloced);
>+			TALLOC_FREE(fullname);
>+			TALLOC_FREE(smb_dname_full);
>+			errno = saved_errno;
>+			goto err;
>+		}
>+
>+		if (S_ISLNK(smb_dname_full->st.st_ex_mode)) {
>+			/* Could it be an msdfs link ? */
>+			if (lp_host_msdfs() &&
>+				lp_msdfs_root(SNUM(conn))) {
>+				struct smb_filename *smb_atname;
>+				smb_atname = synthetic_smb_fname(talloc_tos(),
>+							dname,
>+							NULL,
>+							&smb_dname_full->st,
>+							fsp->fsp_name->twrp,
>+							fsp->fsp_name->flags);
>+				if (smb_atname == NULL) {
>+					TALLOC_FREE(talloced);
>+					TALLOC_FREE(fullname);
>+					TALLOC_FREE(smb_dname_full);
>+					errno = ENOMEM;
>+					goto err;
>+				}
>+				if (is_msdfs_link(conn, smb_atname)) {
>+					TALLOC_FREE(talloced);
>+					TALLOC_FREE(fullname);
>+					TALLOC_FREE(smb_dname_full);
>+					TALLOC_FREE(smb_atname);
>+					DBG_DEBUG("got msdfs link name %s "
>+						"- can't delete directory %s\n",
>+						dname,
>+						fsp_str_dbg(fsp));
>+					errno = ENOTEMPTY;
>+					goto err;
>+				}
>+				TALLOC_FREE(smb_atname);
>+			}
>+
>+			/* 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)) {
>+				/*
>+				 * Dangling symlink.
>+				 * Allow delete as "delete veto files = yes"
>+				 */
>+				TALLOC_FREE(talloced);
>+				TALLOC_FREE(fullname);
>+				TALLOC_FREE(smb_dname_full);
>+				continue;
>+			}
>+
>+			DBG_DEBUG("got symlink name %s - "
>+				"can't delete directory %s\n",
>+				dname,
>+				fsp_str_dbg(fsp));
>+			TALLOC_FREE(talloced);
>+			TALLOC_FREE(fullname);
>+			TALLOC_FREE(smb_dname_full);
>+			errno = ENOTEMPTY;
>+			goto err;
>+		}
>+
> 		if(!IS_VETO_PATH(conn, dname)) {
> 			/*
> 			 * We found a client visible name.
>@@ -1029,12 +1125,15 @@ static NTSTATUS rmdir_internals(TALLOC_CTX *ctx, files_struct *fsp)
> 				"can't delete directory %s\n",
> 				dname,
> 				fsp_str_dbg(fsp));
>-			TALLOC_FREE(dir_hnd);
> 			TALLOC_FREE(talloced);
>+			TALLOC_FREE(fullname);
>+			TALLOC_FREE(smb_dname_full);
> 			errno = ENOTEMPTY;
> 			goto err;
> 		}
> 		TALLOC_FREE(talloced);
>+		TALLOC_FREE(fullname);
>+		TALLOC_FREE(smb_dname_full);
> 	}
> 
> 	/* Do a recursive delete. */
>@@ -1114,7 +1213,6 @@ static NTSTATUS rmdir_internals(TALLOC_CTX *ctx, files_struct *fsp)
> 		if (do_break)
> 			break;
> 	}
>-	TALLOC_FREE(dir_hnd);
> 	/* Retry the rmdir */
> 	ret = SMB_VFS_UNLINKAT(conn,
> 			conn->cwd_fsp,
>@@ -1123,6 +1221,7 @@ static NTSTATUS rmdir_internals(TALLOC_CTX *ctx, files_struct *fsp)
> 
>   err:
> 
>+	TALLOC_FREE(dir_hnd);
> 	if (ret != 0) {
> 		DEBUG(3,("rmdir_internals: couldn't remove directory %s : "
> 			 "%s\n", smb_fname_str_dbg(smb_dname),
>-- 
>2.33.1
>
>
>From 843fc3b857cdfd6c7e902acef933d17690815e7e Mon Sep 17 00:00:00 2001
>From: Jeremy Allison <jra@samba.org>
>Date: Mon, 25 Oct 2021 12:36:57 -0700
>Subject: [PATCH 6/7] s3: smbd: Fix logic in can_delete_directory_fsp() to cope
> with dangling symlinks.
>
>Remove knownfail.
>
>BUG: https://bugzilla.samba.org/show_bug.cgi?id=14879
>
>Signed-off-by: Jeremy Allison <jra@samba.org>
>Reviewed-by: Ralph Boehme <slow@samba.org>
>(cherry picked from commit e9ef970eee5eca8ab3720279c54098e91d2dfda9)
>(backported from commit 5023dbc04bfad7cc39e8c4de96f40c82e7a0288e)
>[pfilipen@redhat.com: can_delete_directory_fsp() got refactored in 4.15]
>---
> selftest/knownfail.d/rmdir_dangle_symlink |  1 -
> source3/smbd/dir.c                        | 97 +++++++++++++++++++++++
> 2 files changed, 97 insertions(+), 1 deletion(-)
> delete mode 100644 selftest/knownfail.d/rmdir_dangle_symlink
>
>diff --git a/selftest/knownfail.d/rmdir_dangle_symlink b/selftest/knownfail.d/rmdir_dangle_symlink
>deleted file mode 100644
>index c775dc5fe15..00000000000
>--- a/selftest/knownfail.d/rmdir_dangle_symlink
>+++ /dev/null
>@@ -1 +0,0 @@
>-^samba3.blackbox.test_dangle_rmdir.rmdir can delete directory containing dangling symlink$fileserver$
>diff --git a/source3/smbd/dir.c b/source3/smbd/dir.c
>index 545c7499f28..96edc8678e5 100644
>--- a/source3/smbd/dir.c
>+++ b/source3/smbd/dir.c
>@@ -1876,6 +1876,8 @@ NTSTATUS can_delete_directory_fsp(files_struct *fsp)
> 	char *talloced = NULL;
> 	SMB_STRUCT_STAT st;
> 	struct connection_struct *conn = fsp->conn;
>+	struct smb_filename *smb_dname = fsp->fsp_name;
>+	int ret;
> 	struct smb_Dir *dir_hnd = OpenDir(talloc_tos(),
> 					conn,
> 					fsp->fsp_name,
>@@ -1887,6 +1889,9 @@ NTSTATUS can_delete_directory_fsp(files_struct *fsp)
> 	}
> 
> 	while ((dname = ReadDirName(dir_hnd, &dirpos, &st, &talloced))) {
>+		struct smb_filename *smb_dname_full = NULL;
>+		char *fullname = NULL;
>+
> 		if (ISDOT(dname) || (ISDOTDOT(dname))) {
> 			TALLOC_FREE(talloced);
> 			continue;
>@@ -1901,6 +1906,98 @@ NTSTATUS can_delete_directory_fsp(files_struct *fsp)
> 			continue;
> 		}
> 
>+		fullname = talloc_asprintf(talloc_tos(),
>+					   "%s/%s",
>+					   smb_dname->base_name,
>+					   dname);
>+
>+		if (fullname == NULL) {
>+			TALLOC_FREE(dir_hnd);
>+			TALLOC_FREE(talloced);
>+			return NT_STATUS_NO_MEMORY;
>+		}
>+
>+		smb_dname_full = synthetic_smb_fname(talloc_tos(),
>+						     fullname,
>+						     NULL,
>+						     NULL,
>+						     smb_dname->twrp,
>+						     smb_dname->flags);
>+		if (smb_dname_full == NULL) {
>+			TALLOC_FREE(dir_hnd);
>+			TALLOC_FREE(talloced);
>+			TALLOC_FREE(fullname);
>+			return NT_STATUS_NO_MEMORY;
>+		}
>+
>+		ret = SMB_VFS_LSTAT(conn, smb_dname_full);
>+		if (ret != 0) {
>+			TALLOC_FREE(dir_hnd);
>+			TALLOC_FREE(talloced);
>+			TALLOC_FREE(fullname);
>+			TALLOC_FREE(smb_dname_full);
>+			return map_nt_error_from_unix(errno);
>+		}
>+
>+		if (S_ISLNK(smb_dname_full->st.st_ex_mode)) {
>+			/* Could it be an msdfs link ? */
>+			if (lp_host_msdfs() &&
>+			    lp_msdfs_root(SNUM(conn))) {
>+				struct smb_filename *smb_atname;
>+				smb_atname = synthetic_smb_fname(talloc_tos(),
>+							dname,
>+							NULL,
>+							&smb_dname_full->st,
>+							fsp->fsp_name->twrp,
>+							fsp->fsp_name->flags);
>+				if (smb_atname == NULL) {
>+					TALLOC_FREE(dir_hnd);
>+					TALLOC_FREE(talloced);
>+					TALLOC_FREE(fullname);
>+					TALLOC_FREE(smb_dname_full);
>+					return NT_STATUS_NO_MEMORY;
>+				}
>+				if (is_msdfs_link(conn, smb_atname)) {
>+					TALLOC_FREE(dir_hnd);
>+					TALLOC_FREE(talloced);
>+					TALLOC_FREE(fullname);
>+					TALLOC_FREE(smb_dname_full);
>+					TALLOC_FREE(smb_atname);
>+					DBG_DEBUG("got msdfs link name %s "
>+						"- can't delete directory %s\n",
>+						dname,
>+						fsp_str_dbg(fsp));
>+					return NT_STATUS_DIRECTORY_NOT_EMPTY;
>+				}
>+				TALLOC_FREE(smb_atname);
>+			}
>+
>+			/* 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)) {
>+				/*
>+				 * Dangling symlink.
>+				 * Allow if "delete veto files = yes"
>+				 */
>+				if (lp_delete_veto_files(SNUM(conn))) {
>+					TALLOC_FREE(talloced);
>+					TALLOC_FREE(fullname);
>+					TALLOC_FREE(smb_dname_full);
>+					continue;
>+				}
>+			}
>+
>+			DBG_DEBUG("got symlink name %s - "
>+				"can't delete directory %s\n",
>+				dname,
>+				fsp_str_dbg(fsp));
>+			TALLOC_FREE(dir_hnd);
>+			TALLOC_FREE(talloced);
>+			TALLOC_FREE(fullname);
>+			TALLOC_FREE(smb_dname_full);
>+			return NT_STATUS_DIRECTORY_NOT_EMPTY;
>+		}
>+
> 		DEBUG(10,("got name %s - can't delete\n",
> 			 dname ));
> 		status = NT_STATUS_DIRECTORY_NOT_EMPTY;
>-- 
>2.33.1
>
>
>From 7189ea825d8c9e4777dba737227ead602ad9b651 Mon Sep 17 00:00:00 2001
>From: Jeremy Allison <jra@samba.org>
>Date: Mon, 25 Oct 2021 12:42:02 -0700
>Subject: [PATCH 7/7] s3: docs-xml: Clarify the "delete veto files" paramter.
>MIME-Version: 1.0
>Content-Type: text/plain; charset=UTF-8
>Content-Transfer-Encoding: 8bit
>
>BUG: https://bugzilla.samba.org/show_bug.cgi?id=14879
>
>Signed-off-by: Jeremy Allison <jra@samba.org>
>Reviewed-by: Ralph Boehme <slow@samba.org>
>
>Autobuild-User(master): Ralph BÃ¶hme <slow@samba.org>
>Autobuild-Date(master): Fri Oct 29 14:57:14 UTC 2021 on sn-devel-184
>
>(cherry picked from commit 0b818c6b77e972626d0b071bebcf4ce55619fb84)
>(cherry picked from commit a549dc219cba5bd61969e4919ae4142f52c133ea)
>---
> docs-xml/smbdotconf/filename/deletevetofiles.xml | 9 ++++++---
> 1 file changed, 6 insertions(+), 3 deletions(-)
>
>diff --git a/docs-xml/smbdotconf/filename/deletevetofiles.xml b/docs-xml/smbdotconf/filename/deletevetofiles.xml
>index 581dc05396d..570d4ac60a0 100644
>--- a/docs-xml/smbdotconf/filename/deletevetofiles.xml
>+++ b/docs-xml/smbdotconf/filename/deletevetofiles.xml
>@@ -4,9 +4,12 @@
>                  xmlns:samba="http://www.samba.org/samba/DTD/samba-doc">
> <description>
> 	<para>This option is used when Samba is attempting to 
>-	delete a directory that contains one or more vetoed directories 
>-	(see the <smbconfoption name="veto files"/>
>-	option).  If this option is set to <constant>no</constant> (the default) then if a vetoed 
>+	delete a directory that contains one or more vetoed files
>+	or directories or non-visible files or directories (such
>+	as dangling symlinks that point nowhere).
>+	(see the <smbconfoption name="veto files"/>, <smbconfoption name="hide special files"/>,
>+	<smbconfoption name="hide unreadable"/>, <smbconfoption name="hide unwriteable files"/>
>+	options).  If this option is set to <constant>no</constant> (the default) then if a vetoed
> 	directory contains any non-vetoed files or directories then the 
> 	directory delete will fail. This is usually what you want.</para>
> 
>-- 
>2.33.1
>

Flags:

jra: review+
slow: review+

Actions: View

Attachments on bug 14879: 16863 | 16900 | 17024