diff mbox series

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

Message ID 20220713150311.4152528-2-dmitry.semenets@gmail.com (mailing list archive)
State New, archived
Headers show
Series [v2,1/4] tools: remove xenstore entries on vchan server closure | expand

Commit Message

Dmytro Semenets July 13, 2022, 3:03 p.m. UTC 
From: Oleksandr Andrushchenko <oleksandr_andrushchenko@epam.com>

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@epam.com>
Signed-off-by: Dmytro Semenets <dmytro_semenets@epam.com>
---
 tools/libs/vchan/init.c | 28 ++++++++++++++++++++++------
 1 file changed, 22 insertions(+), 6 deletions(-)

Comments

Jürgen Groß Aug. 1, 2022, 8:59 a.m. UTC | #1 
On 13.07.22 17:03, dmitry.semenets@gmail.com wrote:
> From: Oleksandr Andrushchenko <oleksandr_andrushchenko@epam.com>
> 
> 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@epam.com>
> Signed-off-by: Dmytro Semenets <dmytro_semenets@epam.com>
> ---
>   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".


Juergen
Dmytro Semenets Aug. 1, 2022, 9:11 a.m. UTC | #2 
пн, 1 авг. 2022 г. в 11:59, Juergen Gross <jgross@suse.com>:
>
> On 13.07.22 17:03, dmitry.semenets@gmail.com wrote:
> > From: Oleksandr Andrushchenko <oleksandr_andrushchenko@epam.com>
> >
> > 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@epam.com>
> > Signed-off-by: Dmytro Semenets <dmytro_semenets@epam.com>
> > ---
> >   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
Jürgen Groß Aug. 1, 2022, 9:17 a.m. UTC | #3 
On 01.08.22 11:11, Dmytro Semenets wrote:
> пн, 1 авг. 2022 г. в 11:59, Juergen Gross <jgross@suse.com>:
>>
>> On 13.07.22 17:03, dmitry.semenets@gmail.com wrote:
>>> From: Oleksandr Andrushchenko <oleksandr_andrushchenko@epam.com>
>>>
>>> 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@epam.com>
>>> Signed-off-by: Dmytro Semenets <dmytro_semenets@epam.com>
>>> ---
>>>    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@suse.com>


Juergen
Anthony PERARD Sept. 27, 2022, 3 p.m. UTC | #4 
Hi Dmitry,

On Wed, Jul 13, 2022 at 06:03:09PM +0300, dmitry.semenets@gmail.com wrote:
> 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
> @@ -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;

I don't understand what you are trying to achieve here. If we can't
allocate `buf`, we should return an error, right?
Also, `ctrl` isn't allocated in this function but by the caller, so I
don't think we need to free it here. Also, if it's free here, the caller
is going to continue to use the pointer, after free.

> +	}
> +
>  	xs = xs_open(0);
>  	if (!xs)
>  		goto fail;
> @@ -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;

Nit: could you write NULL instead of 0 here? It would makes it much
easier to understand that we return a pointer.


Thanks,
diff mbox series

Patch

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);