Message ID | 201301241729.31903.hahn@univention.de |
---|---|
State | New |
Headers | show |
Philipp Hahn <hahn@univention.de> writes: > Hello, > > I tried to open a "twoGbMaxExtentSparse" VMDK file, which uses spaces in its > own and for the referenced file names. This breaks in line 646 of > block/vmdk.c because "%511s" stops at the first space and thus fname is > incomplete: > ret = sscanf(p, "%10s %" SCNd64 " %10s %511s %" SCNd64, > access, §ors, type, fname, &flat_offset); > > I've only checked with our very old VMware workstation version, which refuses > to create new images with unsupported characters with the following message: >> The characters !#%^&*><:;'"<>/? cannot be used. > So it looks like spaces are valid, at least we have several VMs with spaces in > their name. > > If the quotes around the file name are required, the simpliest solution would > be to change %511s to "%511[^"]": > > diff --git a/block/vmdk.c b/block/vmdk.c > index 19298c2..045f6a1 100644 > --- a/block/vmdk.c > +++ b/block/vmdk.c > @@ -641,7 +641,7 @@ static int vmdk_parse_extents(const char *desc, > BlockDriverState *bs, > * RW [size in sectors] SPARSE "file-name.vmdk" > */ > flat_offset = -1; > - ret = sscanf(p, "%10s %" SCNd64 " %10s %511s %" SCNd64, > + ret = sscanf(p, "%10s %" SCNd64 " %10s \"%511[^\"]\" %" SCNd64, > access, §ors, type, fname, &flat_offset); > if (ret < 4 || strcmp(access, "RW")) { > goto next_line; Suggest to include '\n' in the stop set, like \"%511[^\"\n]\", to better detect malformed input. > I don't know how portable %[ together with a maximum width is, because the > manual page for sscanf() doesn't mention "max width" for "%[", but it works > with Debian/GNU Linux Squeeze. It's fine according to my reading of C89. I'm afraid your patch is flawed. For RW 1048576 FLAT ""test-f001.vmdk"" 0 fname is now "test-f001.vmdk" instead of "\"test-f001.vmdk\"". That's because you change sscanf() to ignore the double-quotes without dropping the quote stripping code below. Care to post a fixed up patch?
On Thu, Jan 24, 2013 at 05:29:27PM +0100, Philipp Hahn wrote: > Hello, > > I tried to open a "twoGbMaxExtentSparse" VMDK file, which uses spaces in its > own and for the referenced file names. This breaks in line 646 of > block/vmdk.c because "%511s" stops at the first space and thus fname is > incomplete: > ret = sscanf(p, "%10s %" SCNd64 " %10s %511s %" SCNd64, > access, §ors, type, fname, &flat_offset); > > I've only checked with our very old VMware workstation version, which refuses > to create new images with unsupported characters with the following message: > > The characters !#%^&*><:;'"<>/? cannot be used. > So it looks like spaces are valid, at least we have several VMs with spaces in > their name. > > If the quotes around the file name are required, the simpliest solution would > be to change %511s to "%511[^"]": > > diff --git a/block/vmdk.c b/block/vmdk.c > index 19298c2..045f6a1 100644 > --- a/block/vmdk.c > +++ b/block/vmdk.c > @@ -641,7 +641,7 @@ static int vmdk_parse_extents(const char *desc, > BlockDriverState *bs, > * RW [size in sectors] SPARSE "file-name.vmdk" > */ > flat_offset = -1; > - ret = sscanf(p, "%10s %" SCNd64 " %10s %511s %" SCNd64, > + ret = sscanf(p, "%10s %" SCNd64 " %10s \"%511[^\"]\" %" SCNd64, > access, §ors, type, fname, &flat_offset); > if (ret < 4 || strcmp(access, "RW")) { > goto next_line; > > I don't know how portable %[ together with a maximum width is, because the > manual page for sscanf() doesn't mention "max width" for "%[", but it works > with Debian/GNU Linux Squeeze. sscanf(3) is from the C standard. I checked that C99 specifies the length modifier for %[ in "7.19.6.2 The fscanf function" paragraph 12. I also did a quick sample of vmdk parsers on the net. It seems filenames are always double-quoted. The file format specification also shows it this way but never explicitly states if they are optional or not. Your fix looks good. Please also drop the '"' trimming code below and resend with Signed-off-by:. Thanks, Stefan
Hello, On Friday 25 January 2013 09:37:39 Markus Armbruster wrote: > Suggest to include '\n' in the stop set, like \"%511[^\"\n]\", to better > detect malformed input. Will do, also \r. > > I don't know how portable %[ together with a maximum width is, because > > the manual page for sscanf() doesn't mention "max width" for "%[", but it > > works with Debian/GNU Linux Squeeze. > > It's fine according to my reading of C89. Thank you for lloking this up. > I'm afraid your patch is flawed. For > > RW 1048576 FLAT ""test-f001.vmdk"" 0 You seem to assume " is allowed in the file name, I've assumed that " is not allowed, since we don't know the quoting rules for vmdk files, if there are any. That's why I checked our old VMware workstation, which refused to create volumes containing !#%^&*><:;'"<>/? Should we print a warning or error out if a " is detected? > fname is now "test-f001.vmdk" instead of "\"test-f001.vmdk\"". That's > because you change sscanf() to ignore the double-quotes without dropping > the quote stripping code below. I'll remove the stripping code. > Care to post a fixed up patch? Will do so. Sincerely Philipp
diff --git a/block/vmdk.c b/block/vmdk.c index 19298c2..045f6a1 100644 --- a/block/vmdk.c +++ b/block/vmdk.c @@ -641,7 +641,7 @@ static int vmdk_parse_extents(const char *desc, BlockDriverState *bs, * RW [size in sectors] SPARSE "file-name.vmdk" */ flat_offset = -1; - ret = sscanf(p, "%10s %" SCNd64 " %10s %511s %" SCNd64, + ret = sscanf(p, "%10s %" SCNd64 " %10s \"%511[^\"]\" %" SCNd64, access, §ors, type, fname, &flat_offset); if (ret < 4 || strcmp(access, "RW")) {
Hello, I tried to open a "twoGbMaxExtentSparse" VMDK file, which uses spaces in its own and for the referenced file names. This breaks in line 646 of block/vmdk.c because "%511s" stops at the first space and thus fname is incomplete: ret = sscanf(p, "%10s %" SCNd64 " %10s %511s %" SCNd64, access, §ors, type, fname, &flat_offset); I've only checked with our very old VMware workstation version, which refuses to create new images with unsupported characters with the following message: > The characters !#%^&*><:;'"<>/? cannot be used. So it looks like spaces are valid, at least we have several VMs with spaces in their name. If the quotes around the file name are required, the simpliest solution would be to change %511s to "%511[^"]": goto next_line; I don't know how portable %[ together with a maximum width is, because the manual page for sscanf() doesn't mention "max width" for "%[", but it works with Debian/GNU Linux Squeeze. Sincerely Philipp