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

[Anthony PERARD <anthony.perard@citrix.com>]

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,

-- 
Anthony PERARD