On 12/04/2010 04:21 AM, Matthias Bolte wrote:
2010/12/3 Eric Blake <eblake@redhat.com>:
popen must be matched with pclose (not fclose), or it will leak resources. Furthermore, it is a lousy interface when it comes to signal handling. We're much better off using our decent command wrapper.
+ line = *outbuf ? outbuf : NULL;
Is outbuf guaranteed to be non-NULL when virCommandRun succeeds? Otherwise we have a potential NULL dereference here.
Yep, I had already noticed that, which is why I posted two versions of a fix to virCommand to cover alternative approaches to this problem (only the v2 fix would guarantee non-NULL here).
+ if (virCommandRun(cmd, NULL) < 0) { + virCommandFree(cmd);
Is outbuf guaranteed to be unallocated when virCommandRun fails? Otherwise a VIR_FREE(outbuf) is missing here.
Not yet, but I think it should be (in other words, I need a v3 of my virCommand fix that guarantees: on success, if virCommandSet{Output,Error}Buffer was called, then that buffer will be allocated to a non-NULL string, and caller is responsible for freeing it on failure, then those buffers are guaranteed to be NULL (if the caller uses VIR_FREE, it won't hurt, but they do not have to explicitly worry about partial data being collected on failure)
+ virCommandFree(cmd); + ok = sscanf(outbuf, "%d\n", &veid) == 1;
Same question here about outbuf being guaranteed to be non-NULL when virCommandRun succeeds as in openvzLoadDomains. If not then sscanf is called with NULL and that'll probably segfault.
That sscanf is overkill anyways; I can probably convert both *scanf touched by this patch into lower-level functions, and in the process, avoid problems such as ignoring overflow that are inherent in scanf. -- Eric Blake eblake@redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org