[libvirt] Sys::Virt Perl bindings - calling newSVpv with unsafe length
Dan, After tracking down a very obscure bug in hivex & libguestfs Perl bindings, I started looking for places where we call newSVpv with a variable length argument that could be zero. In such cases, one should call newSVpvn instead, otherwise Perl will try to use strlen() to calculate the length of the data buffer when length is passed in as zero [I'm sure you are aware of this already]. There seems to be one such case in the Sys::Virt bindings: http://cpansearch.perl.org/src/DANBERR/Sys-Virt-0.2.3/Virt.xs CODE: if ((bytes = virSecretGetValue(sec, &len, flags)) == NULL) { _croak_error(virConnGetLastError(virSecretGetConnect(sec))); } RETVAL = newSVpv((char*)bytes, len); Maybe 'len' can never be zero here, but I think it's safer to turn this into a call to newSVpvn anyway. Also, if you look at the source to newSVpv: http://github.com/mirrors/perl/blob/blead/sv.c#L7752 You'll see there is an extra test, not present with newSVpvn. So it's better to always call newSVpvn wherever there is a constant-sized buffer, eg in code like this: if ((virDomainGetUUID(dom, rawuuid)) < 0) { _croak_error(virConnGetLastError(virDomainGetConnect(dom))); } RETVAL = newSVpv((char*)rawuuid, sizeof(rawuuid)); Rich. -- Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones New in Fedora 11: Fedora Windows cross-compiler. Compile Windows programs, test, and build Windows installers. Over 70 libraries supprt'd http://fedoraproject.org/wiki/MinGW http://www.annexia.org/fedora_mingw
On Sat, Apr 03, 2010 at 09:21:41AM +0100, Richard W.M. Jones wrote:
Dan,
After tracking down a very obscure bug in hivex & libguestfs Perl bindings, I started looking for places where we call newSVpv with a variable length argument that could be zero. In such cases, one should call newSVpvn instead, otherwise Perl will try to use strlen() to calculate the length of the data buffer when length is passed in as zero [I'm sure you are aware of this already].
Ewwww, totally forgot about that.
There seems to be one such case in the Sys::Virt bindings:
http://cpansearch.perl.org/src/DANBERR/Sys-Virt-0.2.3/Virt.xs
CODE: if ((bytes = virSecretGetValue(sec, &len, flags)) == NULL) { _croak_error(virConnGetLastError(virSecretGetConnect(sec))); } RETVAL = newSVpv((char*)bytes, len);
Maybe 'len' can never be zero here, but I think it's safer to turn this into a call to newSVpvn anyway.
Agreed, I think it can actually be zero.
Also, if you look at the source to newSVpv:
http://github.com/mirrors/perl/blob/blead/sv.c#L7752
You'll see there is an extra test, not present with newSVpvn. So it's better to always call newSVpvn wherever there is a constant-sized buffer, eg in code like this:
if ((virDomainGetUUID(dom, rawuuid)) < 0) { _croak_error(virConnGetLastError(virDomainGetConnect(dom))); } RETVAL = newSVpv((char*)rawuuid, sizeof(rawuuid));
Yep, will update this all. Daniel -- |: Red Hat, Engineering, London -o- http://people.redhat.com/berrange/ :| |: http://libvirt.org -o- http://virt-manager.org -o- http://deltacloud.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: GnuPG: 7D3B9505 -o- F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 :|
participants (2)
-
Daniel P. Berrange -
Richard W.M. Jones