Libguestfs - Feb 2014 - Re: [PATCH 2/3] New API: part-get-name (RHBZ#593511).

On Tue, Feb 04, 2014 at 04:01:32PM +0100, Pino Toscano
wrote:
> +static char *
> +extract_optionally_quoted (const char *value)
> +{
> +  size_t value_len = strlen (value);
> +
> +  if (value_len >= 2 &&
> +      ((value[0] == '\'' && value[value_len - 1] ==
'\'') ||
> +       (value[0] == '"' && value[value_len - 1] ==
'"'))) {
> +    value_len -= 2;
> +    ++value;
> +  }
> +
> +  char *ret = strndup (value, value_len);
> +  if (ret == NULL) {
> +    reply_with_perror ("strndup");
> +    return NULL;
> +  }
> +
> +  return ret;
> +}
My spidey sense asks what happens if the value contains quote
characters?  I wonder if sgdisk escapes them.

Rest of the patch looks fine, but I think you should try setting the
partition name to a few weird values and see what sgdisk does.

Rich.

-- 
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
virt-df lists disk usage of guests without needing to install any
software inside the virtual machine.  Supports Linux and Windows.
http://people.redhat.com/~rjones/virt-df/

On Tuesday 04 February 2014 15:59:36 Richard W.M. Jones
wrote:
> On Tue, Feb 04, 2014 at 04:01:32PM +0100, Pino Toscano wrote:
> > +static char *
> > +extract_optionally_quoted (const char *value)
> > +{
> > +  size_t value_len = strlen (value);
> > +
> > +  if (value_len >= 2 &&
> > +      ((value[0] == '\'' && value[value_len - 1]
== '\'') ||
> > +       (value[0] == '"' && value[value_len - 1]
== '"'))) {
> > +    value_len -= 2;
> > +    ++value;
> > +  }
> > +
> > +  char *ret = strndup (value, value_len);
> > +  if (ret == NULL) {
> > +    reply_with_perror ("strndup");
> > +    return NULL;
> > +  }
> > +
> > +  return ret;
> > +}
> 
> My spidey sense asks what happens if the value contains quote
> characters?  I wonder if sgdisk escapes them.
> 
> Rest of the patch looks fine, but I think you should try setting the
> partition name to a few weird values and see what sgdisk does.
It seems sgdisk just picks whatever it is being passed to it:

  ><rescue> sgdisk -c 3:test /dev/sda
  [  129.149627]  sda: sda1 sda2 sda3
  The operation has completed successfully.
  ><rescue> sgdisk -i 3 /dev/sda | grep name
  Partition name: 'test'
  ><rescue> sgdisk -c 3:test /dev/sda
  [  146.992530]  sda: sda1 sda2 sda3
  The operation has completed successfully.
  ><rescue> sgdisk -i 3 /dev/sda | grep name
  Partition name: 'test'
  ><rescue> sgdisk -c 3:"test" /dev/sda
  [  194.584171]  sda: sda1 sda2 sda3
  The operation has completed successfully.
  ><rescue> sgdisk -i 3 /dev/sda | grep name
  Partition name: 'test'
  ><rescue> sgdisk -c 3:'test' /dev/sda
  [  202.042122]  sda: sda1 sda2 sda3
  The operation has completed successfully.
  ><rescue> sgdisk -i 3 /dev/sda | grep name
  Partition name: 'test'
  ><rescue> sgdisk -c 3:"'test'" /dev/sda
  [  211.002185]  sda: sda1 sda2 sda3
  The operation has completed successfully.
  ><rescue> sgdisk -i 3 /dev/sda | grep name
  Partition name: ''test''
  ><rescue> sgdisk -c 3:'"test"' /dev/sda
  [  217.441540]  sda: sda1 sda2 sda3
  The operation has completed successfully.
  ><rescue> sgdisk -i 3 /dev/sda | grep name
  Partition name: '"test"'

So the output is always single-quoted, with other quote characters just 
appearing as they were in the partition label.

-- 
Pino Toscano

On Tue, Feb 04, 2014 at 05:20:22PM +0100, Pino Toscano
wrote:
> On Tuesday 04 February 2014 15:59:36 Richard W.M. Jones wrote:
> > On Tue, Feb 04, 2014 at 04:01:32PM +0100, Pino Toscano wrote:
> > > +static char *
> > > +extract_optionally_quoted (const char *value)
> > > +{
> > > +  size_t value_len = strlen (value);
> > > +
> > > +  if (value_len >= 2 &&
> > > +      ((value[0] == '\'' && value[value_len
- 1] == '\'') ||
> > > +       (value[0] == '"' && value[value_len
- 1] == '"'))) {
> > > +    value_len -= 2;
> > > +    ++value;
> > > +  }
> > > +
> > > +  char *ret = strndup (value, value_len);
> > > +  if (ret == NULL) {
> > > +    reply_with_perror ("strndup");
> > > +    return NULL;
> > > +  }
> > > +
> > > +  return ret;
> > > +}
> > 
> > My spidey sense asks what happens if the value contains quote
> > characters?  I wonder if sgdisk escapes them.
> > 
> > Rest of the patch looks fine, but I think you should try setting the
> > partition name to a few weird values and see what sgdisk does.
> 
> It seems sgdisk just picks whatever it is being passed to it:
> 
>   ><rescue> sgdisk -c 3:test /dev/sda
>   [  129.149627]  sda: sda1 sda2 sda3
>   The operation has completed successfully.
>   ><rescue> sgdisk -i 3 /dev/sda | grep name
>   Partition name: 'test'
>   ><rescue> sgdisk -c 3:test /dev/sda
>   [  146.992530]  sda: sda1 sda2 sda3
>   The operation has completed successfully.
>   ><rescue> sgdisk -i 3 /dev/sda | grep name
>   Partition name: 'test'
>   ><rescue> sgdisk -c 3:"test" /dev/sda
>   [  194.584171]  sda: sda1 sda2 sda3
>   The operation has completed successfully.
>   ><rescue> sgdisk -i 3 /dev/sda | grep name
>   Partition name: 'test'
>   ><rescue> sgdisk -c 3:'test' /dev/sda
>   [  202.042122]  sda: sda1 sda2 sda3
>   The operation has completed successfully.
>   ><rescue> sgdisk -i 3 /dev/sda | grep name
>   Partition name: 'test'
>   ><rescue> sgdisk -c 3:"'test'" /dev/sda
>   [  211.002185]  sda: sda1 sda2 sda3
>   The operation has completed successfully.
>   ><rescue> sgdisk -i 3 /dev/sda | grep name
>   Partition name: ''test''
>   ><rescue> sgdisk -c 3:'"test"' /dev/sda
>   [  217.441540]  sda: sda1 sda2 sda3
>   The operation has completed successfully.
>   ><rescue> sgdisk -i 3 /dev/sda | grep name
>   Partition name: '"test"'
> 
> So the output is always single-quoted, with other quote characters just 
> appearing as they were in the partition label.
OK, I'm sure that will catch us out in future when sgdisk
changes/fixes it :-(

ACK.

Rich.

-- 
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming blog: http://rwmj.wordpress.com
Fedora now supports 80 OCaml packages (the OPEN alternative to F#)