[PATCH] Version 2 - Additional fix and test for bug #12899 - smbclient "setmode" no longer works to clear attribute bits due to dialect upgrade

[Jeremy Allison]

On Fri, Jul 14, 2017 at 05:39:41PM -0700, Jeremy Allison via samba-technical wrote:
> 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.

Thinking about this on the drive home, it means
that we just change the SMB2 code to match SMB1.

This means:

cli_setatr(cli, fname, FILE_ATTRIBUTE_NORMAL, 0);

specifies "no change" for both SMB1 and SMB2, and

cli_setatr(cli, fname, FILE_ATTRIBUTE_NORMAL, 0);

means "clear all attributes" for both SMB1 and SMB2.

That's the way the current SMB1-specific cli_setatr()
code works, so this fix swaps the semantics for
SMB2 to keep the existing libsmbclient ABI for callers.

Essentially inside cli_smb2_setatr() I do:

if (attr == 0) {
	attr = FILE_ATTRIBUTE_NORMAL;
} else if (attr == FILE_ATTRIBUTE_NORMAL;
	attr = 0;
}

to swap the meaning to the SMB1 semantics (with
appropriate comments explaining this of course).

That keeps the existing ABI and gives no change
to any cli_setatr() or libsmbclient callers. After
reverting Steve's patch and putting this in place
the new test patch will excercise this perfectly
as a regression test.

I'll also look into adding a source4/torture test
for the insane SMB1/SMB2 semantics to make sure
we test the server correctly without going through
the cli_setatr() interface.

Sounds like a plan - should get this done on Monday !

Jeremy.