[PATCH] Fix valgrind read-after-free error in cli_smb2_close_fnum_recv().

classic Classic list List threaded Threaded
6 messages Options
Reply | Threaded
Open this post in threaded view
|

[PATCH] Fix valgrind read-after-free error in cli_smb2_close_fnum_recv().

Samba - samba-technical mailing list
cli_smb2_close_fnum_recv() uses tevent_req_simple_recv_ntstatus(req), which
frees req, then uses the state pointer which was owned by req.

Please review and push.

Thanks,

        Jeremy.

0001-s3-libsmb-Fix-valgrind-read-after-free-error-in-cli_.patch (1K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] Fix valgrind read-after-free error in cli_smb2_close_fnum_recv().

Samba - samba-technical mailing list
On Wed, Nov 29, 2017 at 09:33:43AM -0800, Jeremy Allison wrote:
> cli_smb2_close_fnum_recv() uses tevent_req_simple_recv_ntstatus(req), which
> frees req, then uses the state pointer which was owned by req.
>
> Please review and push.

looks complicated. What about the attached version?

-slow

--
Ralph Boehme, Samba Team       https://samba.org/
Samba Developer, SerNet GmbH   https://sernet.de/en/samba/

bug13171-master.patch (1K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] Fix valgrind read-after-free error in cli_smb2_close_fnum_recv().

Samba - samba-technical mailing list
On Wed, Nov 29, 2017 at 07:06:08PM +0100, Ralph Böhme wrote:
> On Wed, Nov 29, 2017 at 09:33:43AM -0800, Jeremy Allison wrote:
> > cli_smb2_close_fnum_recv() uses tevent_req_simple_recv_ntstatus(req), which
> > frees req, then uses the state pointer which was owned by req.
> >
> > Please review and push.
>
> looks complicated. What about the attached version?

Yeah, I actually coded that up first and rejected it :-), because
it still leaves the tevent_req_simple_recv_ntstatus(req)
inside, which (silently) frees the data inside req.

There's no indication in that API that req->state is
gone, so I wanted to leave tevent_req_simple_recv_ntstatus(req)
only for cases that look like:

NTSTATUS XXX_recv(struct tevent_req *req)
{
        return tevent_req_simple_recv_ntstatus(req);
}

But if you really want your version, I'm good with
anything that removes the valgrind error really :-).

Jeremy.

> --
> Ralph Boehme, Samba Team       https://samba.org/
> Samba Developer, SerNet GmbH   https://sernet.de/en/samba/

> From da7e28bf7d57ef974a88825a3d78444bac111a2b Mon Sep 17 00:00:00 2001
> From: Ralph Boehme <[hidden email]>
> Date: Wed, 29 Nov 2017 19:03:40 +0100
> Subject: [PATCH] s3: libsmb: Fix valgrind read-after-free error in
>  cli_smb2_close_fnum_recv().
>
> cli_smb2_close_fnum_recv() uses tevent_req_simple_recv_ntstatus(req), which
> frees req, then uses the state pointer which was owned by req.
>
> BUG: https://bugzilla.samba.org/show_bug.cgi?id=13171
>
> Signed-off-by: Ralph Boehme <[hidden email]>
> ---
>  source3/libsmb/cli_smb2_fnum.c | 7 ++++---
>  1 file changed, 4 insertions(+), 3 deletions(-)
>
> diff --git a/source3/libsmb/cli_smb2_fnum.c b/source3/libsmb/cli_smb2_fnum.c
> index 628b17b293b..a827817b45e 100644
> --- a/source3/libsmb/cli_smb2_fnum.c
> +++ b/source3/libsmb/cli_smb2_fnum.c
> @@ -449,9 +449,10 @@ NTSTATUS cli_smb2_close_fnum_recv(struct tevent_req *req)
>  {
>   struct cli_smb2_close_fnum_state *state = tevent_req_data(
>   req, struct cli_smb2_close_fnum_state);
> - NTSTATUS status = tevent_req_simple_recv_ntstatus(req);
> - state->cli->raw_status = status;
> - return status;
> + struct cli_state *cli = state->cli;
> +
> + cli->raw_status  = tevent_req_simple_recv_ntstatus(req);
> + return cli->raw_status;
>  }
>  
>  NTSTATUS cli_smb2_close_fnum(struct cli_state *cli, uint16_t fnum)
> --
> 2.13.6
>


Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] Fix valgrind read-after-free error in cli_smb2_close_fnum_recv().

