[PATCHES] vfs_shadow_copy2: more tests and fix of a bug with SMB1
Uri Simchoni
uri at samba.org
Wed Aug 24 18:08:48 UTC 2016
Hi,
The attached patch set (some of which I already sent yesterday) adds
some more tests to shadow_copy2 module and then fixes a bug that was
discovered by one of those tests, namely that snapshot folders cannot be
listed using SMB1.
The fix may be controversial. The cause of the bug is that when listing
files with SMB1, call_trans2findfirst() is given a path of
path\to\dir\*, as this is what goes over the wire, and the
call_trans2findfirst() passes this path (after minimum SMB->unix
conversion) to some VFS functions (lstat, connectpath, realpath - via
check_reduced_name()). With the trailing "*". The connectpath VFS
function of shadow_copy2 overrides default behavior and fails with this
trailing "*".
The obvious fix seems to be to strip the "*" before going to the VFS,
because the VFS layer doesn't expect those things.
The fix I'm submitting does something else - it fixes things inside the
shadow_copy2 module. It lets connectpath not fail if the file in
question does not exist, and its parent folder exists. In such a case
connectpath is calculated relative to the parent folder. I think that in
general this is the right behavior, same as we're doing with realpath.
It also fixes issues with writeable snapshots (maybe none tried them).
The fix also fixes the listing issue because stat'ing "*" fails but
stat'ing its parent folder succeeds.
The only place where this patch would not give the correct result is if
there's a folder called "*", and it has snapshots underneath it (in
"snapshots everywhere" mode). In that case, if we try to list a snapshot
of "*"'s parent folder, we end up with the snapshots of "*" and the
snapshots of the parent become inaccessible because they're "outside"
and considered "wide links".
So we *can* also fix the trailing "*" issue at SMB level, I just don't
know enough about all the SMB requests that may receive a wildcard path
so it's a pain to scan for them, all to fix legacy code that otherwise
works well.
Review appreciated and sorry for this long email,
Uri.
-------------- next part --------------
From bf0ad8f43e0272196a9dbdd9b68a7c39f5dc52c6 Mon Sep 17 00:00:00 2001
From: Uri Simchoni <uri at samba.org>
Date: Tue, 16 Aug 2016 07:19:04 +0300
Subject: [PATCH 1/5] s2-selftest: run shadow_copy2 test both in NT1 and SMB3
modes
BUG: https://bugzilla.samba.org/show_bug.cgi?id=12172
Signed-off-by: Uri Simchoni <uri at samba.org>
---
source3/script/tests/test_shadow_copy.sh | 4 ++--
source3/selftest/tests.py | 3 ++-
2 files changed, 4 insertions(+), 3 deletions(-)
diff --git a/source3/script/tests/test_shadow_copy.sh b/source3/script/tests/test_shadow_copy.sh
index 45d9cf1..2fb1ef5 100755
--- a/source3/script/tests/test_shadow_copy.sh
+++ b/source3/script/tests/test_shadow_copy.sh
@@ -5,7 +5,7 @@
if [ $# -lt 7 ]; then
cat <<EOF
-Usage: test_shadow_copy SERVER SERVER_IP DOMAIN USERNAME PASSWORD WORKDIR SMBCLIENT
+Usage: test_shadow_copy SERVER SERVER_IP DOMAIN USERNAME PASSWORD WORKDIR SMBCLIENT PARAMS
EOF
exit 1;
fi
@@ -18,8 +18,8 @@ PASSWORD=${5}
WORKDIR=${6}
SMBCLIENT=${7}
shift 7
-SMBCLIENT="$VALGRIND ${SMBCLIENT}"
ADDARGS="$*"
+SMBCLIENT="$VALGRIND ${SMBCLIENT} ${ADDARGS}"
incdir=`dirname $0`/../../../testprogs/blackbox
. $incdir/subunit.sh
diff --git a/source3/selftest/tests.py b/source3/selftest/tests.py
index 0a0cb08..291aaa9 100755
--- a/source3/selftest/tests.py
+++ b/source3/selftest/tests.py
@@ -187,7 +187,8 @@ for env in ["fileserver"]:
plantestsuite("samba3.blackbox.dfree_quota (%s)" % env, env, [os.path.join(samba3srcdir, "script/tests/test_dfree_quota.sh"), '$SERVER', '$DOMAIN', '$USERNAME', '$PASSWORD', '$LOCAL_PATH', smbclient3, smbcquotas, smbcacls])
plantestsuite("samba3.blackbox.valid_users (%s)" % env, env, [os.path.join(samba3srcdir, "script/tests/test_valid_users.sh"), '$SERVER', '$SERVER_IP', '$DOMAIN', '$USERNAME', '$PASSWORD', '$PREFIX', smbclient3])
plantestsuite("samba3.blackbox.offline (%s)" % env, env, [os.path.join(samba3srcdir, "script/tests/test_offline.sh"), '$SERVER', '$SERVER_IP', '$DOMAIN', '$USERNAME', '$PASSWORD', '$LOCAL_PATH/offline', smbclient3])
- plantestsuite("samba3.blackbox.shadow_copy2 (%s)" % env, env, [os.path.join(samba3srcdir, "script/tests/test_shadow_copy.sh"), '$SERVER', '$SERVER_IP', '$DOMAIN', '$USERNAME', '$PASSWORD', '$LOCAL_PATH/shadow', smbclient3])
+ plantestsuite("samba3.blackbox.shadow_copy2 NT1 (%s)" % env, env, [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 (%s)" % env, env, [os.path.join(samba3srcdir, "script/tests/test_shadow_copy.sh"), '$SERVER', '$SERVER_IP', '$DOMAIN', '$USERNAME', '$PASSWORD', '$LOCAL_PATH/shadow', smbclient3, '-m', 'SMB3'])
plantestsuite("samba3.blackbox.smbclient.forceuser_validusers (%s)" % env, env, [os.path.join(samba3srcdir, "script/tests/test_forceuser_validusers.sh"), '$SERVER', '$DOMAIN', '$USERNAME', '$PASSWORD', '$LOCAL_PATH', smbclient3])
plantestsuite("samba3.blackbox.smbget (%s)" % env, env, [os.path.join(samba3srcdir, "script/tests/test_smbget.sh"), '$SERVER', '$SERVER_IP', '$DOMAIN', 'smbget_user', '$PASSWORD', '$LOCAL_PATH/smbget', smbget])
plantestsuite("samba3.blackbox.netshareenum (%s)" % env, env, [os.path.join(samba3srcdir, "script/tests/test_shareenum.sh"), '$SERVER', '$USERNAME', '$PASSWORD', rpcclient])
--
2.5.5
From 6b64501a423044ab5b2785fc5fe3a8b067001140 Mon Sep 17 00:00:00 2001
From: Uri Simchoni <uri at samba.org>
Date: Tue, 23 Aug 2016 11:33:52 +0300
Subject: [PATCH 2/5] selftest: add content to files created during
shadow_copy2 test
This will allow reading them and verifying we got the right version
BUG: https://bugzilla.samba.org/show_bug.cgi?id=12172
Signed-off-by: Uri Simchoni <uri at samba.org>
---
source3/script/tests/test_shadow_copy.sh | 26 ++++++++++++++------------
1 file changed, 14 insertions(+), 12 deletions(-)
diff --git a/source3/script/tests/test_shadow_copy.sh b/source3/script/tests/test_shadow_copy.sh
index 2fb1ef5..a59aae3 100755
--- a/source3/script/tests/test_shadow_copy.sh
+++ b/source3/script/tests/test_shadow_copy.sh
@@ -52,9 +52,11 @@ build_files()
local prefix
local version
local destdir
+ local content
rootdir=$1
prefix=$2
version=$3
+ content=$4
if [ -n "$prefix" ] ; then
destdir=$rootdir/$prefix
else
@@ -66,27 +68,27 @@ build_files()
#non-snapshot files
# for non-snapshot version, create legit files
# so that wide-link checks focus on snapshot files
- touch $destdir/foo
+ echo "$content" > $destdir/foo
mkdir -p $destdir/bar
- touch $destdir/bar/baz
- touch $destdir/bar/lfoo
- touch $destdir/bar/letcpasswd
- touch $destdir/bar/loutside
+ echo "$content" > $destdir/bar/baz
+ echo "$content" > $destdir/bar/lfoo
+ echo "$content" > $destdir/bar/letcpasswd
+ echo "$content" > $destdir/bar/loutside
elif [ "$version" = "fullsnap" ] ; then
#snapshot files
- touch $destdir/foo
+ echo "$content" > $destdir/foo
mkdir -p $destdir/bar
- touch $destdir/bar/baz
+ echo "$content" > $destdir/bar/baz
ln -fs ../foo $destdir/bar/lfoo
ln -fs /etc/passwd $destdir/bar/letcpasswd
ln -fs ../../outside $destdir/bar/loutside
- touch `dirname $destdir`/outside
+ echo "$content" > `dirname $destdir`/outside
else #subshare snapshot - at bar
- touch $destdir/baz
+ echo "$content" > $destdir/baz
ln -fs ../foo $destdir/lfoo
ln -fs /etc/passwd $destdir/letcpasswd
ln -fs ../../outside $destdir/loutside
- touch `dirname $destdir`/../outside
+ echo "$content" > `dirname $destdir`/../outside
fi
}
@@ -127,7 +129,7 @@ build_snapshots()
for i in `seq $start $end` ; do
snapname=${SNAPSHOTS[$i]}
mkdir $snapdir/$snapname
- build_files $snapdir/$snapname "$prefix" $version
+ build_files $snapdir/$snapname "$prefix" $version "$snapname"
done
}
@@ -295,7 +297,7 @@ test_shadow_copy_format()
}
#build "latest" files
-build_files $WORKDIR/mount base/share "latest"
+build_files $WORKDIR/mount base/share "latest" "latest"
failed=0
--
2.5.5
From 8e55b960519990d665310ea974e67581901260a4 Mon Sep 17 00:00:00 2001
From: Uri Simchoni <uri at samba.org>
Date: Tue, 23 Aug 2016 14:03:30 +0300
Subject: [PATCH 3/5] selftest: check file readability in shadow_copy2 test
Add tests which verify that a snapshot file is readable
if and only if it its metadata can be retrieved. Also
verify (in most tests) that file is retrieved from the
correct snapshot.
Together with the existing test for number of previous
versions we can stat, this test checks that we can read
those files, and also that we cannot break out of a snapshot
if wide links are not allowed.
BUG: https://bugzilla.samba.org/show_bug.cgi?id=12172
Signed-off-by: Uri Simchoni <uri at samba.org>
---
source3/script/tests/test_shadow_copy.sh | 44 +++++++++++++++++++++++++++-----
1 file changed, 37 insertions(+), 7 deletions(-)
diff --git a/source3/script/tests/test_shadow_copy.sh b/source3/script/tests/test_shadow_copy.sh
index a59aae3..2b72f06 100755
--- a/source3/script/tests/test_shadow_copy.sh
+++ b/source3/script/tests/test_shadow_copy.sh
@@ -139,18 +139,48 @@ test_count_versions()
local share
local path
local expected_count
+ local skip_content
local versions
+ local tstamps
+ local tstamp
+ local content
share=$1
path=$2
expected_count=$3
+ skip_content=$4
versions=`$SMBCLIENT -U$USERNAME%$PASSWORD "//$SERVER/$share" -I $SERVER_IP -c "allinfo $path" | grep "^create_time:" | wc -l`
- if [ "$versions" = "$expected_count" ] ; then
- true
- else
+ if [ "$versions" != "$expected_count" ] ; then
echo "expected $expected_count versions of $path, got $versions"
- false
+ return 1
fi
+
+ #readable snapshots
+ tstamps=`$SMBCLIENT -U$USERNAME%$PASSWORD "//$SERVER/$share" -I $SERVER_IP -c "allinfo $path" | awk '/^@GMT-/ {snapshot=$1} /^create_time:/ {printf "%s\n", snapshot}'`
+ for tstamp in $tstamps ; do
+ if ! $SMBCLIENT -U$USERNAME%$PASSWORD "//$SERVER/$share" -I $SERVER_IP -c "get $tstamp\\$path $WORKDIR/foo" ; then
+ echo "Failed getting \\\\$SERVER\\$share\\$tstamp\\$path"
+ return 1
+ fi
+ #also check the content, but not for wide links
+ if [ "x$skip_content" != "x1" ] ; then
+ content=`cat $WORKDIR/foo`
+ if [ "$content" != "$tstamp" ] ; then
+ echo "incorrect content of \\\\$SERVER\\$share\\$tstamp\\$path expected [$tstamp] got [$content]"
+ return 1
+ fi
+ fi
+ done
+
+ #non-readable snapshots
+ tstamps=`$SMBCLIENT -U$USERNAME%$PASSWORD "//$SERVER/$share" -I $SERVER_IP -c "allinfo $path" | \
+ awk '/^@GMT-/ {if (snapshot!=""){printf "%s\n", snapshot} ; snapshot=$1} /^create_time:/ {snapshot=""} END {if (snapshot!=""){printf "%s\n", snapshot}}'`
+ for tstamp in $tstamps ; do
+ if $SMBCLIENT -U$USERNAME%$PASSWORD "//$SERVER/$share" -I $SERVER_IP -c "get $tstamp\\$path $WORKDIR/foo" ; then
+ echo "Unexpected success getting \\\\$SERVER\\$share\\$tstamp\\$path"
+ return 1
+ fi
+ done
}
# Test fetching a previous version of a file
@@ -206,11 +236,11 @@ test_shadow_copy_fixed()
failed=`expr $failed + 1`
testit "$msg - abs symlink outside" \
- test_count_versions $share bar/letcpasswd $ncopies_blocked || \
+ test_count_versions $share bar/letcpasswd $ncopies_blocked 1 || \
failed=`expr $failed + 1`
testit "$msg - rel symlink outside" \
- test_count_versions $share bar/loutside $ncopies_blocked || \
+ test_count_versions $share bar/loutside $ncopies_blocked 1 || \
failed=`expr $failed + 1`
}
@@ -292,7 +322,7 @@ test_shadow_copy_format()
build_snapshots $WORKDIR/$where "$prefix" 10 19
testit "$msg - regular file" \
- test_count_versions $share foo $ncopies_allowed || \
+ test_count_versions $share foo $ncopies_allowed 1 || \
failed=`expr $failed + 1`
}
--
2.5.5
From 958c15282cac43861ce38348c484b467f7607e23 Mon Sep 17 00:00:00 2001
From: Uri Simchoni <uri at samba.org>
Date: Tue, 23 Aug 2016 14:29:39 +0300
Subject: [PATCH 4/5] selftest: test listing directories inside snapshots
Verify that directories are also listable.
BUG: https://bugzilla.samba.org/show_bug.cgi?id=12172
Signed-off-by: Uri Simchoni <uri at samba.org>
---
selftest/knownfail | 2 ++
source3/script/tests/test_shadow_copy.sh | 40 +++++++++++++++++++++++++++-----
2 files changed, 36 insertions(+), 6 deletions(-)
diff --git a/selftest/knownfail b/selftest/knownfail
index ffcaf06..38ad4c0 100644
--- a/selftest/knownfail
+++ b/selftest/knownfail
@@ -290,3 +290,5 @@
^samba4.smb2.read.access
#ntvfs server blocks copychunk with execute access on read handle
^samba4.smb2.ioctl.copy_chunk_bad_access
+#new snapshot dir listing test fails on SMB1
+^samba3.blackbox.shadow_copy2 NT1(?!.*wide links).*- list directory
diff --git a/source3/script/tests/test_shadow_copy.sh b/source3/script/tests/test_shadow_copy.sh
index 2b72f06..783e7f32 100755
--- a/source3/script/tests/test_shadow_copy.sh
+++ b/source3/script/tests/test_shadow_copy.sh
@@ -144,6 +144,7 @@ test_count_versions()
local tstamps
local tstamp
local content
+ local is_dir
share=$1
path=$2
@@ -155,13 +156,28 @@ test_count_versions()
return 1
fi
+ is_dir=0
+ $SMBCLIENT -U$USERNAME%$PASSWORD "//$SERVER/$share" -I $SERVER_IP -c "allinfo $path" | grep "^attributes:.*D" && is_dir=1
+ if [ $is_dir = 1 ] ; then
+ skip_content=1
+ fi
+
#readable snapshots
tstamps=`$SMBCLIENT -U$USERNAME%$PASSWORD "//$SERVER/$share" -I $SERVER_IP -c "allinfo $path" | awk '/^@GMT-/ {snapshot=$1} /^create_time:/ {printf "%s\n", snapshot}'`
for tstamp in $tstamps ; do
- if ! $SMBCLIENT -U$USERNAME%$PASSWORD "//$SERVER/$share" -I $SERVER_IP -c "get $tstamp\\$path $WORKDIR/foo" ; then
- echo "Failed getting \\\\$SERVER\\$share\\$tstamp\\$path"
- return 1
+ if [ $is_dir = 0 ] ;
+ then
+ if ! $SMBCLIENT -U$USERNAME%$PASSWORD "//$SERVER/$share" -I $SERVER_IP -c "get $tstamp\\$path $WORKDIR/foo" ; then
+ echo "Failed getting \\\\$SERVER\\$share\\$tstamp\\$path"
+ return 1
+ fi
+ else
+ if ! $SMBCLIENT -U$USERNAME%$PASSWORD "//$SERVER/$share" -I $SERVER_IP -c "ls $tstamp\\$path\\*" ; then
+ echo "Failed listing \\\\$SERVER\\$share\\$tstamp\\$path"
+ return 1
+ fi
fi
+
#also check the content, but not for wide links
if [ "x$skip_content" != "x1" ] ; then
content=`cat $WORKDIR/foo`
@@ -176,9 +192,17 @@ test_count_versions()
tstamps=`$SMBCLIENT -U$USERNAME%$PASSWORD "//$SERVER/$share" -I $SERVER_IP -c "allinfo $path" | \
awk '/^@GMT-/ {if (snapshot!=""){printf "%s\n", snapshot} ; snapshot=$1} /^create_time:/ {snapshot=""} END {if (snapshot!=""){printf "%s\n", snapshot}}'`
for tstamp in $tstamps ; do
- if $SMBCLIENT -U$USERNAME%$PASSWORD "//$SERVER/$share" -I $SERVER_IP -c "get $tstamp\\$path $WORKDIR/foo" ; then
- echo "Unexpected success getting \\\\$SERVER\\$share\\$tstamp\\$path"
- return 1
+ if [ $is_dir = 0 ] ;
+ then
+ if $SMBCLIENT -U$USERNAME%$PASSWORD "//$SERVER/$share" -I $SERVER_IP -c "get $tstamp\\$path $WORKDIR/foo" ; then
+ echo "Unexpected success getting \\\\$SERVER\\$share\\$tstamp\\$path"
+ return 1
+ fi
+ else
+ if $SMBCLIENT -U$USERNAME%$PASSWORD "//$SERVER/$share" -I $SERVER_IP -c "ls $tstamp\\$path\\*" ; then
+ echo "Unexpected success listing \\\\$SERVER\\$share\\$tstamp\\$path"
+ return 1
+ fi
fi
done
}
@@ -242,6 +266,10 @@ test_shadow_copy_fixed()
testit "$msg - rel symlink outside" \
test_count_versions $share bar/loutside $ncopies_blocked 1 || \
failed=`expr $failed + 1`
+
+ testit "$msg - list directory" \
+ test_count_versions $share bar $ncopies_allowed || \
+ failed=`expr $failed + 1`
}
test_shadow_copy_everywhere()
--
2.5.5
From 28ef030d482a08a2a3acc0b68222f95dd8d48b00 Mon Sep 17 00:00:00 2001
From: Uri Simchoni <uri at samba.org>
Date: Wed, 24 Aug 2016 14:42:23 +0300
Subject: [PATCH 5/5] vfs_shadow_copy: handle non-existant files and wildcards
During path checking, the vfs connectpath_fn is called to
determine the share's root, relative to the file being
queried (for example, in snapshot file this may be other
than the share's "usual" root directory). connectpath_fn
must be able to answer this question even if the path does
not exist and its parent does exist. The convention in this
case is that this refers to a yet-uncreated file under the parent
and all queries are relative to the parent.
This also serves as a workaround for the case where connectpath_fn
has to handle wildcards, as with the case of SMB1 trans2 findfirst.
BUG: https://bugzilla.samba.org/show_bug.cgi?id=12172
Signed-off-by: Uri Simchoni <uri at samba.org>
---
selftest/knownfail | 2 --
source3/modules/vfs_shadow_copy2.c | 31 ++++++++++++++++++++++++++++++-
2 files changed, 30 insertions(+), 3 deletions(-)
diff --git a/selftest/knownfail b/selftest/knownfail
index 38ad4c0..ffcaf06 100644
--- a/selftest/knownfail
+++ b/selftest/knownfail
@@ -290,5 +290,3 @@
^samba4.smb2.read.access
#ntvfs server blocks copychunk with execute access on read handle
^samba4.smb2.ioctl.copy_chunk_bad_access
-#new snapshot dir listing test fails on SMB1
-^samba3.blackbox.shadow_copy2 NT1(?!.*wide links).*- list directory
diff --git a/source3/modules/vfs_shadow_copy2.c b/source3/modules/vfs_shadow_copy2.c
index 2a72740..1f876e7 100644
--- a/source3/modules/vfs_shadow_copy2.c
+++ b/source3/modules/vfs_shadow_copy2.c
@@ -2207,6 +2207,7 @@ static const char *shadow_copy2_connectpath(struct vfs_handle_struct *handle,
char *stripped = NULL;
char *tmp = NULL;
char *result = NULL;
+ char *parent_dir = NULL;
int saved_errno;
size_t rootpath_len = 0;
@@ -2223,7 +2224,34 @@ static const char *shadow_copy2_connectpath(struct vfs_handle_struct *handle,
tmp = shadow_copy2_do_convert(talloc_tos(), handle, stripped, timestamp,
&rootpath_len);
if (tmp == NULL) {
- goto done;
+ if (errno != ENOENT) {
+ goto done;
+ }
+
+ /*
+ * If the converted path does not exist, and converting
+ * the parent yields something that does exist, then
+ * this path refers to something that has not been
+ * created yet, relative to the parent path.
+ * The snapshot finding is relative to the parent.
+ * (usually snapshots are read/only but this is not
+ * necessarily true).
+ * This code also covers getting a wildcard in the
+ * last component, because this function is called
+ * prior to sanitizing the path, and in SMB1 we may
+ * get wildcards in path names.
+ */
+ if (!parent_dirname(talloc_tos(), stripped, &parent_dir,
+ NULL)) {
+ errno = ENOMEM;
+ goto done;
+ }
+
+ tmp = shadow_copy2_do_convert(talloc_tos(), handle, parent_dir,
+ timestamp, &rootpath_len);
+ if (tmp == NULL) {
+ goto done;
+ }
}
DBG_DEBUG("converted path is [%s] root path is [%.*s]\n", tmp,
@@ -2241,6 +2269,7 @@ done:
saved_errno = errno;
TALLOC_FREE(tmp);
TALLOC_FREE(stripped);
+ TALLOC_FREE(parent_dir);
errno = saved_errno;
return result;
}
--
2.5.5
More information about the samba-technical
mailing list