Re: [PATCH] Path string from the host should not be treated as wchar_t

[Easwar Hariharan <eahariha@linux.microsoft.com>]

On 6/1/2025 6:45 AM, yasuenag@gmail.com wrote:
> From: Yasumasa Suenaga <yasuenag@gmail.com>
> 
> hv_fcopy_uio_daemon handles file copy request from the host.
> (e.g. Copy-VMFile commandlet)
> The request has file path and its name, they would be stored as
> __u16 arrays in struct hv_start_fcopy. They are casted to wchar_t*
> in fcopyd to convert to UTF-8 string. wchar_t is 32bit in Linux
> unlike Windows (16bit), so string conversion would be failed and
> the user cannot copy file to Linux guest from Host via fcopyd.
> 
> fcopyd converts each characters to char if the value is less
> than 0x80. Thus we can convert straightly without wcstombs() call,
> it means we are no longer to convert to wchar_t.
> 
> Length of path depends on PATH_MAX (Linux) and W_MAX_PATH (Windows),
> so this change also addes new check to snprintf() call to make
> target path.

Missing Signed-off-by for developer certificate of origin

> ---
>  tools/hv/hv_fcopy_uio_daemon.c | 38 ++++++++++++++--------------------
>  1 file changed, 16 insertions(+), 22 deletions(-)
> 

Thanks for the patch! Please look through https://www.kernel.org/doc/html/latest/process/submitting-patches.html
to see hints for the expected subject line, explanation body, and developer's certificate of origin

+CC: Saurabh for review, weird why get_maintainer.pl didn't get him 


> diff --git a/tools/hv/hv_fcopy_uio_daemon.c b/tools/hv/hv_fcopy_uio_daemon.c
> index 0198321d1..049d4fd9c 100644
> --- a/tools/hv/hv_fcopy_uio_daemon.c
> +++ b/tools/hv/hv_fcopy_uio_daemon.c
> @@ -58,12 +58,16 @@ static unsigned long long filesize;
>  static int hv_fcopy_create_file(char *file_name, char *path_name, __u32 flags)
>  {
>  	int error = HV_E_FAIL;
> +	int ret_snprintf;
>  	char *q, *p;
>  
>  	filesize = 0;
>  	p = path_name;
> -	snprintf(target_fname, sizeof(target_fname), "%s/%s",
> -		 path_name, file_name);
> +	ret_snprintf = snprintf(target_fname, sizeof(target_fname), "%s/%s",
> +	                        path_name, file_name);
> +	if (ret_snprintf >= sizeof(target_fname))
> +		/* target file name is too long */
> +		goto done;

The error check can be inlined since we don't use ret_snprintf elsewhere, i.e.

if(snprintf(target_fname, sizeof(target_fname), "%s/%s", path_name, file_name) >= sizeof(target_fname)) {
	/* target file name is too long */
	goto done;
}

Note also the added braces, even if the extra line is a comment. https://www.kernel.org/doc/html/latest/process/coding-style.html

>  
>  	/*
>  	 * Check to see if the path is already in place; if not,
> @@ -273,6 +277,8 @@ static void wcstoutf8(char *dest, const __u16 *src, size_t dest_size)
>  	while (len < dest_size) {
>  		if (src[len] < 0x80)
>  			dest[len++] = (char)(*src++);
> +		else if (src[len] == '0')
> +			break;
>  		else
>  			dest[len++] = 'X';
>  	}
> @@ -282,27 +288,15 @@ static void wcstoutf8(char *dest, const __u16 *src, size_t dest_size)
>  
>  static int hv_fcopy_start(struct hv_start_fcopy *smsg_in)
>  {
> -	setlocale(LC_ALL, "en_US.utf8");
> -	size_t file_size, path_size;
> -	char *file_name, *path_name;
> -	char *in_file_name = (char *)smsg_in->file_name;
> -	char *in_path_name = (char *)smsg_in->path_name;
> -
> -	file_size = wcstombs(NULL, (const wchar_t *restrict)in_file_name, 0) + 1;
> -	path_size = wcstombs(NULL, (const wchar_t *restrict)in_path_name, 0) + 1;
> -
> -	file_name = (char *)malloc(file_size * sizeof(char));
> -	path_name = (char *)malloc(path_size * sizeof(char));
> -
> -	if (!file_name || !path_name) {
> -		free(file_name);
> -		free(path_name);
> -		syslog(LOG_ERR, "Can't allocate memory for file name and/or path name");
> -		return HV_E_FAIL;
> -	}
> +	/*
> +	 * file_name and path_name should have same length with appropriate
> +	 * member of hv_start_fcopy.
> +	*/
> +	char file_name[W_MAX_PATH], path_name[W_MAX_PATH];
>  
> -	wcstoutf8(file_name, (__u16 *)in_file_name, file_size);
> -	wcstoutf8(path_name, (__u16 *)in_path_name, path_size);
> +	setlocale(LC_ALL, "en_US.utf8");
> +	wcstoutf8(file_name, smsg_in->file_name, W_MAX_PATH - 1);
> +	wcstoutf8(path_name, smsg_in->path_name, W_MAX_PATH - 1);
>  
>  	return hv_fcopy_create_file(file_name, path_name, smsg_in->copy_flags);
>  }