Libguestfs - Feb 2016 - Re: [PATCH 1/3] launch: add internal helper for socket paths creation

On Tuesday 02 February 2016 19:47:12 Richard W.M. Jones
wrote:
> On Tue, Feb 02, 2016 at 03:27:39PM +0100, Pino Toscano wrote:
> > diff --git a/src/launch.c b/src/launch.c
> > index f59818f..ec061e3 100644
> > --- a/src/launch.c
> > +++ b/src/launch.c
> > @@ -418,6 +418,21 @@ guestfs_int_get_cpu_model (int kvm)
> >  #endif
> >  }
> >  
> > +/* Create the path for a socket with the selected filename in the
> > + * tmpdir.
> > + */
> > +int
> > +guestfs_int_create_socketname (guestfs_h *g, const char *filename,
> > +                               char (*sockpath)[UNIX_PATH_MAX])
> > +{
> > +  char *path = g->tmpdir;
> > +
> > +  snprintf (*sockpath, UNIX_PATH_MAX-1, "%s/%s", path,
filename);
> > +  (*sockpath)[UNIX_PATH_MAX-1] = '\0';
> 
> What's wrong with:
> 
>   snprintf (*sockpath, UNIX_PATH_MAX, "%s/%s", path, filename);
If the "$path/$filename" string is longer than UNIX_PATH_MAX, then
*sockpath won't be 0-terminated.  Since the line after that always
puts 0 at the end, we can just save one character.

The truncation of long paths always happens with this patch, and that
is what patch #3 addresses.

-- 
Pino Toscano

On Wed, Feb 03, 2016 at 10:35:07AM +0100, Pino Toscano
wrote:
> On Tuesday 02 February 2016 19:47:12 Richard W.M. Jones wrote:
> > On Tue, Feb 02, 2016 at 03:27:39PM +0100, Pino Toscano wrote:
> > > diff --git a/src/launch.c b/src/launch.c
> > > index f59818f..ec061e3 100644
> > > --- a/src/launch.c
> > > +++ b/src/launch.c
> > > @@ -418,6 +418,21 @@ guestfs_int_get_cpu_model (int kvm)
> > >  #endif
> > >  }
> > >  
> > > +/* Create the path for a socket with the selected filename in
the
> > > + * tmpdir.
> > > + */
> > > +int
> > > +guestfs_int_create_socketname (guestfs_h *g, const char
*filename,
> > > +                               char (*sockpath)[UNIX_PATH_MAX])
> > > +{
> > > +  char *path = g->tmpdir;
> > > +
> > > +  snprintf (*sockpath, UNIX_PATH_MAX-1, "%s/%s", path,
filename);
> > > +  (*sockpath)[UNIX_PATH_MAX-1] = '\0';
> > 
> > What's wrong with:
> > 
> >   snprintf (*sockpath, UNIX_PATH_MAX, "%s/%s", path,
filename);
> 
> If the "$path/$filename" string is longer than UNIX_PATH_MAX,
then
> *sockpath won't be 0-terminated.
That's not true though:

--------------------------------------------------------- test.c -----
#include <stdio.h>
#include <stdlib.h>

int
main ()
{
  char s[10];

  snprintf (s, sizeof s, "%s", "0123456789");
  printf ("s = %s\n", s);
  return 0;
}
----------------------------------------------------------------------

$ gcc -Wall test.c -o test
$ ./test
s = 012345678

Rich.

-- 
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
libguestfs lets you edit virtual machines.  Supports shell scripting,
bindings from many languages.  http://libguestfs.org

On Wed, Feb 03, 2016 at 10:34:06AM +0000, Richard W.M. Jones
wrote:
> On Wed, Feb 03, 2016 at 10:35:07AM +0100, Pino Toscano wrote:
> > On Tuesday 02 February 2016 19:47:12 Richard W.M. Jones wrote:
> > > On Tue, Feb 02, 2016 at 03:27:39PM +0100, Pino Toscano wrote:
> > > > diff --git a/src/launch.c b/src/launch.c
> > > > index f59818f..ec061e3 100644
> > > > --- a/src/launch.c
> > > > +++ b/src/launch.c
> > > > @@ -418,6 +418,21 @@ guestfs_int_get_cpu_model (int kvm)
> > > >  #endif
> > > >  }
> > > >  
> > > > +/* Create the path for a socket with the selected filename
in the
> > > > + * tmpdir.
> > > > + */
> > > > +int
> > > > +guestfs_int_create_socketname (guestfs_h *g, const char
*filename,
> > > > +                               char
(*sockpath)[UNIX_PATH_MAX])
> > > > +{
> > > > +  char *path = g->tmpdir;
> > > > +
> > > > +  snprintf (*sockpath, UNIX_PATH_MAX-1, "%s/%s",
path, filename);
> > > > +  (*sockpath)[UNIX_PATH_MAX-1] = '\0';
> > > 
> > > What's wrong with:
> > > 
> > >   snprintf (*sockpath, UNIX_PATH_MAX, "%s/%s", path,
filename);
> > 
> > If the "$path/$filename" string is longer than
UNIX_PATH_MAX, then
> > *sockpath won't be 0-terminated.
> 
> That's not true though:
Having said that, what this function _could_ usefully do is to give a
fatal error if the final path is longer than UNIX_PATH_MAX-1, since
truncating the path is unlikely to produce correct behaviour.

Rich.

-- 
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
virt-builder quickly builds VMs from scratch
http://libguestfs.org/virt-builder.1.html

On Wednesday 03 February 2016 10:34:06 Richard W.M. Jones
wrote:
> On Wed, Feb 03, 2016 at 10:35:07AM +0100, Pino Toscano wrote:
> > On Tuesday 02 February 2016 19:47:12 Richard W.M. Jones wrote:
> > > On Tue, Feb 02, 2016 at 03:27:39PM +0100, Pino Toscano wrote:
> > > > diff --git a/src/launch.c b/src/launch.c
> > > > index f59818f..ec061e3 100644
> > > > --- a/src/launch.c
> > > > +++ b/src/launch.c
> > > > @@ -418,6 +418,21 @@ guestfs_int_get_cpu_model (int kvm)
> > > >  #endif
> > > >  }
> > > >  
> > > > +/* Create the path for a socket with the selected filename
in the
> > > > + * tmpdir.
> > > > + */
> > > > +int
> > > > +guestfs_int_create_socketname (guestfs_h *g, const char
*filename,
> > > > +                               char
(*sockpath)[UNIX_PATH_MAX])
> > > > +{
> > > > +  char *path = g->tmpdir;
> > > > +
> > > > +  snprintf (*sockpath, UNIX_PATH_MAX-1, "%s/%s",
path, filename);
> > > > +  (*sockpath)[UNIX_PATH_MAX-1] = '\0';
> > > 
> > > What's wrong with:
> > > 
> > >   snprintf (*sockpath, UNIX_PATH_MAX, "%s/%s", path,
filename);
> > 
> > If the "$path/$filename" string is longer than
UNIX_PATH_MAX, then
> > *sockpath won't be 0-terminated.
> 
> That's not true though:
> 
> --------------------------------------------------------- test.c -----
> #include <stdio.h>
> #include <stdlib.h>
> 
> int
> main ()
> {
>   char s[10];
> 
>   snprintf (s, sizeof s, "%s", "0123456789");
>   printf ("s = %s\n", s);
>   return 0;
> }
> ----------------------------------------------------------------------
> 
> $ gcc -Wall test.c -o test
> $ ./test
> s = 012345678
Oh right, snprintf is indeed not strncpy... I stand corrected, thanks.

-- 
Pino Toscano