From 691525073e0c9dfb7287769aaa347f79118957d3 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Pavel=20Filipensk=C3=BD?= <pfilipensky@samba.org>
Date: Thu, 28 Nov 2024 18:39:53 +0100
Subject: [PATCH 1/6] s3:vfs_crossrename: avoid locking panic in copy_reg()
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit
Use low level backend functions that don't go through the FSA layer.
Done via calling transfer_file() as it was in version before 5c18f07
BUG: https://bugzilla.samba.org/show_bug.cgi?id=15724
Signed-off-by: Pavel Filipenský <pfilipensky@samba.org>
---
source3/modules/vfs_crossrename.c | 125 ++++++++++++++++++++----------
1 file changed, 84 insertions(+), 41 deletions(-)
diff --git a/source3/modules/vfs_crossrename.c b/source3/modules/vfs_crossrename.c
index 79f13c28da59..c260bdee1948 100644
--- a/source3/modules/vfs_crossrename.c
+++ b/source3/modules/vfs_crossrename.c
@@ -54,10 +54,12 @@ static NTSTATUS copy_reg(vfs_handle_struct *handle,
struct files_struct *dstfsp,
const struct smb_filename *dest)
{
- NTSTATUS status;
- struct smb_filename *full_fname_src = NULL;
- struct smb_filename *full_fname_dst = NULL;
+ NTSTATUS status = NT_STATUS_OK;
int ret;
+ off_t off;
+ int ifd = -1;
+ int ofd = -1;
+ struct timespec ts[2];
if (!VALID_STAT(source->st)) {
status = NT_STATUS_OBJECT_PATH_NOT_FOUND;
@@ -79,21 +81,6 @@ static NTSTATUS copy_reg(vfs_handle_struct *handle,
goto out;
}
- full_fname_src = full_path_from_dirfsp_atname(talloc_tos(),
- srcfsp,
- source);
- if (full_fname_src == NULL) {
- status = NT_STATUS_NO_MEMORY;
- goto out;
- }
- full_fname_dst = full_path_from_dirfsp_atname(talloc_tos(),
- dstfsp,
- dest);
- if (full_fname_dst == NULL) {
- status = NT_STATUS_NO_MEMORY;
- goto out;
- }
-
ret = SMB_VFS_NEXT_UNLINKAT(handle,
dstfsp,
dest,
@@ -103,40 +90,96 @@ static NTSTATUS copy_reg(vfs_handle_struct *handle,
goto out;
}
+ ifd = openat(fsp_get_pathref_fd(srcfsp),
+ source->base_name,
+ O_RDONLY,
+ 0);
+ if (ifd < 0) {
+ status = map_nt_error_from_unix(errno);
+ goto out;
+ }
+
+ ofd = openat(fsp_get_pathref_fd(dstfsp),
+ dest->base_name,
+ O_WRONLY | O_CREAT | O_TRUNC | O_NOFOLLOW,
+ 0600);
+ if (ofd < 0) {
+ status = map_nt_error_from_unix(errno);
+ goto out;
+ }
+
+ off = transfer_file(ifd, ofd, source->st.st_ex_size);
+ if (off == -1) {
+ status = map_nt_error_from_unix(errno);
+ goto out;
+ }
+
+ ret = fchown(ofd, source->st.st_ex_uid, source->st.st_ex_gid);
+ if (ret == -1 && errno != EPERM) {
+ status = map_nt_error_from_unix(errno);
+ goto out;
+ }
+
/*
- * copy_internals() takes attribute values from the NTrename call.
- *
- * From MS-CIFS:
- *
- * "If the attribute is 0x0000, then only normal files are renamed.
- * If the system file or hidden attributes are specified, then the
- * rename is inclusive of both special types."
+ * fchown turns off set[ug]id bits for non-root,
+ * so do the chmod last.
*/
- status = copy_internals(talloc_tos(),
- handle->conn,
- NULL,
- srcfsp, /* src_dirfsp */
- full_fname_src,
- dstfsp, /* dst_dirfsp */
- full_fname_dst,
- FILE_ATTRIBUTE_HIDDEN | FILE_ATTRIBUTE_SYSTEM);
- if (!NT_STATUS_IS_OK(status)) {
+ ret = fchmod(ofd, source->st.st_ex_mode & 07777);
+ if (ret == -1 && errno != EPERM) {
+ status = map_nt_error_from_unix(errno);
goto out;
}
- ret = SMB_VFS_NEXT_UNLINKAT(handle,
- srcfsp,
- source,
- 0);
+ /* Try to copy the old file's modtime and access time. */
+ ts[0] = source->st.st_ex_atime;
+ ts[1] = source->st.st_ex_mtime;
+ ret = futimens(ofd, ts);
+ if (ret == -1) {
+ DBG_DEBUG("Updating the time stamp on destinaton '%s' failed "
+ "with '%s'. Rename operation can continue.\n",
+ dest->base_name,
+ strerror(errno));
+ }
+
+ ret = close(ifd);
+ if (ret == -1) {
+ status = map_nt_error_from_unix(errno);
+ goto out;
+ }
+ ifd = -1;
+
+ ret = close(ofd);
if (ret == -1) {
status = map_nt_error_from_unix(errno);
goto out;
}
+ ofd = -1;
+
+ ret = SMB_VFS_NEXT_UNLINKAT(handle, srcfsp, source, 0);
+ if (ret == -1) {
+ status = map_nt_error_from_unix(errno);
+ }
- out:
+out:
+ if (ifd != -1) {
+ ret = close(ifd);
+ if (ret == -1) {
+ DBG_DEBUG("Failed to close %s (%d): %s.\n",
+ source->base_name,
+ ifd,
+ strerror(errno));
+ }
+ }
+ if (ofd != -1) {
+ ret = close(ofd);
+ if (ret == -1) {
+ DBG_DEBUG("Failed to close %s (%d): %s.\n",
+ dest->base_name,
+ ofd,
+ strerror(errno));
+ }
+ }
- TALLOC_FREE(full_fname_src);
- TALLOC_FREE(full_fname_dst);
return status;
}
--
GitLab
From f3f2a2d6683d8294c56f4ad275bb23d7a03c7aad Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Pavel=20Filipensk=C3=BD?= <pfilipensky@samba.org>
Date: Thu, 28 Nov 2024 18:32:25 +0100
Subject: [PATCH 2/6] s3:vfs_crossrename: crossrename_renameat() needs to
return 0 if copy_reg() is successful
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit
BUG: https://bugzilla.samba.org/show_bug.cgi?id=15724
Signed-off-by: Pavel Filipenský <pfilipensky@samba.org>
---
source3/modules/vfs_crossrename.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/source3/modules/vfs_crossrename.c b/source3/modules/vfs_crossrename.c
index c260bdee1948..5c6006931864 100644
--- a/source3/modules/vfs_crossrename.c
+++ b/source3/modules/vfs_crossrename.c
@@ -213,6 +213,7 @@ static int crossrename_renameat(vfs_handle_struct *handle,
smb_fname_src,
dstfsp,
smb_fname_dst);
+ result = 0;
if (!NT_STATUS_IS_OK(status)) {
errno = map_errno_from_nt_status(status);
result = -1;
--
GitLab
From cd26b847d044abd2ce0c3918a9c87bce1f4712c8 Mon Sep 17 00:00:00 2001
From: Jones Syue <jonessyue@qnap.com>
Date: Thu, 26 Sep 2024 17:17:14 +0800
Subject: [PATCH 3/6] s3:vfs_crossrename: add back checking for errno ENOENT
strace gives a clue: samba try to remove 'file.txt' in the dst folder but
actually it is not existed yet, and got an errno = ENOENT,
renameat(32, "file.txt", 31, "file.txt") = -1 EXDEV (Invalid cross-device link)
unlinkat(31, "file.txt", 0) = -1 ENOENT (No such file or directory)
Commit 5c18f074be92 ("s3: VFS: crossrename. Use real dirfsp for
SMB_VFS_RENAMEAT()") seems unintentionally removed errno ENOENT checking,
so add it back could address 1st issue.
BUG: https://bugzilla.samba.org/show_bug.cgi?id=15724
Signed-off-by: Jones Syue <jonessyue@qnap.com>
---
source3/modules/vfs_crossrename.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/source3/modules/vfs_crossrename.c b/source3/modules/vfs_crossrename.c
index 5c6006931864..0ce4b44898de 100644
--- a/source3/modules/vfs_crossrename.c
+++ b/source3/modules/vfs_crossrename.c
@@ -85,7 +85,7 @@ static NTSTATUS copy_reg(vfs_handle_struct *handle,
dstfsp,
dest,
0);
- if (ret == -1) {
+ if (ret == -1 && errno != ENOENT) {
status = map_nt_error_from_unix(errno);
goto out;
}
--
GitLab
From aa23df456abe4453e5baf839826434f363ca0711 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Pavel=20Filipensk=C3=BD?= <pfilipensky@samba.org>
Date: Mon, 2 Dec 2024 22:27:39 +0100
Subject: [PATCH 4/6] docs:manpage: vfs_crossrename is not fully stackable VFS
module
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit
BUG: https://bugzilla.samba.org/show_bug.cgi?id=15724
Signed-off-by: Pavel Filipenský <pfilipensky@samba.org>
---
docs-xml/manpages/vfs_crossrename.8.xml | 5 ++++-
1 file changed, 4 insertions(+), 1 deletion(-)
diff --git a/docs-xml/manpages/vfs_crossrename.8.xml b/docs-xml/manpages/vfs_crossrename.8.xml
index 72d67d685b1f..7ea0b50cc9b1 100644
--- a/docs-xml/manpages/vfs_crossrename.8.xml
+++ b/docs-xml/manpages/vfs_crossrename.8.xml
@@ -62,7 +62,10 @@
</varlistentry>
</variablelist>
- <para>This module is stackable.</para>
+ <para> This module is not fully stackable. It can be combined with other
+ modules, but should be the last module in the <command>vfs objects</command>
+ list. It directly access the files in the OS filesystem.
+ </para>
</refsect1>
--
GitLab
From 62741e4cdd368b162b961a8afef4b9dc3142739c Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Pavel=20Filipensk=C3=BD?= <pfilipensky@samba.org>
Date: Wed, 4 Dec 2024 11:02:18 +0100
Subject: [PATCH 5/6] selftest: Add test for vfs crossrename module
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit
BUG: https://bugzilla.samba.org/show_bug.cgi?id=15724
Signed-off-by: Pavel Filipenský <pfilipensky@samba.org>
---
selftest/target/Samba3.pm | 12 +++++
source3/script/tests/test_recycle.sh | 80 +++++++++++++++++++++++++++-
source3/selftest/tests.py | 2 +-
3 files changed, 92 insertions(+), 2 deletions(-)
diff --git a/selftest/target/Samba3.pm b/selftest/target/Samba3.pm
index 05027b26f469..d7abda753547 100755
--- a/selftest/target/Samba3.pm
+++ b/selftest/target/Samba3.pm
@@ -2783,6 +2783,9 @@ sub provision($$)
my $recycle_shrdir="$shrdir/recycle";
push(@dirs,$recycle_shrdir);
+ my $recycle_shrdir2="$shrdir/recycle2";
+ push(@dirs,$recycle_shrdir2);
+
my $fakedircreatetimes_shrdir="$shrdir/fakedircreatetimes";
push(@dirs,$fakedircreatetimes_shrdir);
@@ -3718,6 +3721,15 @@ sub provision($$)
recycle : exclude = *.tmp
recycle : directory_mode = 755
+[recycle2]
+ copy = tmp
+ path = $recycle_shrdir2
+ vfs objects = recycle crossrename
+ recycle : repository = .trash
+ recycle : exclude = *.tmp
+ recycle : directory_mode = 755
+ wide links = yes
+
[fakedircreatetimes]
copy = tmp
path = $fakedircreatetimes_shrdir
diff --git a/source3/script/tests/test_recycle.sh b/source3/script/tests/test_recycle.sh
index ba1d0a598b14..779683f4822f 100755
--- a/source3/script/tests/test_recycle.sh
+++ b/source3/script/tests/test_recycle.sh
@@ -29,7 +29,8 @@ export SAMBA_DEPRECATED_SUPPRESS
# Define the test environment/filenames.
#
-share_test_dir="$LOCAL_PATH"
+share_test_dir="$LOCAL_PATH/recycle"
+share_test_dir2="$LOCAL_PATH/recycle2"
#
# Cleanup function.
@@ -43,6 +44,13 @@ do_cleanup()
rm -f testfile2.tmp
rm -rf .trash
)
+ (
+ #subshell.
+ cd "$share_test_dir2" || return
+ rm -f testfile3
+ rm -f testfile4.tmp
+ rm -rf .trash
+ )
}
#
@@ -50,6 +58,25 @@ do_cleanup()
#
do_cleanup
+# Setup .trash on a different filesystem to test crossrename
+# /tmp or /dev/shm should provide tmpfs
+#
+for T in /tmp /dev/shm
+do
+ if df --portability --print-type $T 2>/dev/null | grep -q tmpfs; then
+ TRASHDIR=$T
+ break
+ fi
+done
+
+if [ -z $TRASHDIR ]; then
+ echo "No tmpfs filesystem found."
+ exit 1
+fi
+
+TRASHDIR=$(mktemp -d /$TRASHDIR/.trash_XXXXXX)
+chmod 0755 $TRASHDIR
+ln -s $TRASHDIR $share_test_dir2/.trash
test_recycle()
{
@@ -90,12 +117,61 @@ quit
return 0
}
+test_recycle_crossrename()
+{
+ tmpfile=$PREFIX/smbclient_interactive_prompt_commands
+ echo "
+put $tmpfile testfile3
+put $tmpfile testfile4.tmp
+del testfile3
+del testfile4.tmp
+quit
+" > $tmpfile
+ cmd='CLI_FORCE_INTERACTIVE=yes $SMBCLIENT -U$USERNAME%$PASSWORD //$SERVER/recycle2 -I$SERVER_IP $ADDARGS < $tmpfile 2>&1'
+ eval echo "$cmd"
+ out=$(eval "$cmd")
+ ret=$?
+ rm -f "$tmpfile"
+
+ if [ $ret != 0 ]; then
+ printf "%s\n" "$out"
+ printf "failed recycle smbclient run with error %s\n" "$ret"
+ return 1
+ fi
+
+ test -e "$share_test_dir2/.trash/testfile3" || {
+ printf ".trash/testfile3 expected to exist but does NOT exist\n"
+ return 1
+ }
+ test -e "$share_test_dir2/.trash/testfile4.tmp" && {
+ printf ".trash/testfile4.tmp not expected to exist but DOES exist\n"
+ return 1
+ }
+ deviceid1=`stat -c '%d' "$share_test_dir2/"`
+ deviceid2=`stat -c '%d' "$share_test_dir2/.trash/"`
+ test "$deviceid1=" != "$deviceid2" || {
+ printf ".trash/ should be on a different filesystem!\n"
+ return 1
+ }
+ perm_want=755
+ perm_is=`stat -c '%a' "$share_test_dir2/.trash/"`
+ test "$perm_is" = "$perm_want" || {
+ printf ".trash/ permission should be $perm_want but is $perm_is\n"
+ return 1
+ }
+ return 0
+}
+
panic_count_0=$(grep -c PANIC $SMBD_TEST_LOG)
testit "recycle" \
test_recycle ||
failed=$((failed + 1))
+testit "recycle_crossrename" \
+ test_recycle_crossrename ||
+ failed=$((failed + 1))
+
panic_count_1=$(grep -c PANIC $SMBD_TEST_LOG)
testit "check_panic" test $panic_count_0 -eq $panic_count_1 || failed=$(expr $failed + 1)
@@ -103,5 +179,7 @@ testit "check_panic" test $panic_count_0 -eq $panic_count_1 || failed=$(expr $fa
#
# Cleanup.
do_cleanup
+# Cleanup above only deletes a symlink, delete also /tmp/.trash_XXXXXX dir
+rm -rf "$TRASHDIR"
testok "$0" "$failed"
diff --git a/source3/selftest/tests.py b/source3/selftest/tests.py
index 6b690e2fc1f2..4540aef24b52 100755
--- a/source3/selftest/tests.py
+++ b/source3/selftest/tests.py
@@ -784,7 +784,7 @@ for env in ["fileserver"]:
plantestsuite("samba3.blackbox.force_create_mode", env, [os.path.join(samba3srcdir, "script/tests/test_force_create_mode.sh"), '$SERVER', '$DOMAIN', '$USERNAME', '$PASSWORD', '$PREFIX', env, smbclient3])
plantestsuite("samba3.blackbox.dropbox", env, [os.path.join(samba3srcdir, "script/tests/test_dropbox.sh"), '$SERVER', '$DOMAIN', 'gooduser', '$PASSWORD', '$PREFIX', env, smbclient3])
plantestsuite("samba3.blackbox.offline", env, [os.path.join(samba3srcdir, "script/tests/test_offline.sh"), '$SERVER', '$SERVER_IP', '$DOMAIN', '$USERNAME', '$PASSWORD', '$LOCAL_PATH/offline', smbclient3])
- plantestsuite("samba3.blackbox.recycle", env, [os.path.join(samba3srcdir, "script/tests/test_recycle.sh"), '$SERVER', '$SERVER_IP', '$USERNAME', '$PASSWORD', '$LOCAL_PATH/recycle', '$PREFIX', smbclient3])
+ plantestsuite("samba3.blackbox.recycle", env, [os.path.join(samba3srcdir, "script/tests/test_recycle.sh"), '$SERVER', '$SERVER_IP', '$USERNAME', '$PASSWORD', '$LOCAL_PATH', '$PREFIX', smbclient3])
plantestsuite("samba3.blackbox.fakedircreatetimes", env, [os.path.join(samba3srcdir, "script/tests/test_fakedircreatetimes.sh"), '$SERVER', '$SERVER_IP', '$USERNAME', '$PASSWORD', '$LOCAL_PATH/fakedircreatetimes', '$PREFIX', smbclient3])
plantestsuite("samba3.blackbox.shadow_copy2.NT1", env + "_smb1_done", [os.path.join(samba3srcdir, "script/tests/test_shadow_copy.sh"), '$SERVER', '$SERVER_IP', '$DOMAIN', '$USERNAME', '$PASSWORD', '$LOCAL_PATH/shadow', smbclient3, '-m', 'NT1'])
plantestsuite("samba3.blackbox.shadow_copy2.SMB3", env, [os.path.join(samba3srcdir, "script/tests/test_shadow_copy.sh"), '$SERVER', '$SERVER_IP', '$DOMAIN', '$USERNAME', '$PASSWORD', '$LOCAL_PATH/shadow', smbclient3, '-m', 'SMB3'])
--
GitLab
From c2cb4004f7a996863970c47471b55b9e6ee9caed Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Pavel=20Filipensk=C3=BD?= <pfilipensky@samba.org>
Date: Wed, 11 Dec 2024 22:33:17 +0100
Subject: [PATCH 6/6] s3:open.c: Fix a typo
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit
Signed-off-by: Pavel Filipenský <pfilipensky@samba.org>
---
source3/smbd/open.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/source3/smbd/open.c b/source3/smbd/open.c
index b87b74988ae4..8aac03677a21 100644
--- a/source3/smbd/open.c
+++ b/source3/smbd/open.c
@@ -4749,7 +4749,7 @@ mkdir_first:
&rhow);
if (ret == -1 && errno == EINVAL) {
/*
- * This is the strategie we use without having
+ * This is the strategy we use without having
* renameat2(RENAME_NOREPLACE):
*
* renameat() is able to replace a directory if the source is
--
GitLab