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 <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.
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