[PATCH] Fix regression with non-wide symlinks to directory over SMB3.
Ralph Böhme
slow at samba.org
Sat Jun 24 15:37:19 UTC 2017
Hi Jeremy,
On Fri, Jun 23, 2017 at 12:10:58PM -0700, Jeremy Allison wrote:
> Great catch (and fix) from Daniel Kobras <d.kobras at science-computing.de>
> for bug: https://bugzilla.samba.org/show_bug.cgi?id=12860
>
> I'll let him explain:
>
> -----------------------------------------------------------
> The errno returned by open() is ambiguous when called with flags O_NOFOLLOW and
> O_DIRECTORY on a symlink. With ELOOP, we know for certain that we've tried to
> open a symlink. With ENOTDIR, we might have hit a symlink, and need to perform
> further checks to be sure. Adjust non_widelink_open() accordingly. This fixes
> a regression where symlinks to directories within the same share were no
> longer followed for some call paths on systems returning ENOTDIR in the above
> case.
> -----------------------------------------------------------
oh, good catch! Sorry for not spotting this in the review.
Patch generally looks good, two nitpicks, see attached patch:
- use a file in selftest/knownfail.d/
- update another comment in non_widelink_open
Besides that, reviewed-by-me.
Cheerio!
-slow
-------------- next part --------------
From aa520e22ca03073114e0739f5539b680aac82ba5 Mon Sep 17 00:00:00 2001
From: Jeremy Allison <jra at samba.org>
Date: Fri, 23 Jun 2017 11:12:22 -0700
Subject: [PATCH 1/4] s3: smbd: Add regression test for non-wide symlinks to
directories fail over SMB3.
Mark as knownfail.
BUG: https://bugzilla.samba.org/show_bug.cgi?id=12860
Signed-off-by: Jeremy Allison <jra at samba.org>
---
selftest/knownfail | 1 +
selftest/target/Samba3.pm | 8 +++++
source3/script/tests/test_smbclient_s3.sh | 55 +++++++++++++++++++++++++++++++
3 files changed, 64 insertions(+)
diff --git a/selftest/knownfail b/selftest/knownfail
index c6047c8..9f9e976 100644
--- a/selftest/knownfail
+++ b/selftest/knownfail
@@ -335,3 +335,4 @@
# We currently don't send referrals for LDAP modify of non-replicated attrs
^samba4.ldap.rodc.python\(rodc\).__main__.RodcTests.test_modify_nonreplicated.*
^samba4.ldap.rodc_rwdc.python.*.__main__.RodcRwdcTests.test_change_password_reveal_on_demand_kerberos
+^samba3.blackbox.smbclient_s3.*follow local symlinks.*
diff --git a/selftest/target/Samba3.pm b/selftest/target/Samba3.pm
index d93d98e..6854d7c 100755
--- a/selftest/target/Samba3.pm
+++ b/selftest/target/Samba3.pm
@@ -1355,6 +1355,9 @@ sub provision($$$$$$$$$)
my $nosymlinks_shrdir="$shrdir/nosymlinks";
push(@dirs,$nosymlinks_shrdir);
+ my $local_symlinks_shrdir="$shrdir/local_symlinks";
+ push(@dirs,$local_symlinks_shrdir);
+
# this gets autocreated by winbindd
my $wbsockdir="$prefix_abs/winbindd";
@@ -1976,6 +1979,11 @@ sub provision($$$$$$$$$)
path = $nosymlinks_shrdir
follow symlinks = no
+[local_symlinks]
+ copy = tmp
+ path = $local_symlinks_shrdir
+ follow symlinks = yes
+
[kernel_oplocks]
copy = tmp
kernel oplocks = yes
diff --git a/source3/script/tests/test_smbclient_s3.sh b/source3/script/tests/test_smbclient_s3.sh
index 050dd81..1c5a13d 100755
--- a/source3/script/tests/test_smbclient_s3.sh
+++ b/source3/script/tests/test_smbclient_s3.sh
@@ -1215,6 +1215,57 @@ EOF
fi
}
+# Test we can follow normal symlinks.
+# Bug: https://bugzilla.samba.org/show_bug.cgi?id=12860
+# Note - this needs to be tested over SMB3, not SMB1.
+
+test_local_symlinks()
+{
+# Setup test dirs.
+ LOCAL_RAWARGS="${CONFIGURATION} -mSMB3"
+ LOCAL_ADDARGS="${LOCAL_RAWARGS} $*"
+
+ test_dir="$LOCAL_PATH/local_symlinks/test"
+
+ slink_name="$test_dir/sym_name"
+ slink_target_dir="$test_dir/dir1"
+
+ rm -rf $test_dir
+
+ mkdir -p $test_dir
+ mkdir $slink_target_dir
+ ln -s $slink_target_dir $slink_name
+
+# Can we cd into the symlink name and ls ?
+ tmpfile=$PREFIX/smbclient_interactive_prompt_commands
+ cat > $tmpfile <<EOF
+cd test\\sym_name
+ls
+quit
+EOF
+ cmd='CLI_FORCE_INTERACTIVE=yes $SMBCLIENT "$@" -U$USERNAME%$PASSWORD //$SERVER/local_symlinks -I $SERVER_IP $LOCAL_ADDARGS < $tmpfile 2>&1'
+ eval echo "$cmd"
+ out=`eval $cmd`
+ ret=$?
+ rm -f $tmpfile
+
+ if [ $ret -ne 0 ] ; then
+ echo "$out"
+ echo "failed accessing local_symlinks with error $ret"
+ false
+ return
+ fi
+
+ echo "$out" | grep 'NT_STATUS_'
+ ret=$?
+ if [ $ret -eq 0 ] ; then
+ echo "$out"
+ echo "failed - got an NT_STATUS error"
+ false
+ return
+ fi
+}
+
test_server_os_message()
{
tmpfile=$PREFIX/smbclient_interactive_prompt_commands
@@ -1348,6 +1399,10 @@ testit "follow symlinks = no" \
test_nosymlinks || \
failed=`expr $failed + 1`
+testit "follow local symlinks" \
+ test_local_symlinks || \
+ failed=`expr $failed + 1`
+
testit "server os message" \
test_server_os_message || \
failed=`expr $failed + 1`
--
2.9.3
From c23ed76915efa33454cc285dc704be0cff90f0c6 Mon Sep 17 00:00:00 2001
From: Ralph Boehme <slow at samba.org>
Date: Sat, 24 Jun 2017 17:26:58 +0200
Subject: [PATCH 2/4] FIXUP: use file in selftest/knownfail.d/
---
selftest/knownfail | 1 -
selftest/knownfail.d/samba3.blackbox.smbclient_s3.follow_local_symlinks | 1 +
2 files changed, 1 insertion(+), 1 deletion(-)
create mode 100644 selftest/knownfail.d/samba3.blackbox.smbclient_s3.follow_local_symlinks
diff --git a/selftest/knownfail b/selftest/knownfail
index 9f9e976..c6047c8 100644
--- a/selftest/knownfail
+++ b/selftest/knownfail
@@ -335,4 +335,3 @@
# We currently don't send referrals for LDAP modify of non-replicated attrs
^samba4.ldap.rodc.python\(rodc\).__main__.RodcTests.test_modify_nonreplicated.*
^samba4.ldap.rodc_rwdc.python.*.__main__.RodcRwdcTests.test_change_password_reveal_on_demand_kerberos
-^samba3.blackbox.smbclient_s3.*follow local symlinks.*
diff --git a/selftest/knownfail.d/samba3.blackbox.smbclient_s3.follow_local_symlinks b/selftest/knownfail.d/samba3.blackbox.smbclient_s3.follow_local_symlinks
new file mode 100644
index 0000000..7be44c1
--- /dev/null
+++ b/selftest/knownfail.d/samba3.blackbox.smbclient_s3.follow_local_symlinks
@@ -0,0 +1 @@
+^samba3.blackbox.smbclient_s3.*follow local symlinks.*
--
2.9.3
From 70543230d76640c18189257cadb85e044c26f45c Mon Sep 17 00:00:00 2001
From: Daniel Kobras <d.kobras at science-computing.de>
Date: Fri, 23 Jun 2017 15:39:21 +0200
Subject: [PATCH 3/4] s3: smbd: fix regression with non-wide symlinks to
directories over SMB3.
The errno returned by open() is ambiguous when called with flags O_NOFOLLOW and
O_DIRECTORY on a symlink. With ELOOP, we know for certain that we've tried to
open a symlink. With ENOTDIR, we might have hit a symlink, and need to perform
further checks to be sure. Adjust non_widelink_open() accordingly. This fixes
a regression where symlinks to directories within the same share were no
longer followed for some call paths on systems returning ENOTDIR in the above
case.
Also remove the knownfail added in previous commit.
BUG: https://bugzilla.samba.org/show_bug.cgi?id=12860
Signed-off-by: Daniel Kobras <d.kobras at science-computing.de>
Reviewed-by: Jeremy Allison <jra at samba.org>
---
...ba3.blackbox.smbclient_s3.follow_local_symlinks | 1 -
source3/smbd/open.c | 22 +++++++++++++++++++++-
2 files changed, 21 insertions(+), 2 deletions(-)
delete mode 100644 selftest/knownfail.d/samba3.blackbox.smbclient_s3.follow_local_symlinks
diff --git a/selftest/knownfail.d/samba3.blackbox.smbclient_s3.follow_local_symlinks b/selftest/knownfail.d/samba3.blackbox.smbclient_s3.follow_local_symlinks
deleted file mode 100644
index 7be44c1..0000000
--- a/selftest/knownfail.d/samba3.blackbox.smbclient_s3.follow_local_symlinks
+++ /dev/null
@@ -1 +0,0 @@
-^samba3.blackbox.smbclient_s3.*follow local symlinks.*
diff --git a/source3/smbd/open.c b/source3/smbd/open.c
index e68e2ac..382b16a 100644
--- a/source3/smbd/open.c
+++ b/source3/smbd/open.c
@@ -581,7 +581,18 @@ static int non_widelink_open(struct connection_struct *conn,
if (fd == -1) {
saved_errno = link_errno_convert(errno);
- if (saved_errno == ELOOP) {
+ /*
+ * Trying to open a symlink to a directory with O_NOFOLLOW and
+ * O_DIRECTORY can return either of ELOOP and ENOTDIR. So
+ * ENOTDIR really means: might be a symlink, but we're not sure.
+ * In this case, we just assume there's a symlink. If we were
+ * wrong, process_symlink_open() will return EINVAL. We check
+ * this below, and fall back to returning the initial
+ * saved_errno.
+ *
+ * BUG: https://bugzilla.samba.org/show_bug.cgi?id=12860
+ */
+ if (saved_errno == ELOOP || saved_errno == ENOTDIR) {
if (fsp->posix_flags & FSP_POSIX_FLAGS_OPEN) {
/* Never follow symlinks on posix open. */
goto out;
@@ -602,6 +613,15 @@ static int non_widelink_open(struct connection_struct *conn,
mode,
link_depth);
if (fd == -1) {
+ if (saved_errno == ENOTDIR &&
+ errno == EINVAL) {
+ /*
+ * O_DIRECTORY on neither a directory,
+ * nor a symlink. Just return
+ * saved_errno from initial open()
+ */
+ goto out;
+ }
saved_errno =
link_errno_convert(errno);
}
--
2.9.3
From 24fd40e3c40ba65b3d563e8e938c475e6973ce58 Mon Sep 17 00:00:00 2001
From: Ralph Boehme <slow at samba.org>
Date: Sat, 24 Jun 2017 17:24:15 +0200
Subject: [PATCH 4/4] FIXUP: update another comment
---
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 382b16a..fa74f48 100644
--- a/source3/smbd/open.c
+++ b/source3/smbd/open.c
@@ -602,7 +602,7 @@ static int non_widelink_open(struct connection_struct *conn,
goto out;
}
/*
- * We have a symlink. Follow in userspace
+ * We may have a symlink. Follow in userspace
* to ensure it's under the share definition.
*/
fd = process_symlink_open(conn,
--
2.9.3
More information about the samba-technical
mailing list