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

[Dmytro Semenets <dmitry.semenets@xxxxxxxxx>]

пн, 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;
}
>
>
> Juergen