Re: [Gluster-devel] [libvirt] [PATCH v2 1/2] Qemu/Gluster: Add Gluster protocol as supported network disk formats.

[Harsh Bora]

On 10/05/2012 03:13 AM, Daniel P. Berrange wrote:
> On Thu, Oct 04, 2012 at 09:17:08PM +0530, Deepak C Shetty wrote:
> > > +        virBufferAsprintf(opt, "gluster+%s://",
> > > +                          
> > > virDomainDiskProtocolTransportTypeToString(disk->hosts->transport));
> > > +
> > > +        /* if transport type is not unix, specify server:port */
> > > +        if (disk->hosts->transport != VIR_DOMAIN_DISK_PROTO_TRANS_UNIX) {
> > > +            if (strstr(disk->hosts->name, ":")) {
> > Wouldn't it be better to check for ':' and check for absence of "."
> > (and vice versa in the else) so that if someone specified a.b.c:d
> > or a:b:c:d:e.f we can throw error rite away, instead of qemu
> > erroring out later in time ? Its a very primtive check but
> > can help to catch user input error early enuf.
> >
> > > +                /* if IPv6 addr, use square brackets to enclose it */
> > > +                virBufferAsprintf(opt, "[%s]:%s", disk->hosts->name, 
> > > disk->hosts->port);
> > > +            } else {
> > > +                virBufferAsprintf(opt, "%s:%s", disk->hosts->name, 
> > > disk->hosts->port);
> > > +            }
> > > +        }
> > > +
> > > +        /* append source path to gluster disk image */
> > > +        virBufferAsprintf(opt, "/%s", disk->src);
> > > +
> > > +        /* if transport type is unix, server name is path to unix socket, 
> > > ignore port */
> > > +        if (disk->hosts->transport == VIR_DOMAIN_DISK_PROTO_TRANS_UNIX) {
> > > +            virBufferAsprintf(opt, "?socket=%s", disk->hosts->name);
> > > +        }
> > This can be clubbed as part of else clause to the above if condn (if
> > (disk->hosts->transport != VIR_DOMAIN_DISK_PROTO_TRANS_UNIX)), that
> > ways we avoid an un-necessary check of transport here.
> > It also means that disk->src needs to be pulled inside the if & else
> > clauses, which I feel should be ok.
>
> Since this code is building up a URI, it really should be re-written to
> use the virURIPtr object, instead of virBufferPtr. This will ensure that
> things like [] for IPv6 are handled automatically, as well as doing the
> correct escaping of parameter values. This code is quite possibly
> exploitable as it stands if the user provided cleverly crafted values
> for disks->hosts->name which abused URI syntax

Hm, I was thinking of moving to URI infra as a separate patch later, 
however looks like that is the way to go. I will look into that.

regards,
Harsh

> Daniel