Re: [libvirt] [PATCH 10/10] vbox: avoid sprintf

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.