On 08/19/2010 04:14 PM, Matthias Bolte wrote:
>> if (guiPresent) {
>> if (guiDisplay) {
>> - sprintf(displayutf8, "DISPLAY=%.24s", guiDisplay);
>> - VBOX_UTF8_TO_UTF16(displayutf8, &env);
>> + char *displayutf8;
>> + if (virAsprintf(&displayutf8, "DISPLAY=%s", guiDisplay) < 0)
>> + virReportOOMError();
>
> Why don't we abort the try to start the guest when hitting OOM?
> Starting the guest will probably fail because of OOM anyway.
The existing code didn't worry about it, but you are probably right that
we might as well give up harder on the first OOM.
>
> Also why switch to virAsprintf when snprintf would work too?
Even this sprintf is currently safe (the %.24s fits into the length
allocated for guiDisplay), but fragile and arbitrarily limited. That
is, the s[n]printf solution locks you into a particular length, and has
to be revisited if someone ever has a reason why a 24-character display
string is too short (and I can totally see someone complaining that
their 25-byte FQDN is a reason to support a longer DISPLAY). At least
snprintf only has one place to touch (the array declaration length)
instead of two with sprintf (the array declaration and the %.24s in the
format string), but virAsprintf gets rid of the length limitation once
and for all.
But I'll be reposting the patch after refactoring the other three
instances in the file, so you can wait for a v2 of this patch.
--
Eric Blake eblake@xxxxxxxxxx +1-801-349-2682
Libvirt virtualization library http://libvirt.org
Attachment:
signature.asc
Description: OpenPGP digital signature
-- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list