Samba - samba-technical mailing list
On Wed, Nov 29, 2017 at 10:13:00AM -0800, Jeremy Allison wrote:

> On Wed, Nov 29, 2017 at 07:06:08PM +0100, Ralph Böhme wrote:
> > On Wed, Nov 29, 2017 at 09:33:43AM -0800, Jeremy Allison wrote:
> > > cli_smb2_close_fnum_recv() uses tevent_req_simple_recv_ntstatus(req), which
> > > frees req, then uses the state pointer which was owned by req.
> > >
> > > Please review and push.
> >
> > looks complicated. What about the attached version?
>
> Yeah, I actually coded that up first and rejected it :-), because
> it still leaves the tevent_req_simple_recv_ntstatus(req)
> inside, which (silently) frees the data inside req.
>
> There's no indication in that API that req->state is
> gone, so I wanted to leave tevent_req_simple_recv_ntstatus(req)
> only for cases that look like:
>
> NTSTATUS XXX_recv(struct tevent_req *req)
> {
> return tevent_req_simple_recv_ntstatus(req);
> }
>
> But if you really want your version, I'm good with
> anything that removes the valgrind error really :-).

ok, I see your point. And given the fact that there's another function that gets
it wrong (finddcs_nbt_recv, the caller finddcs_nbt() doesn't seem to be used
anywhere), let's go with your version.

RB: me.

-slow

--
Ralph Boehme, Samba Team       https://samba.org/
Samba Developer, SerNet GmbH   https://sernet.de/en/samba/

Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] Fix valgrind read-after-free error in cli_smb2_close_fnum_recv().

Samba - samba-technical mailing list
In reply to this post by Samba - samba-technical mailing list
On Wed, Nov 29, 2017 at 10:13:00AM -0800, Jeremy Allison via samba-technical wrote:
> Yeah, I actually coded that up first and rejected it :-), because
> it still leaves the tevent_req_simple_recv_ntstatus(req)
> inside, which (silently) frees the data inside req.

Probably this is because I did not comment that sufficiently: Those
simple_recv_ functions are really meant as a short-cut if there is
nothing but this single call in the _recv function. If you have to do
anything but this call, do it manually. That was at least my intention
when I wrote this function. Sorry if this is not clear enough.

Volker

--
SerNet GmbH, Bahnhofsallee 1b, 37081 Göttingen
phone: +49-551-370000-0, fax: +49-551-370000-9
AG Göttingen, HRB 2816, GF: Dr. Johannes Loxen
http://www.sernet.de, mailto:[hidden email]

Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] Fix valgrind read-after-free error in cli_smb2_close_fnum_recv().

Samba - samba-technical mailing list
On Thu, Nov 30, 2017 at 08:13:08AM +0100, Volker Lendecke wrote:

> On Wed, Nov 29, 2017 at 10:13:00AM -0800, Jeremy Allison via samba-technical wrote:
> > Yeah, I actually coded that up first and rejected it :-), because
> > it still leaves the tevent_req_simple_recv_ntstatus(req)
> > inside, which (silently) frees the data inside req.
>
> Probably this is because I did not comment that sufficiently: Those
> simple_recv_ functions are really meant as a short-cut if there is
> nothing but this single call in the _recv function. If you have to do
> anything but this call, do it manually. That was at least my intention
> when I wrote this function. Sorry if this is not clear enough.

Yes, it's clear now (at least to me). I think maybe a later
patch that adds a comment to these calls that explains the
correct idiom might help prevent future mis-use.