[libvirt] PATCH: Fix misc memory alloc/free/usage bugs

[Daniel Veillard]

On Mon, Mar 30, 2009 at 11:41:20PM +0900, Ryota Ozaki wrote:
> Hi Daniel,
> 
> On Mon, Mar 30, 2009 at 10:39 PM, Daniel P. Berrange
> <berrange at redhat.com> wrote:
> > A few more small bugs
> >
> >  * src/node_device.c: Don't strdup() a NULL device parent
> >  * src/qemu_conf.c: Don't try to run access() on a NULL binary path
> >  * src/security_selinux.c Make sure result from readlink() is NULL
> >   terminated
> >  * src/storage_conf.c: DOn't free 'mode' while it is still used

  Patch looks fine, with the exception

[...]
> > +++ b/src/security_selinux.c    Mon Mar 30 14:37:45 2009 +0100
> > @@ -303,11 +303,13 @@ SELinuxRestoreSecurityImageLabel(virConn
> >         return -1;
> >
> >     if (S_ISLNK(buf.st_mode)) {
> > +        int n;
> >         if (VIR_ALLOC_N(newpath, buf.st_size + 1) < 0)
> >             return -1;
> >
> > -        if (readlink(path, newpath, buf.st_size) < 0)
> > +        if ((n =readlink(path, newpath, buf.st_size)) < 0)
> >             goto err;
> > +        buf.st_size[n] = '\0';
>            newpath[n] = '\0';
> 
> correct?

  Yup, I doubt it would compile otherwise :-)


  I'm still doubtful about this piece of code though.
Suppose you have path an absolute path to /tmp/bar, and
bar is a relative symlink to foo, you will get buf.st_size
which is 3 i.e. the length of "foo" which is insufficient
to hold the expected path /tmp/foo.
  Also /tmp/bar may point to /tmp/foo, which itself points to
/tmp/very_long_filename and again readlink will fail to provide
the full path.
  Seems to me that if you're to use readlink there is no other
way than to allocate a PATH_MAX (+1) buffer and use that for
the link resolution.

 So ACK to a patch but using PATH_MAX instead of buf.st_size.
Another option is to add
  char *virFileReadLink(const char *path);
to util.[ch], doing the encapsulation of this rather dangerous
function and modify security_selinux.c and storage_backend_disk.c
to use that instead.

Daniel

-- 
Daniel Veillard      | libxml Gnome XML XSLT toolkit  http://xmlsoft.org/
daniel at veillard.com  | Rpmfind RPM search engine http://rpmfind.net/
http://veillard.com/ | virtualization library  http://libvirt.org/