[PATCH] Fix incorrect dosmode on stream associated with dir
Jeremy Allison
jra at samba.org
Wed Apr 11 20:42:23 UTC 2018
On Wed, Apr 11, 2018 at 10:32:26AM -0700, Jeremy Allison via samba-technical wrote:
>
> Cancelled my push and will post a complete patch
> + regression test to follow :-).
OK, here is the correct (IMHO) fix. The initial fix
Volker RB+'ed is correct, but not enough to fix the
bug completely.
I discovered this by running the:
mkdir foo
put "localfile" foo:bar
setmode foo -a
allinfo foo:bar
tests against the streams_depot as well as streams_xattr
modules. streams_depot doesn't have the bug in not
removing the S_ISDIR bit, but still displayed the 'D'
attribute in error.
Digging deeper and reading the MS-FSA document
gave me the fix. Samba 4.7.x and above use a
new VFS function get_dos_attributes_fn() to
return DOS attributes, and both streams_depot
and streams_xattr don't implment these, meaning
we fallback to the default call that gets the
user.DOSATTRIB from the base file.
MS-FSA states that file streams do *NOT* have
their own DOS attribute metadata, this only
exists on the file object.
However, the client error discovered by Andrew
shows that Windows filters out the 'FILE_ATTRIBUTE_DIRECTORY'
attribute when base file attributes are returned
on non-default name stream objects stored on a directory
(as far as I know this isn't mentioned in the MS-docs, I
should probably raise a dochelp issue on that).
So I could have added the [f]get_dos_attributes()
functions to our current streams modules which
then filters the returned attributes to return
the base file attribute minus the FILE_ATTRIBUTE_DIRECTORY
attribute, and anyone with their own out-of-tree modules
implementing streams would need to implement
the identical code. This is needless code
duplication IMHO.
As logic must be identical for all stream-implementing
modules, it seemed clearer to me to implement this in
the calling code, dos_mode(), which is the central function
for returning all dos attributes. This is close to
Andrew's original fix but in a more central place,
plus we have to ensure it's a non-default NTFS
stream name (and not a POSIX name of course).
Included are the regression tests that allowed
me to find this and will make sure we don't
break again in future :-).
Please review and push if happy !
Jeremy.
-------------- next part --------------
>From b893c3506a83bf8c96830d2e2d17e92af6705d04 Mon Sep 17 00:00:00 2001
From: Jeremy Allison <jra at samba.org>
Date: Wed, 11 Apr 2018 08:41:00 -0700
Subject: [PATCH 1/3] s3: vfs: vfs_streams_xattr: Don't blindly re-use the base
file mode bits.
When returning the stat struct for an xattr stream,
we originally base the st_ex_mode field on the value
from the base file containing the xattr. If the base
file is a directory, it will have S_IFDIR set in st_ex_mode,
but streams can never be directories, they must be reported
as regular files.
The original code OR'ed in S_IFREG, but neglected to
AND out S_IFDIR.
Note this is not a complete to fix bug 13380 as
it doesn't fix the generic case with all streams
modules. See later fix and regression test.
Found in real-world use case by Andrew Walker <awalker at ixsystems.com>.
BUG: https://bugzilla.samba.org/show_bug.cgi?id=13380
Signed-off-by: Jeremy Allison <jra at samba.org>
Reviewed-by: Volker Lendecke <vl at samba.org>
---
source3/modules/vfs_streams_xattr.c | 2 ++
1 file changed, 2 insertions(+)
diff --git a/source3/modules/vfs_streams_xattr.c b/source3/modules/vfs_streams_xattr.c
index 580ecd0e5ff..c653656e5f8 100644
--- a/source3/modules/vfs_streams_xattr.c
+++ b/source3/modules/vfs_streams_xattr.c
@@ -277,6 +277,7 @@ static int streams_xattr_fstat(vfs_handle_struct *handle, files_struct *fsp,
sbuf->st_ex_ino = stream_inode(sbuf, io->xattr_name);
sbuf->st_ex_mode &= ~S_IFMT;
+ sbuf->st_ex_mode &= ~S_IFDIR;
sbuf->st_ex_mode |= S_IFREG;
sbuf->st_ex_blocks = sbuf->st_ex_size / STAT_ST_BLOCKSIZE + 1;
@@ -331,6 +332,7 @@ static int streams_xattr_stat(vfs_handle_struct *handle,
smb_fname->st.st_ex_ino = stream_inode(&smb_fname->st, xattr_name);
smb_fname->st.st_ex_mode &= ~S_IFMT;
+ smb_fname->st.st_ex_mode &= ~S_IFDIR;
smb_fname->st.st_ex_mode |= S_IFREG;
smb_fname->st.st_ex_blocks =
smb_fname->st.st_ex_size / STAT_ST_BLOCKSIZE + 1;
--
2.14.1
>From 5fa2dd0b343f585de37ac27cab34ed59116d6634 Mon Sep 17 00:00:00 2001
From: Jeremy Allison <jra at samba.org>
Date: Wed, 11 Apr 2018 11:05:14 -0700
Subject: [PATCH 2/3] s3: smbd. Generic fix for incorrect reporting of stream
dos attributes on a directory
According to MS-FSA a stream name does not have
separate DOS attribute metadata, so we must return
the DOS attribute from the base filename. With one caveat,
a non-default stream name can never be a directory.
As this is common to all streams data stores, we handle
it here instead of inside all stream VFS modules.
Otherwise identical logic would have to be added to
all streams modules in their [f]get_dos_attribute_fn()
VFS calls.
Found in real-world use case by Andrew Walker <awalker at ixsystems.com>.
BUG: https://bugzilla.samba.org/show_bug.cgi?id=13380
Signed-off-by: Jeremy Allison <jra at samba.org>
---
source3/smbd/dosmode.c | 22 ++++++++++++++++++++++
1 file changed, 22 insertions(+)
diff --git a/source3/smbd/dosmode.c b/source3/smbd/dosmode.c
index 8a11c8fd62a..7ac876a47bf 100644
--- a/source3/smbd/dosmode.c
+++ b/source3/smbd/dosmode.c
@@ -681,6 +681,28 @@ uint32_t dos_mode(connection_struct *conn, struct smb_filename *smb_fname)
}
}
+ /*
+ * According to MS-FSA a stream name does not have
+ * separate DOS attribute metadata, so we must return
+ * the DOS attribute from the base filename. With one caveat,
+ * a non-default stream name can never be a directory.
+ *
+ * As this is common to all streams data stores, we handle
+ * it here instead of inside all stream VFS modules.
+ *
+ * BUG: https://bugzilla.samba.org/show_bug.cgi?id=13380
+ */
+
+ if (is_ntfs_stream_smb_fname(smb_fname)) {
+ /* is_ntfs_stream_smb_fname() returns false for a POSIX path. */
+ if (!is_ntfs_default_stream_smb_fname(smb_fname)) {
+ /*
+ * Non-default stream name, not a posix path.
+ */
+ result &= ~(FILE_ATTRIBUTE_DIRECTORY);
+ }
+ }
+
if (conn->fs_capabilities & FILE_FILE_COMPRESSION) {
bool compressed = false;
status = dos_mode_check_compressed(conn, smb_fname,
--
2.14.1
>From efabe6866d5c559fd3fe4280e167550244eb2b63 Mon Sep 17 00:00:00 2001
From: Jeremy Allison <jra at samba.org>
Date: Wed, 11 Apr 2018 10:33:22 -0700
Subject: [PATCH 3/3] s3: tests: Regression test to ensure we can never
return a DIRECTORY attribute on a stream.
Tests streams_xattr and also streams_depot.
Inspired from a real-world test case by Andrew Walker <awalker at ixsystems.com>.
BUG: https://bugzilla.samba.org/show_bug.cgi?id=13380
Signed-off-by: Jeremy Allison <jra at samba.org>
---
selftest/target/Samba3.pm | 4 ++
source3/script/tests/test_smbclient_s3.sh | 76 +++++++++++++++++++++++++++++++
2 files changed, 80 insertions(+)
diff --git a/selftest/target/Samba3.pm b/selftest/target/Samba3.pm
index 8914507c12e..b93cbc27507 100755
--- a/selftest/target/Samba3.pm
+++ b/selftest/target/Samba3.pm
@@ -2143,6 +2143,10 @@ sub provision($$$$$$$$$)
kernel oplocks = yes
vfs objects = streams_xattr xattr_tdb
+[streams_xattr]
+ copy = tmp
+ vfs objects = streams_xattr xattr_tdb
+
[compound_find]
copy = tmp
smbd:find async delay usec = 10000
diff --git a/source3/script/tests/test_smbclient_s3.sh b/source3/script/tests/test_smbclient_s3.sh
index cc0d69dfb6e..b38c628698a 100755
--- a/source3/script/tests/test_smbclient_s3.sh
+++ b/source3/script/tests/test_smbclient_s3.sh
@@ -1606,6 +1606,78 @@ EOF
return 0
}
+# Test xattr_stream correctly reports mode.
+# BUG: https://bugzilla.samba.org/show_bug.cgi?id=13380
+
+test_stream_directory_xattr()
+{
+ tmpfile=$PREFIX/smbclient_interactive_prompt_commands
+#
+# Test against streams_xattr
+#
+ cat > $tmpfile <<EOF
+deltree foo
+mkdir foo
+put ${PREFIX}/smbclient_interactive_prompt_commands foo:bar
+setmode foo -a
+allinfo foo:bar
+deltree foo
+quit
+EOF
+ cmd='CLI_FORCE_INTERACTIVE=yes $SMBCLIENT "$@" -U$USERNAME%$PASSWORD //$SERVER/streams_xattr -I $SERVER_IP $ADDARGS < $tmpfile 2>&1'
+ eval echo "$cmd"
+ out=`eval $cmd`
+ ret=$?
+ rm -f $tmpfile
+
+ if [ $ret != 0 ] ; then
+ echo "$out"
+ echo "failed checking attributes on xattr stream foo:bar with error $ret"
+ return 1
+ fi
+
+ echo "$out" | grep "attributes:.*80"
+ ret=$?
+ if [ $ret != 0 ] ; then
+ echo "$out"
+ echo "failedecking attributes on xattr stream foo:bar"
+ return 1
+ fi
+
+#
+# Test against streams_depot
+#
+ cat > $tmpfile <<EOF
+deltree foo
+mkdir foo
+put ${PREFIX}/smbclient_interactive_prompt_commands foo:bar
+setmode foo -a
+allinfo foo:bar
+deltree foo
+quit
+EOF
+ cmd='CLI_FORCE_INTERACTIVE=yes $SMBCLIENT "$@" -U$USERNAME%$PASSWORD //$SERVER/tmp -I $SERVER_IP $ADDARGS < $tmpfile 2>&1'
+ eval echo "$cmd"
+ out=`eval $cmd`
+ ret=$?
+ rm -f $tmpfile
+
+ if [ $ret != 0 ] ; then
+ echo "$out"
+ echo "failed checking attributes on depot stream foo:bar with error $ret"
+ return 1
+ fi
+
+ echo "$out" | grep "attributes:.*80"
+ ret=$?
+ if [ $ret != 0 ] ; then
+ echo "$out"
+ echo "failedecking attributes on depot stream foo:bar"
+ return 1
+ fi
+}
+
+#
LOGDIR_PREFIX=test_smbclient_s3
# possibly remove old logdirs:
@@ -1705,6 +1777,10 @@ testit "streams_depot can delete correctly" \
test_streams_depot_delete || \
failed=`expr $failed + 1`
+testit "stream_xattr attributes" \
+ test_stream_directory_xattr || \
+ failed=`expr $failed + 1`
+
testit "follow symlinks = no" \
test_nosymlinks || \
failed=`expr $failed + 1`
--
2.14.1
More information about the samba-technical
mailing list