On 05/14/2015 08:35 AM, John Ferlan wrote:
On 05/13/2015 02:43 PM, Jiri Denemark 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@redhat.com> --- 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
On Wed, May 13, 2015 at 12:32:20 -0400, John Ferlan wrote: 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...
In case there's any question - ACK to Jirka's version of the patch.