Re: [libvirt] [PATCH 1/3] storage: Resolve Coverity FORWARD_NULL

On 05/13/2015 02:43 PM, Jiri Denemark wrote: > On Wed, May 13, 2015 at 12:32:20 -0400, John Ferlan wrote: >> Coverity points out it's possible for one of the virCommand{Output|Error}* >> API's to have not allocated 'output' and/or 'error' in which case the >> strstr comparison will cause a NULL deref >> >> Signed-off-by: John Ferlan <jferlan(a)redhat.com&gt; >> --- >> src/storage/storage_backend_disk.c | 2 +- >> 1 file changed, 1 insertion(+), 1 deletion(-) >> >> diff --git a/src/storage/storage_backend_disk.c b/src/storage/storage_backend_disk.c >> index 6394dac..5c2c49a 100644 >> --- a/src/storage/storage_backend_disk.c >> +++ b/src/storage/storage_backend_disk.c >> @@ -412,7 +412,7 @@ virStorageBackendDiskFindLabel(const char* device) >> >> /* if parted succeeds we have a valid partition table */ >> ret = virCommandRun(cmd, NULL); >> - if (ret < 0) { >> + if (ret < 0 && output && error) { >> if (strstr(output, "unrecognised disk label") || >> strstr(error, "unrecognised disk label")) { >> ret = 1; > This doesn't seem to be correct if either output or error is NULL and > the other one is non-NULL. I'm too lazy to check if it's possible or > not, but I think we should change this code in the following way and be > safe: > > if (ret < 0) { > if ((output && strstr(output, "unrecognised disk label")) || > (error && strstr(error, "unrecognised disk label"))) { > ret = 1; > > Jirka > Sure - seems reasonable, although I suspect if allocation of memory for the output buffer fails, then allocation of memory for the error buffer will fail too, but just in case it succeeds...