[BUG,RFC] block/vmdk.c: File name with space fails to open

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, &sectors, 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

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, &sectors, 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, &sectors, 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, &sectors, 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, &sectors, 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