Re: [Qemu-devel] [Qemu-trivial] [PATCH] block: Fix regression for MinGW (assertion caused by short string)

[Stefan Hajnoczi <stefanha@gmail.com>]

On Sun, Nov 18, 2012 at 09:26:15AM +0100, Stefan Weil wrote:
> The local string tmp_filename is passed to function get_tmp_filename
> which expects a string with minimum size MAX_PATH for w32 hosts.
> 
> MAX_PATH is 260 and PATH_MAX is 259, so tmp_filename was too short.
> 
> Commit eba25057b9a5e19d10ace2bc7716667a31297169 introduced this
> regression.
> 
> Signed-off-by: Stefan Weil <sw@weilnetz.de>
> ---
>  block.c |    2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/block.c b/block.c
> index 49dd6bb..8739635 100644
> --- a/block.c
> +++ b/block.c
> @@ -787,7 +787,7 @@ int bdrv_open(BlockDriverState *bs, const char *filename, int flags,
>                BlockDriver *drv)
>  {
>      int ret;
> -    char tmp_filename[PATH_MAX];
> +    char tmp_filename[PATH_MAX + 1];

This is not maintainable - the patch is making an assumption about the relative
values of MAX_PATH and PATH_MAX.  There is no comment explaining this so it's
likely to be changed and break in the future again.

A clean solution is to change get_tmp_filename() so the caller doesn't need to
pass in a fixed size:

/*
 * Create a uniquely-named empty temporary file.
 * Return 0 upon success, otherwise a negative errno value.
 *
 * The filename must be freed with g_free() when it is no longer needed.
 */
int get_tmp_filename(char **filename)

The existing get_tmp_filename() code has another problem.  Here is the Windows
get_tmp_filename() code:

    char temp_dir[MAX_PATH];
    /* GetTempFileName requires that its output buffer (4th param)
       have length MAX_PATH or greater.  */
    assert(size >= MAX_PATH);
    return (GetTempPath(MAX_PATH, temp_dir)
            && GetTempFileName(temp_dir, "qem", 0, filename)
            ? 0 : -GetLastError());

The assert has an off-by-one error because the documentation says:

  "This buffer should be MAX_PATH characters to accommodate the path plus the
   terminating null character."
  http://msdn.microsoft.com/en-us/library/windows/desktop/aa364991(v=vs.85).aspx

Since the function returns -errno the assert could be turned into:

  /* GetTempFileName requires that its output buffer (4th param)
     have length MAX_PATH + 1 or greater.  */
  if (size < MAX_PATH + 1) {
      return -ENOSPC;
  }

Stefan