[PATCH] Fix libsmb renaming over existing files

[Jeremy Allison]

On Mon, Dec 12, 2016 at 09:11:09PM +0100, Stefan Metzmacher wrote:
> Am 12.12.2016 um 19:56 schrieb Jeremy Allison:
> > On Mon, Dec 12, 2016 at 05:04:22PM +0100, Volker Lendecke wrote:
> >> Hi!
> >>
> >> Review appreciated!
> > 
> > LGTM. Pushed with bug report id attached:
> > 
> > https://bugzilla.samba.org/show_bug.cgi?id=12468
> > 
> > as I think we need a back-port on this one.
> > 
> > A question. There are other places in that file
> > where we also use smb2cli_query_info() but
> > don't set raw_status after an error.
> > 
> > cli_smb2_qpathinfo_alt_name()
> > cli_smb2_qfileinfo_basic()
> > cli_smb2_qpathinfo_streams()
> > cli_smb2_dskattr()
> > cli_smb2_query_security_descriptor()
> > cli_smb2_get_ea_list_path()
> > cli_smb2_get_user_quota()
> > cli_smb2_list_user_quota_step()
> > cli_smb2_get_fs_quota_info()
> > 
> > Shall I prepare an additional patch that sets
> > raw_status appropriately there also ?
> 
> The goal would be to remove cli->raw_status in future.

Yep. It's horrible and the semantics are poorly
defined.

> Until then I think it's better to catch this generically
> in the caller.
> 
> E.g. in cli_rename() and a lot of others we have the following
> pattern at the beginning:
> 
>         if (smbXcli_conn_protocol(cli->conn) >= PROTOCOL_SMB2_02) {
>                 return cli_smb2_rename(cli,
>                                         fname_src,
>                                         fname_dst);
>         }
> 
> I think we should change this to:
> 
>         if (smbXcli_conn_protocol(cli->conn) >= PROTOCOL_SMB2_02) {
>                 cli->raw_status = cli_smb2_rename(cli,
>                                         fname_src,
>                                         fname_dst);
> 		return cli->raw_status;
>         }
> 
> 
> Otherwise we'll miss a lot of places within the cli_smb2_* functions
> where we miss to set cli->raw_status.

Unfortunately there are places inside source3/libsmb/cli_smb2_fnum.c
where cli->raw_status gets set by an intermediate function. The
classic case is where a pathname function calls ntcreate()/operation/close
as SMB2 only has handle-based ops. The close call sets cli->raw_status
so with just your suggestion we lose the raw_status on the 'operation'
that we actually wanted the error code from.

That's why Volker's patch sets cli->raw_status *after* the close
is done, to overwrite whatever the close set it to.

Right now inside source3/libsmb/cli_smb2_fnum.c cli->raw_status
always gets set in:

cli_smb2_create_fnum_recv()
cli_smb2_close_fnum_recv()
cli_smb2_read_recv()
cli_smb2_write_recv()
cli_smb2_writeall_recv()
cli_smb2_splice_recv()

And gets set only on error in:

cli_smb2_get_fs_attr_info()

Unfortunately we can't just set it on error due to
upper level functions such as:

cli_is_error()

which expect cli->raw_status to be NT_STATUS_OK
if the last operation succeeded.

So I think the lower-level patch is needed for
now.