[PATCH] Fix a file handle leak in cli_posix_mkdir
Jeremy Allison
jra at samba.org
Fri Feb 22 18:43:22 UTC 2019
On Wed, Feb 20, 2019 at 03:33:55PM +0100, Volker Lendecke via samba-technical wrote:
> Hi!
>
> Review appreciated!
RB+ and pushed. Thanks a *LOT* for catching
this. I looked into a bug and back-port, but
the only user is smbclient itself, not libsmbclient
so we're not exposed from a library point of
view, thank goodness !
Great work, thanks !
Jeremy.
> --
> SerNet GmbH, Bahnhofsallee 1b, 37081 Göttingen
> phone: 0551-370000-0, mailto:kontakt at sernet.de
> Gesch.F.: Dr. Johannes Loxen und Reinhild Jung
> AG Göttingen: HR-B 2816 - http://www.sernet.de
> From c881ec76ea2273552533b6968f9f76bdf0680954 Mon Sep 17 00:00:00 2001
> From: Volker Lendecke <vl at samba.org>
> Date: Fri, 15 Feb 2019 20:14:47 +0100
> Subject: [PATCH 1/3] libsmb: Convert cli_posix_open to normal tevent_req
> pattern
>
> Signed-off-by: Volker Lendecke <vl at samba.org>
> ---
> source3/libsmb/clifile.c | 199 ++++++++++++++++++++++++++++++++++-------------
> 1 file changed, 143 insertions(+), 56 deletions(-)
>
> diff --git a/source3/libsmb/clifile.c b/source3/libsmb/clifile.c
> index 6defa38fdee..d505fe517b1 100644
> --- a/source3/libsmb/clifile.c
> +++ b/source3/libsmb/clifile.c
> @@ -5099,31 +5099,14 @@ static uint32_t open_flags_to_wire(int flags)
> Open a file - POSIX semantics. Returns fnum. Doesn't request oplock.
> ****************************************************************************/
>
> -struct posix_open_state {
> +struct cli_posix_open_internal_state {
> uint16_t setup;
> uint8_t *param;
> uint8_t data[18];
> uint16_t fnum; /* Out */
> };
>
> -static void cli_posix_open_internal_done(struct tevent_req *subreq)
> -{
> - struct tevent_req *req = tevent_req_callback_data(
> - subreq, struct tevent_req);
> - struct posix_open_state *state = tevent_req_data(req, struct posix_open_state);
> - NTSTATUS status;
> - uint8_t *data;
> - uint32_t num_data;
> -
> - status = cli_trans_recv(subreq, state, NULL, NULL, 0, NULL,
> - NULL, 0, NULL, &data, 12, &num_data);
> - TALLOC_FREE(subreq);
> - if (tevent_req_nterror(req, status)) {
> - return;
> - }
> - state->fnum = SVAL(data,2);
> - tevent_req_done(req);
> -}
> +static void cli_posix_open_internal_done(struct tevent_req *subreq);
>
> static struct tevent_req *cli_posix_open_internal_send(TALLOC_CTX *mem_ctx,
> struct tevent_context *ev,
> @@ -5134,10 +5117,11 @@ static struct tevent_req *cli_posix_open_internal_send(TALLOC_CTX *mem_ctx,
> bool is_dir)
> {
> struct tevent_req *req = NULL, *subreq = NULL;
> - struct posix_open_state *state = NULL;
> + struct cli_posix_open_internal_state *state = NULL;
> uint32_t wire_flags = open_flags_to_wire(flags);
>
> - req = tevent_req_create(mem_ctx, &state, struct posix_open_state);
> + req = tevent_req_create(
> + mem_ctx, &state, struct cli_posix_open_internal_state);
> if (req == NULL) {
> return NULL;
> }
> @@ -5146,15 +5130,18 @@ static struct tevent_req *cli_posix_open_internal_send(TALLOC_CTX *mem_ctx,
> SSVAL(&state->setup, 0, TRANSACT2_SETPATHINFO);
>
> /* Setup param array. */
> - state->param = talloc_array(state, uint8_t, 6);
> + state->param = talloc_zero_array(state, uint8_t, 6);
> if (tevent_req_nomem(state->param, req)) {
> return tevent_req_post(req, ev);
> }
> - memset(state->param, '\0', 6);
> SSVAL(state->param, 0, SMB_POSIX_PATH_OPEN);
>
> - state->param = trans2_bytes_push_str(state->param, smbXcli_conn_use_unicode(cli->conn), fname,
> - strlen(fname)+1, NULL);
> + state->param = trans2_bytes_push_str(
> + state->param,
> + smbXcli_conn_use_unicode(cli->conn),
> + fname,
> + strlen(fname)+1,
> + NULL);
>
> if (tevent_req_nomem(state->param, req)) {
> return tevent_req_post(req, ev);
> @@ -5197,6 +5184,57 @@ static struct tevent_req *cli_posix_open_internal_send(TALLOC_CTX *mem_ctx,
> return req;
> }
>
> +static void cli_posix_open_internal_done(struct tevent_req *subreq)
> +{
> + struct tevent_req *req = tevent_req_callback_data(
> + subreq, struct tevent_req);
> + struct cli_posix_open_internal_state *state = tevent_req_data(
> + req, struct cli_posix_open_internal_state);
> + NTSTATUS status;
> + uint8_t *data;
> + uint32_t num_data;
> +
> + status = cli_trans_recv(
> + subreq,
> + state,
> + NULL,
> + NULL,
> + 0,
> + NULL,
> + NULL,
> + 0,
> + NULL,
> + &data,
> + 12,
> + &num_data);
> + TALLOC_FREE(subreq);
> + if (tevent_req_nterror(req, status)) {
> + return;
> + }
> + state->fnum = SVAL(data,2);
> + tevent_req_done(req);
> +}
> +
> +static NTSTATUS cli_posix_open_internal_recv(struct tevent_req *req,
> + uint16_t *pfnum)
> +{
> + struct cli_posix_open_internal_state *state = tevent_req_data(
> + req, struct cli_posix_open_internal_state);
> + NTSTATUS status;
> +
> + if (tevent_req_is_nterror(req, &status)) {
> + return status;
> + }
> + *pfnum = state->fnum;
> + return NT_STATUS_OK;
> +}
> +
> +struct cli_posix_open_state {
> + uint16_t fnum;
> +};
> +
> +static void cli_posix_open_done(struct tevent_req *subreq);
> +
> struct tevent_req *cli_posix_open_send(TALLOC_CTX *mem_ctx,
> struct tevent_context *ev,
> struct cli_state *cli,
> @@ -5204,13 +5242,44 @@ struct tevent_req *cli_posix_open_send(TALLOC_CTX *mem_ctx,
> int flags,
> mode_t mode)
> {
> - return cli_posix_open_internal_send(mem_ctx, ev,
> - cli, fname, flags, mode, false);
> + struct tevent_req *req = NULL, *subreq = NULL;
> + struct cli_posix_open_state *state = NULL;
> +
> + req = tevent_req_create(mem_ctx, &state,
> + struct cli_posix_open_state);
> + if (req == NULL) {
> + return NULL;
> + }
> +
> + subreq = cli_posix_open_internal_send(
> + mem_ctx, ev, cli, fname, flags, mode, false);
> + if (tevent_req_nomem(subreq, req)) {
> + return tevent_req_post(req, ev);
> + }
> + tevent_req_set_callback(subreq, cli_posix_open_done, req);
> + return req;
> +}
> +
> +static void cli_posix_open_done(struct tevent_req *subreq)
> +{
> + struct tevent_req *req = tevent_req_callback_data(
> + subreq, struct tevent_req);
> + struct cli_posix_open_state *state = tevent_req_data(
> + req, struct cli_posix_open_state);
> + NTSTATUS status;
> +
> + status = cli_posix_open_internal_recv(subreq, &state->fnum);
> + TALLOC_FREE(subreq);
> + if (tevent_req_nterror(req, status)) {
> + return;
> + }
> + tevent_req_done(req);
> }
>
> NTSTATUS cli_posix_open_recv(struct tevent_req *req, uint16_t *pfnum)
> {
> - struct posix_open_state *state = tevent_req_data(req, struct posix_open_state);
> + struct cli_posix_open_state *state = tevent_req_data(
> + req, struct cli_posix_open_state);
> NTSTATUS status;
>
> if (tevent_req_is_nterror(req, &status)) {
> @@ -5231,7 +5300,7 @@ NTSTATUS cli_posix_open(struct cli_state *cli, const char *fname,
> TALLOC_CTX *frame = talloc_stackframe();
> struct tevent_context *ev = NULL;
> struct tevent_req *req = NULL;
> - NTSTATUS status = NT_STATUS_OK;
> + NTSTATUS status = NT_STATUS_NO_MEMORY;
>
> if (smbXcli_conn_has_async_calls(cli->conn)) {
> /*
> @@ -5240,43 +5309,70 @@ NTSTATUS cli_posix_open(struct cli_state *cli, const char *fname,
> status = NT_STATUS_INVALID_PARAMETER;
> goto fail;
> }
> -
> ev = samba_tevent_context_init(frame);
> if (ev == NULL) {
> - status = NT_STATUS_NO_MEMORY;
> goto fail;
> }
> -
> - req = cli_posix_open_send(frame,
> - ev,
> - cli,
> - fname,
> - flags,
> - mode);
> + req = cli_posix_open_send(
> + frame, ev, cli, fname, flags, mode);
> if (req == NULL) {
> - status = NT_STATUS_NO_MEMORY;
> goto fail;
> }
> -
> if (!tevent_req_poll_ntstatus(req, ev, &status)) {
> goto fail;
> }
> -
> status = cli_posix_open_recv(req, pfnum);
> -
> fail:
> TALLOC_FREE(frame);
> return status;
> }
>
> +struct cli_posix_mkdir_state {
> + struct tevent_context *ev;
> + struct cli_state *cli;
> +};
> +
> +static void cli_posix_mkdir_done(struct tevent_req *subreq);
> +
> struct tevent_req *cli_posix_mkdir_send(TALLOC_CTX *mem_ctx,
> struct tevent_context *ev,
> struct cli_state *cli,
> const char *fname,
> mode_t mode)
> {
> - return cli_posix_open_internal_send(mem_ctx, ev,
> - cli, fname, O_CREAT, mode, true);
> + struct tevent_req *req = NULL, *subreq = NULL;
> + struct cli_posix_mkdir_state *state = NULL;
> +
> + req = tevent_req_create(
> + mem_ctx, &state, struct cli_posix_mkdir_state);
> + if (req == NULL) {
> + return NULL;
> + }
> + state->ev = ev;
> + state->cli = cli;
> +
> + subreq = cli_posix_open_internal_send(
> + mem_ctx, ev, cli, fname, O_CREAT, mode, true);
> + if (tevent_req_nomem(subreq, req)) {
> + return tevent_req_post(req, ev);
> + }
> + tevent_req_set_callback(subreq, cli_posix_mkdir_done, req);
> + return req;
> +}
> +
> +static void cli_posix_mkdir_done(struct tevent_req *subreq)
> +{
> + struct tevent_req *req = tevent_req_callback_data(
> + subreq, struct tevent_req);
> + NTSTATUS status;
> + uint16_t fnum;
> +
> + status = cli_posix_open_internal_recv(subreq, &fnum);
> + TALLOC_FREE(subreq);
> + if (tevent_req_nterror(req, status)) {
> + return;
> + }
> + tevent_req_done(req);
> }
>
> NTSTATUS cli_posix_mkdir_recv(struct tevent_req *req)
> @@ -5289,7 +5385,7 @@ NTSTATUS cli_posix_mkdir(struct cli_state *cli, const char *fname, mode_t mode)
> TALLOC_CTX *frame = talloc_stackframe();
> struct tevent_context *ev = NULL;
> struct tevent_req *req = NULL;
> - NTSTATUS status = NT_STATUS_OK;
> + NTSTATUS status = NT_STATUS_NO_MEMORY;
>
> if (smbXcli_conn_has_async_calls(cli->conn)) {
> /*
> @@ -5301,26 +5397,17 @@ NTSTATUS cli_posix_mkdir(struct cli_state *cli, const char *fname, mode_t mode)
>
> ev = samba_tevent_context_init(frame);
> if (ev == NULL) {
> - status = NT_STATUS_NO_MEMORY;
> goto fail;
> }
> -
> - req = cli_posix_mkdir_send(frame,
> - ev,
> - cli,
> - fname,
> - mode);
> + req = cli_posix_mkdir_send(
> + frame, ev, cli, fname, mode);
> if (req == NULL) {
> - status = NT_STATUS_NO_MEMORY;
> goto fail;
> }
> -
> if (!tevent_req_poll_ntstatus(req, ev, &status)) {
> goto fail;
> }
> -
> status = cli_posix_mkdir_recv(req);
> -
> fail:
> TALLOC_FREE(frame);
> return status;
> --
> 2.11.0
>
>
> From 3f2fc75e1f63a6630a721bb504f7271ed747597d Mon Sep 17 00:00:00 2001
> From: Volker Lendecke <vl at samba.org>
> Date: Wed, 20 Feb 2019 11:41:42 +0100
> Subject: [PATCH 2/3] libsmb: Pull up wire_flags calculation from open_internal
>
> This avoids passing down a boolean
>
> Signed-off-by: Volker Lendecke <vl at samba.org>
> ---
> source3/libsmb/clifile.c | 21 ++++++++++-----------
> 1 file changed, 10 insertions(+), 11 deletions(-)
>
> diff --git a/source3/libsmb/clifile.c b/source3/libsmb/clifile.c
> index d505fe517b1..61bc73effa2 100644
> --- a/source3/libsmb/clifile.c
> +++ b/source3/libsmb/clifile.c
> @@ -5112,13 +5112,11 @@ static struct tevent_req *cli_posix_open_internal_send(TALLOC_CTX *mem_ctx,
> struct tevent_context *ev,
> struct cli_state *cli,
> const char *fname,
> - int flags,
> - mode_t mode,
> - bool is_dir)
> + uint32_t wire_flags,
> + mode_t mode)
> {
> struct tevent_req *req = NULL, *subreq = NULL;
> struct cli_posix_open_internal_state *state = NULL;
> - uint32_t wire_flags = open_flags_to_wire(flags);
>
> req = tevent_req_create(
> mem_ctx, &state, struct cli_posix_open_internal_state);
> @@ -5147,11 +5145,6 @@ static struct tevent_req *cli_posix_open_internal_send(TALLOC_CTX *mem_ctx,
> return tevent_req_post(req, ev);
> }
>
> - /* Setup data words. */
> - if (is_dir) {
> - wire_flags |= SMB_O_DIRECTORY;
> - }
> -
> SIVAL(state->data,0,0); /* No oplock. */
> SIVAL(state->data,4,wire_flags);
> SIVAL(state->data,8,unix_perms_to_wire(mode));
> @@ -5244,6 +5237,7 @@ struct tevent_req *cli_posix_open_send(TALLOC_CTX *mem_ctx,
> {
> struct tevent_req *req = NULL, *subreq = NULL;
> struct cli_posix_open_state *state = NULL;
> + uint32_t wire_flags;
>
> req = tevent_req_create(mem_ctx, &state,
> struct cli_posix_open_state);
> @@ -5251,8 +5245,10 @@ struct tevent_req *cli_posix_open_send(TALLOC_CTX *mem_ctx,
> return NULL;
> }
>
> + wire_flags = open_flags_to_wire(flags);
> +
> subreq = cli_posix_open_internal_send(
> - mem_ctx, ev, cli, fname, flags, mode, false);
> + mem_ctx, ev, cli, fname, wire_flags, mode);
> if (tevent_req_nomem(subreq, req)) {
> return tevent_req_post(req, ev);
> }
> @@ -5342,6 +5338,7 @@ struct tevent_req *cli_posix_mkdir_send(TALLOC_CTX *mem_ctx,
> {
> struct tevent_req *req = NULL, *subreq = NULL;
> struct cli_posix_mkdir_state *state = NULL;
> + uint32_t wire_flags;
>
> req = tevent_req_create(
> mem_ctx, &state, struct cli_posix_mkdir_state);
> @@ -5351,8 +5348,10 @@ struct tevent_req *cli_posix_mkdir_send(TALLOC_CTX *mem_ctx,
> state->ev = ev;
> state->cli = cli;
>
> + wire_flags = open_flags_to_wire(O_CREAT) | SMB_O_DIRECTORY;
> +
> subreq = cli_posix_open_internal_send(
> - mem_ctx, ev, cli, fname, O_CREAT, mode, true);
> + mem_ctx, ev, cli, fname, wire_flags, mode);
> if (tevent_req_nomem(subreq, req)) {
> return tevent_req_post(req, ev);
> }
> --
> 2.11.0
>
>
> From 177fc9005d944ad54cd68f8cb15bc4266e08c576 Mon Sep 17 00:00:00 2001
> From: Volker Lendecke <vl at samba.org>
> Date: Wed, 20 Feb 2019 11:55:01 +0100
> Subject: [PATCH 3/3] libsmb: Fix a resource leak in cli_posix_mkdir
>
> smbd does posix_mkdir if the wire flags are exactly
>
> if (wire_open_mode == (SMB_O_CREAT|SMB_O_DIRECTORY))
>
> open_flags_to_wire however adds a SMB_O_RDONLY, so that we enter the
> normal open routine which happens to create a directory as well. The
> main difference is that posix_mkdir does *NOT* return an open
> handle. As we did not enter this code path due to the SMB_O_RDONLY we
> leak a SMB1 fd per cli_posix_mkdir call.
>
> Pretty hard to test automatically, this would be an interaction with
> smbstatus.
>
> Signed-off-by: Volker Lendecke <vl at samba.org>
> ---
> source3/libsmb/clifile.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/source3/libsmb/clifile.c b/source3/libsmb/clifile.c
> index 61bc73effa2..ff98ba60034 100644
> --- a/source3/libsmb/clifile.c
> +++ b/source3/libsmb/clifile.c
> @@ -5348,7 +5348,7 @@ struct tevent_req *cli_posix_mkdir_send(TALLOC_CTX *mem_ctx,
> state->ev = ev;
> state->cli = cli;
>
> - wire_flags = open_flags_to_wire(O_CREAT) | SMB_O_DIRECTORY;
> + wire_flags = SMB_O_CREAT | SMB_O_DIRECTORY;
>
> subreq = cli_posix_open_internal_send(
> mem_ctx, ev, cli, fname, wire_flags, mode);
> --
> 2.11.0
>
More information about the samba-technical
mailing list