Re: [PATCH v2 2/4] tools: allow vchan XenStore paths more then 64 bytes long

[Juergen Gross <jgross@xxxxxxxx>]

On 01.08.22 11:11, Dmytro Semenets wrote:
> пн, 1 авг. 2022 г. в 11:59, Juergen Gross <jgross@xxxxxxxx>:
> > On 13.07.22 17:03, dmitry.semenets@xxxxxxxxx wrote:
> > > From: Oleksandr Andrushchenko <oleksandr_andrushchenko@xxxxxxxx>
> > >
> > > Current vchan implementation, while dealing with XenStore paths,
> > > allocates 64 bytes buffer on the stack which may not be enough for
> > > some use-cases. Make the buffer longer to respect maximum allowed
> > > XenStore path of XENSTORE_ABS_PATH_MAX.
> > >
> > > Signed-off-by: Oleksandr Andrushchenko <oleksandr_andrushchenko@xxxxxxxx>
> > > Signed-off-by: Dmytro Semenets <dmytro_semenets@xxxxxxxx>
> > > ---
> > >    tools/libs/vchan/init.c | 28 ++++++++++++++++++++++------
> > >    1 file changed, 22 insertions(+), 6 deletions(-)
> > >
> > > diff --git a/tools/libs/vchan/init.c b/tools/libs/vchan/init.c
> > > index 9195bd3b98..38658f30af 100644
> > > --- a/tools/libs/vchan/init.c
> > > +++ b/tools/libs/vchan/init.c
> > > @@ -249,7 +249,7 @@ static int init_xs_srv(struct libxenvchan *ctrl, int 
> > > domain, const char* xs_base
> > >        int ret = -1;
> > >        struct xs_handle *xs;
> > >        struct xs_permissions perms[2];
> > > -     char buf[64];
> > > +     char *buf;
> > >        char ref[16];
> > >        char* domid_str = NULL;
> > >        xs_transaction_t xs_trans = XBT_NULL;
> > > @@ -259,6 +259,12 @@ static int init_xs_srv(struct libxenvchan *ctrl, int 
> > > domain, const char* xs_base
> > >        if (!ctrl->xs_path)
> > >                return -1;
> > >
> > > +     buf = malloc(XENSTORE_ABS_PATH_MAX);
> > > +     if (!buf) {
> > > +             free(ctrl);
> > > +             return 0;
> > > +     }
> > > +
> > >        xs = xs_open(0);
> > >        if (!xs)
> > >                goto fail;
> > > @@ -280,14 +286,14 @@ retry_transaction:
> > >                goto fail_xs_open;
> > >
> > >        snprintf(ref, sizeof ref, "%d", ring_ref);
> > > -     snprintf(buf, sizeof buf, "%s/ring-ref", xs_base);
> > > +     snprintf(buf, XENSTORE_ABS_PATH_MAX, "%s/ring-ref", xs_base);
> > >        if (!xs_write(xs, xs_trans, buf, ref, strlen(ref)))
> > >                goto fail_xs_open;
> > >        if (!xs_set_permissions(xs, xs_trans, buf, perms, 2))
> > >                goto fail_xs_open;
> > >
> > >        snprintf(ref, sizeof ref, "%d", ctrl->event_port);
> > > -     snprintf(buf, sizeof buf, "%s/event-channel", xs_base);
> > > +     snprintf(buf, XENSTORE_ABS_PATH_MAX, "%s/event-channel", xs_base);
> > >        if (!xs_write(xs, xs_trans, buf, ref, strlen(ref)))
> > >                goto fail_xs_open;
> > >        if (!xs_set_permissions(xs, xs_trans, buf, perms, 2))
> > > @@ -303,6 +309,7 @@ retry_transaction:
> > >        free(domid_str);
> > >        xs_close(xs);
> > >     fail:
> > > +     free(buf);
> > >        return ret;
> > >    }
> > >
> > > @@ -419,13 +426,20 @@ struct libxenvchan *libxenvchan_client_init(struct 
> > > xentoollog_logger *logger,
> > >    {
> > >        struct libxenvchan *ctrl = malloc(sizeof(struct libxenvchan));
> > >        struct xs_handle *xs = NULL;
> > > -     char buf[64];
> > > +     char *buf;
> > >        char *ref;
> > >        int ring_ref;
> > >        unsigned int len;
> > >
> > >        if (!ctrl)
> > >                return 0;
> > > +
> > > +     buf = malloc(XENSTORE_ABS_PATH_MAX);
> > > +     if (!buf) {
> > > +             free(ctrl);
> > > +             return 0;
> > > +     }
> > > +
> > >        ctrl->ring = NULL;
> > >        ctrl->event = NULL;
> > >        ctrl->gnttab = NULL;
> > > @@ -436,8 +450,9 @@ struct libxenvchan *libxenvchan_client_init(struct 
> > > xentoollog_logger *logger,
> > >        if (!xs)
> > >                goto fail;
> > >
> > > +
> > >    // find xenstore entry
> > > -     snprintf(buf, sizeof buf, "%s/ring-ref", xs_path);
> > > +     snprintf(buf, XENSTORE_ABS_PATH_MAX, "%s/ring-ref", xs_path);
> > >        ref = xs_read(xs, 0, buf, &len);
> > >        if (!ref)
> > >                goto fail;
> > > @@ -445,7 +460,7 @@ struct libxenvchan *libxenvchan_client_init(struct 
> > > xentoollog_logger *logger,
> > >        free(ref);
> > >        if (!ring_ref)
> > >                goto fail;
> > > -     snprintf(buf, sizeof buf, "%s/event-channel", xs_path);
> > > +     snprintf(buf, XENSTORE_ABS_PATH_MAX, "%s/event-channel", xs_path);
> > >        ref = xs_read(xs, 0, buf, &len);
> > >        if (!ref)
> > >                goto fail;
> > > @@ -475,6 +490,7 @@ struct libxenvchan *libxenvchan_client_init(struct 
> > > xentoollog_logger *logger,
> > >     out:
> > >        if (xs)
> > >                xs_close(xs);
> > > +     free(buf);
> > >        return ctrl;
> > >     fail:
> > >        libxenvchan_close(ctrl);
> > I think you are leaking buf in case of "goto fail".
> No. File with patch doesn't have follows lines:
>      ctrl = NULL;
>      goto out;
> }
Oh, what a nasty control flow!

You are right, sorry for the noise.

Reviewed-by: Juergen Gross <jgross@xxxxxxxx>


Juergen

Attachment: OpenPGP_0xB0DE9DD628BF132F.asc
Description: OpenPGP public key

Attachment: OpenPGP_signature
Description: OpenPGP digital signature