[PATCHES] vfs_shadow_copy2: more tests and fix of a bug with SMB1
Jeremy Allison
jra at samba.org
Wed Aug 24 23:04:32 UTC 2016
On Wed, Aug 24, 2016 at 09:08:48PM +0300, Uri Simchoni wrote:
> 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.
I'm happy with the shadow_copy2 fix. The same problem doesn't
exist in the vfs_snapper module.
Reviewed-by: Jeremy Allison <jra at samba.org>
Pushed !
> 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