[PATCH] Fix libsmb renaming over existing files
Uri Simchoni
uri at samba.org
Tue Dec 13 06:13:06 UTC 2016
On 12/13/2016 01:58 AM, Jeremy Allison wrote:
> On Mon, Dec 12, 2016 at 12:53:16PM -0800, Jeremy Allison wrote:
>> On Mon, Dec 12, 2016 at 09:11:09PM +0100, Stefan Metzmacher wrote:
>>> 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.
>
> And here it is. Please review and push if happy (passes
> make test here).
>
> Jeremy.
>
One comment + one patch to apply on top (mea cupla...)
Otherwise RB+ me.
> From 61236af38d52a063d2b3b46d0b1c1f625d10a67d Mon Sep 17 00:00:00 2001
> From: Jeremy Allison <jra at samba.org>
> Date: Mon, 12 Dec 2016 15:52:11 -0800
> Subject: [PATCH] s3: libsmb: Ensure SMB2 operations correctly set
> cli->raw_status.
>
> Needs to be done even on success (cli_is_error() checks if
> cli->raw_status was NT_STATUS_OK).
>
> BUG: https://bugzilla.samba.org/show_bug.cgi?id=12468
>
> Signed-off-by: Jeremy Allison <jra at samba.org>
> ---
> source3/libsmb/cli_smb2_fnum.c | 57 +++++++++++++++++++++++++++++++++++++-----
> 1 file changed, 51 insertions(+), 6 deletions(-)
>
> diff --git a/source3/libsmb/cli_smb2_fnum.c b/source3/libsmb/cli_smb2_fnum.c
> index 5a96b05..346af89 100644
> --- a/source3/libsmb/cli_smb2_fnum.c
> +++ b/source3/libsmb/cli_smb2_fnum.c
> @@ -886,6 +886,9 @@ NTSTATUS cli_smb2_list(struct cli_state *cli,
> if (fnum != 0xffff) {
> cli_smb2_close_fnum(cli, fnum);
> }
> +
> + cli->raw_status = status;
> +
> TALLOC_FREE(subframe);
> TALLOC_FREE(frame);
> return status;
> @@ -957,7 +960,7 @@ NTSTATUS cli_smb2_qpathinfo_basic(struct cli_state *cli,
> return status;
> }
>
> - cli_smb2_close_fnum(cli, fnum);
> + status = cli_smb2_close_fnum(cli, fnum);
>
> ZERO_STRUCTP(sbuf);
>
> @@ -967,7 +970,7 @@ NTSTATUS cli_smb2_qpathinfo_basic(struct cli_state *cli,
> sbuf->st_ex_size = cr.end_of_file;
> *attributes = cr.file_attributes;
>
> - return NT_STATUS_OK;
> + return status;
> }
>
This doesn't seem to set raw_status, and also, do you really want to
fail an otherwise successful query call because the close failed?
> /***************************************************************
> @@ -1133,6 +1136,9 @@ NTSTATUS cli_smb2_qpathinfo_alt_name(struct cli_state *cli,
> if (fnum != 0xffff) {
> cli_smb2_close_fnum(cli, fnum);
> }
> +
> + cli->raw_status = status;
> +
> TALLOC_FREE(frame);
> return status;
> }
> @@ -1232,6 +1238,8 @@ NTSTATUS cli_smb2_qfileinfo_basic(struct cli_state *cli,
>
> fail:
>
> + cli->raw_status = status;
> +
> TALLOC_FREE(frame);
> return status;
> }
> @@ -1263,6 +1271,8 @@ NTSTATUS cli_smb2_getattrE(struct cli_state *cli,
> &change_time_ts,
> NULL);
>
> + cli->raw_status = status;
> +
> if (!NT_STATUS_IS_OK(status)) {
> return status;
> }
> @@ -1340,6 +1350,8 @@ NTSTATUS cli_smb2_getatr(struct cli_state *cli,
> cli_smb2_close_fnum(cli, fnum);
> }
>
> + cli->raw_status = status;
> +
> TALLOC_FREE(frame);
> return status;
> }
> @@ -1410,6 +1422,8 @@ NTSTATUS cli_smb2_qpathinfo2(struct cli_state *cli,
> cli_smb2_close_fnum(cli, fnum);
> }
>
> + cli->raw_status = status;
> +
> TALLOC_FREE(frame);
> return status;
> }
> @@ -1498,6 +1512,8 @@ NTSTATUS cli_smb2_qpathinfo_streams(struct cli_state *cli,
> cli_smb2_close_fnum(cli, fnum);
> }
>
> + cli->raw_status = status;
> +
> TALLOC_FREE(frame);
> return status;
> }
> @@ -1580,6 +1596,8 @@ NTSTATUS cli_smb2_setatr(struct cli_state *cli,
> cli_smb2_close_fnum(cli, fnum);
> }
>
> + cli->raw_status = status;
> +
> TALLOC_FREE(frame);
> return status;
> }
> @@ -1636,7 +1654,7 @@ NTSTATUS cli_smb2_setattrE(struct cli_state *cli,
> put_long_date((char *)inbuf.data + 16, write_time);
> }
>
> - return smb2cli_set_info(cli->conn,
> + cli->raw_status = smb2cli_set_info(cli->conn,
> cli->timeout,
> cli->smb2.session,
> cli->smb2.tcon,
> @@ -1646,6 +1664,8 @@ NTSTATUS cli_smb2_setattrE(struct cli_state *cli,
> 0, /* in_additional_info */
> ph->fid_persistent,
> ph->fid_volatile);
> +
> + return cli->raw_status;
> }
>
> /***************************************************************
> @@ -1752,6 +1772,8 @@ NTSTATUS cli_smb2_dskattr(struct cli_state *cli, const char *path,
> cli_smb2_close_fnum(cli, fnum);
> }
>
> + cli->raw_status = status;
> +
> TALLOC_FREE(frame);
> return status;
> }
> @@ -1829,9 +1851,7 @@ fail:
> cli_smb2_close_fnum(cli, fnum);
> }
>
> - if (!NT_STATUS_IS_OK(status)) {
> - cli->raw_status = status;
> - }
> + cli->raw_status = status;
>
> TALLOC_FREE(frame);
> return status;
> @@ -1913,6 +1933,8 @@ NTSTATUS cli_smb2_query_security_descriptor(struct cli_state *cli,
>
> fail:
>
> + cli->raw_status = status;
> +
> TALLOC_FREE(frame);
> return status;
> }
> @@ -1976,6 +1998,8 @@ NTSTATUS cli_smb2_set_security_descriptor(struct cli_state *cli,
>
> fail:
>
> + cli->raw_status = status;
> +
> TALLOC_FREE(frame);
> return status;
> }
> @@ -2090,6 +2114,8 @@ NTSTATUS cli_smb2_rename(struct cli_state *cli,
> cli_smb2_close_fnum(cli, fnum);
> }
>
> + cli->raw_status = status;
> +
> TALLOC_FREE(frame);
> return status;
> }
> @@ -2183,6 +2209,8 @@ NTSTATUS cli_smb2_set_ea_fnum(struct cli_state *cli,
>
> fail:
>
> + cli->raw_status = status;
> +
> TALLOC_FREE(frame);
> return status;
> }
> @@ -2238,6 +2266,8 @@ NTSTATUS cli_smb2_set_ea_path(struct cli_state *cli,
> cli_smb2_close_fnum(cli, fnum);
> }
>
> + cli->raw_status = status;
> +
> return status;
> }
>
> @@ -2348,6 +2378,8 @@ NTSTATUS cli_smb2_get_ea_list_path(struct cli_state *cli,
> cli_smb2_close_fnum(cli, fnum);
> }
>
> + cli->raw_status = status;
> +
> TALLOC_FREE(frame);
> return status;
> }
> @@ -2433,6 +2465,8 @@ NTSTATUS cli_smb2_get_user_quota(struct cli_state *cli,
> }
>
> fail:
> + cli->raw_status = status;
> +
> TALLOC_FREE(frame);
> return status;
> }
> @@ -2506,6 +2540,8 @@ NTSTATUS cli_smb2_list_user_quota_step(struct cli_state *cli,
> pqt_list);
>
> cleanup:
> + cli->raw_status = status;
> +
> TALLOC_FREE(frame);
> return status;
> }
> @@ -2559,6 +2595,8 @@ NTSTATUS cli_smb2_get_fs_quota_info(struct cli_state *cli,
> status = parse_fs_quota_buffer(outbuf.data, outbuf.length, pqt);
>
> cleanup:
> + cli->raw_status = status;
> +
> TALLOC_FREE(frame);
> return status;
> }
> @@ -2607,6 +2645,9 @@ NTSTATUS cli_smb2_set_user_quota(struct cli_state *cli,
> 0, /* in_additional_info */
> ph->fid_persistent, ph->fid_volatile);
> cleanup:
> +
> + cli->raw_status = status;
> +
> TALLOC_FREE(frame);
>
> return status;
> @@ -2652,6 +2693,8 @@ NTSTATUS cli_smb2_set_fs_quota_info(struct cli_state *cli,
> 0, /* in_additional_info */
> ph->fid_persistent, ph->fid_volatile);
> cleanup:
> + cli->raw_status = status;
> +
> TALLOC_FREE(frame);
> return status;
> }
> @@ -3526,6 +3569,8 @@ NTSTATUS cli_smb2_shadow_copy_data(TALLOC_CTX *mem_ctx,
> pnames,
> pnum_names);
> fail:
> + cli->raw_status = status;
> +
> TALLOC_FREE(frame);
> return status;
> }
> --
> 2.8.0.rc3.226.g39d4020
>
-------------- next part --------------
From 49393c4339455a0b514f2a5f67c552a47141c3f0 Mon Sep 17 00:00:00 2001
From: Uri Simchoni <uri at samba.org>
Date: Tue, 13 Dec 2016 08:10:56 +0200
Subject: [PATCH] cli-quotas: fix potential memory leak
Fix a memory leak in out-of-memory condition
Signed-off-by: Uri Simchoni <uri at samba.org>
---
source3/libsmb/cli_smb2_fnum.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/source3/libsmb/cli_smb2_fnum.c b/source3/libsmb/cli_smb2_fnum.c
index 346af89..266f2d3 100644
--- a/source3/libsmb/cli_smb2_fnum.c
+++ b/source3/libsmb/cli_smb2_fnum.c
@@ -2682,7 +2682,7 @@ NTSTATUS cli_smb2_set_fs_quota_info(struct cli_state *cli,
status = build_fs_quota_buffer(talloc_tos(), pqt, &inbuf, 0);
if (!NT_STATUS_IS_OK(status)) {
- return status;
+ goto cleanup;
}
status = smb2cli_set_info(
--
2.9.3
More information about the samba-technical
mailing list