With the upgrade to SMB3.1.1 from cifs in smbclient, it exposes a bug in smbclient. When clearing dos attributes (e.g. "setmode file +s" followed by "setmode file -s" the second operation works when smbclient negotiates cifs but fails with SMB2 or later since in cifs setting a 0x0 in the attributes field of setpathinfo is allowed, but in SMB2 and later we are using SET_INFO (SET_FILE_BASIC_INFO) in which case setting a field to 0 is a no op. If we do a "setmode -s" (or setmode -h or setmode -a etc.) and it results in a dos attribute of zero - we now need to change the 0 in that field to 0x80 (ATTRIBUTE_NORMAL) otherwise it will end up that removing the last dos attribute flag is a no op and leaves the bit set. One of our tests caught this when running against 4.7rc1 but we can reproduce this on earlier Samba if you force "client min dialect" to a later dialect (smb 3.1.1 e.g. instead of its default of cifs)
See MS-FSCC section 2.6 "The following attributes are defined for files and directories. They can be used in any combination unless noted in the description of the attribute's meaning. There is no file attribute with the value 0x00000000 because a value of 0x00000000 in the FileAttributes field means that the file attributes for this file MUST NOT be changed when setting basic information for the file."
Created attachment 13376 [details] patch to fix smbclient setmode (clearing last attribute)
Created attachment 13377 [details] git-am fix for master. Updated patch (follows README.Coding coding standards). Includes bug report number - needed for any backport.
(In reply to Jeremy Allison from comment #3) Thanks Jeremy! Can we please get some regression tests for this? We need to run this tests with -mNT1 and -mSMB3, so maybe just add something to source3/script/tests/test_smbclient_s3.sh
Steve wrote the patch so I'm going to let him write the regression tests too :-).
Created attachment 13387 [details] git-am fix for 4.7.next. Cherry-picked from master. As metze says, this does need a new test.
Oh, just wrote the test and found a bug in our SMB1 implementation ! Like Windows, we never store FILE_ATTRIBUTE_NORMAL - only strip it out of the stored attributes inside file_set_dosmode(). But source3/smbd/reply.c doesn't call file_set_dosmode() if the passed in attributes == FILE_ATTRIBUTE_NORMAL. This should be a check against zero, not FILE_ATTRIBUTE_NORMAL ! Updated patchset to follow.
OK - figured it out. Against Windows ------------------------------------------------------------------- Over SMB2 - doing a SMB2_CREATE/SET_FILE_BASIC_INFORMATION/CLOSE if attr == 0 - all attributes are left alone. if attr == 0x80 (FILE_ATTRIBUTE_NORMAL) - all attributes are cleared (server returns FILE_ATTRIBUTE_NORMAL on query). Over SMB1 - using SMB1setatr (0x9): if attr == 0 - all attributes are cleared (server returns FILE_ATTRIBUTE_NORMAL on query). if attr == 0x80 (FILE_ATTRIBUTE_NORMAL) - all attributes are left alone. ------------------------------------------------------------------- Completely and utterly insane, but there you go and I have the wireshark traces to prove it :-). What that means is Steve's original patch is incorrect (sorry for the +1 review, all I can say is it looked correct at the time but further testing proved otherwise :-). Yay for more tests :-). Most of our existing code uses the API: cli_setatr(cli1, fname, 0, 0); to mean clear all attributes. So what I think I'm going to do is revert Steve's patch and then change cli_smb2_setatr() to change attr == 0 to map attr to FILE_ATTRIBUTE_NORMAL internally for SMB2. That's actually following the intent. It does mean there's no way to use '0' in SMB2 to mean 'leave this alone', but I know of no use cases of that in our existing code. The most important thing is that cli_setatr() exposed to libsmbclient via SMBC_setatr(), so doing the SMB2 mapping will keep existing external users working correctly with the move to SMB2 by default.
Remember that this is only the case (apparently per-your testing) the case for SMB1 path based (old) setpath info not the normal info level based NT style set info. Also seems simpler to just override the old OS/2 / DOS level of setpathinfo to convert "ATTR_NORMAL" to 0 - rather than the reverse - since we actually should send down the more intuitive (the think that 99.9% of the servers expect) and special case the old path based setpathinfo
We probably want to keep the part of the patch that avoids the double set of attributes - but am a little worried about anywhere else in the code where we might do SMB1 setfileinfo (not using the very old path based call) where presumably the same behavior as SMB2 set file info applies ...
(In reply to Steve French from comment #9) > Also seems simpler to just override the old OS/2 / DOS level of setpathinfo to > convert "ATTR_NORMAL" to 0 - rather than the reverse - since we actually should > send down the more intuitive (the think that 99.9% of the servers expect) and > special case the old path based setpathinfo cli_setatr() is exposed as an external ABI via SMBC_setatr(), which means that existing external code (think Gnome VFS) can pass in 0 or FILE_ATTRIBUTE_NORMAL and get the SMB1 effects. We need to make the SMB2 code underneath cli_setatr() do the same. If you want a new call, then you can add a cli_setatr2() call later and use that inside SMBC_setatr(), but that's a bug report for another day.
Created attachment 13398 [details] git-am fix for 4.7.0 Back-port from master.
Reassigning to Karolin for inclusion in 4.7.
(In reply to Ralph Böhme from comment #13) Pushed to autobuild-v4-7-test.
(In reply to Karolin Seeger from comment #14) Pushed to v4-7-test. Closing out bug report. Thanks!
Created attachment 13433 [details] git-am fixes for 4.6.next, 4.5.next. Back-ports also needed for 4.6.next, 4.5.next.
Pushed to autobuild-v4-{6,5-}test.
(In reply to Karolin Seeger from comment #17) Pushed to all branches. Closing out bug report. Thanks!