[Top][All Lists]
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [Qemu-trivial] [PATCH] block: Fix regression for MinGW
From: |
Stefan Hajnoczi |
Subject: |
Re: [Qemu-devel] [Qemu-trivial] [PATCH] block: Fix regression for MinGW (assertion caused by short string) |
Date: |
Tue, 20 Nov 2012 14:21:42 +0100 |
User-agent: |
Mutt/1.5.21 (2010-09-15) |
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