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 <s...@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.
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.
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)
For block/vvfat, that would even simplify the code.
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
In a short experiment, I was able to create filenames with up to 228
characters
using GetTempFileName, so even 229 bytes instead of MAX_PATH (260) would
be sufficient.
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.
Regards,
Stefan
