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

[Stefan Hajnoczi]

On Mon, Nov 19, 2012 at 06:38:16PM +0100, Stefan Weil wrote:
> Am 19.11.2012 12:31, schrieb Stefan Hajnoczi:
> >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 <address@hidden>
> >>---
> >>  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.
> 
> The relation between MAX_PATH and PATH_MAX seems to be well definedand
> is valid for w32 and w64, so there is no need to make any assumption:
> 
> buffers must allocate MAX_PATH elements for up to PATH_MAX
> character entries plus a terminating 0.
> 
> I considered writing a comment, but decided not to do so because
> caller and called function are in the same file and most people
> are not very interested in Windows peculiarities.
> 
> The code in block/vvfat.c which uses a length of 1024without
> explanation is worse.

Since there are only two callers it's not a big effort to rewrite the
function so it allocates the string.  That way the platform-specific
length requirement doesn't leak to the caller.

Interesting related patch from Eric Blake a few years ago ;-):
http://permalink.gmane.org/gmane.comp.gnu.mingw.devel/4089

> >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
> 
> No. The documentation can be read in two ways:
> 
>   "This buffer should be (MAX_PATH characters) to accommodate (the path plus 
> the
>    terminating null character)."
> 
> 
>   "This buffer should be (MAX_PATH characters to accommodate the path) plus 
> (the
>    terminating null character)."
> 
> 
> The first one matches the current code and also the example from MS:
> http://msdn.microsoft.com/en-us/library/windows/desktop/aa363875%28v=vs.85%29.aspx

After reading more on MSDN it looks like interpretation #1 is correct.
I thought the NUL character needs to be added on afterwards.

> >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
> 
> "if (size < MAX_PATH) {"
> 
> I'd still prefer the assertion because that is not a general purpose
> library function but a QEMU internal function which must be called
> with correct parameters. Here raising an assertion is better than
> silently returning an error status. Of course we could do both.
> 
> Any patch which fixes this regression for MinGW is fine -
> my patch was simply the smallest possible change to achieve this goal,
> and I still think that it is sufficient.
> 
> If we want a better solution, we should also consider g_mkstemp
> and related functions like g_file_open_tmp. We could also modify
> bdrv_create to allow a NULL filename which is interpreted as a
> temporary filename. IMHO that would be a good solution for the
> future (= after 1.3). Feel free to add a TODO comment to the
> code in my patch.

/* TODO extra byte is a hack to ensure MAX_PATH space on Windows */

Stefan