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

[Dmytro Semenets <dmitry.semenets@xxxxxxxxx>]

Hello Juergen,
пн, 11 июл. 2022 г. в 10:24, Juergen Gross <jgross@xxxxxxxx>:
>
> On 09.07.22 12:10, 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>
> > ---
> >   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 c6b8674ef5..17945c24a1 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;
> > @@ -257,6 +257,12 @@ static int init_xs_srv(struct libxenvchan *ctrl, int 
> > domain, const char* xs_base
> >       // store the base path so we can clean up on server closure
> >       ctrl->xs_path = strdup(xs_base);
> >
> > +     buf = malloc(XENSTORE_ABS_PATH_MAX);
> > +     if (!buf) {
> > +             free(ctrl);
> > +             return 0;
> > +     }
> > +
> >       xs = xs_open(0);
> >       if (!xs)
> >               goto fail;
> > @@ -278,14 +284,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))
> > @@ -301,6 +307,7 @@ retry_transaction:
> >       free(domid_str);
> >       xs_close(xs);
> >    fail:
> > +     free(buf);
> >       return ret;
> >   }
> >
> > @@ -418,13 +425,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;
> > @@ -435,8 +449,9 @@ struct libxenvchan *libxenvchan_client_init(struct 
> > xentoollog_logger *logger,
> >       if (!xs)
> >               goto fail;
>
> You are leaking buf in this case.
No. buf released in line 474. ctrl leaks in fail case
>
> >
> > +
> >   // 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;
> > @@ -444,7 +459,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;
> > @@ -474,6 +489,7 @@ struct libxenvchan *libxenvchan_client_init(struct 
> > xentoollog_logger *logger,
> >    out:
> >       if (xs)
> >               xs_close(xs);
> > +     free(buf);
> >       return ctrl;
> >    fail:
> >       libxenvchan_close(ctrl);
>
> Juergen