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

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 Hi Daniel, I had a look at src/util/viruri.c and found that we have virURIParse() which takes a const char* and returns a virURIPtr object, likewise we have virURIFormat() which take a virURIPtr and returns a char*. Are you suggesting to manually populate a virURI struct and pass its address to virURIFormat() to build the cmdline from libvirt XML or are you suggesting to first build a cmdline (virBuffer) from the XML and pass it to virURIParse to validate if the libvirt XML has been provided a valid URI ?