[libvirt-php][PATCH] libvirt_domain_get_screenshot_api: More memleak fixes and checks

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



Firstly, we are not checking whether virStreamNew succeeds or
not. Secondly, we are not freeing the stream in all exit paths.
Then we are copying the same pattern which frees allocated memory
over and over again. Oh, and also we should abort stream if we
are exiting abnormally.

Signed-off-by: Michal Privoznik <mprivozn@xxxxxxxxxx>
---

Pushed under "I'm a lonely libvirt-php maintainer" rule.

 src/libvirt-php.c | 64 +++++++++++++++++++++++++++++--------------------------
 1 file changed, 34 insertions(+), 30 deletions(-)

diff --git a/src/libvirt-php.c b/src/libvirt-php.c
index 3aed882..df2f32a 100644
--- a/src/libvirt-php.c
+++ b/src/libvirt-php.c
@@ -4758,49 +4758,38 @@ PHP_FUNCTION(libvirt_domain_get_screenshot_api)
     RETURN_FALSE;
 #endif
 
-    GET_DOMAIN_FROM_ARGS("r|l",&zdomain, &screen);
+    GET_DOMAIN_FROM_ARGS("r|l", &zdomain, &screen);
+
+    if (!(st = virStreamNew(domain->conn->conn, 0))) {
+        set_error("Cannot create new stream" TSRMLS_CC);
+        goto error;
+    }
 
-    st = virStreamNew(domain->conn->conn, 0);
     mime = virDomainScreenshot(domain->domain, st, screen, 0);
     if (!mime) {
         set_error_if_unset("Cannot get domain screenshot" TSRMLS_CC);
-        RETURN_FALSE;
+        goto error;
     }
 
-#ifndef EXTWIN
-    if (mkstemp(file) == 0) {
-        free(mime);
-        RETURN_FALSE;
-    }
-#endif
-
-    if ((fd = open(file, O_WRONLY|O_CREAT|O_EXCL, 0666)) < 0) {
-        if (errno != EEXIST ||
-            (fd = open(file, O_WRONLY|O_TRUNC, 0666)) < 0) {
-            virStreamFree(st);
-            set_error_if_unset("Cannot get create file to save domain screenshot" TSRMLS_CC);
-            free(mime);
-            RETURN_FALSE;
-        }
+    if (!(fd = mkstemp(file))) {
+        virStreamAbort(st);
+        set_error_if_unset("Cannot get create file to save domain screenshot" TSRMLS_CC);
+        goto error;
     }
 
     if (virStreamRecvAll(st, streamSink, &fd) < 0) {
-        virStreamFree(st);
         set_error_if_unset("Cannot receive screenshot data" TSRMLS_CC);
-        free(mime);
-        RETURN_FALSE;
+        virStreamAbort(st);
+        goto error;
     }
 
-    close(fd);
-
     if (virStreamFinish(st) < 0) {
-        virStreamFree(st);
         set_error_if_unset("Cannot close stream for domain" TSRMLS_CC);
-        free(mime);
-        RETURN_FALSE;
+        goto error;
     }
 
     virStreamFree(st);
+    st = NULL;
 
     array_init(return_value);
     if (bin) {
@@ -4811,19 +4800,34 @@ PHP_FUNCTION(libvirt_domain_get_screenshot_api)
         snprintf(fileNew, sizeof(fileNew), "%s.png", file);
         snprintf(tmp, sizeof(tmp), "%s %s %s > /dev/null 2> /dev/null", bin, file, fileNew);
         exitStatus = system(tmp);
-        unlink(file);
         if (WEXITSTATUS(exitStatus) != 0)
-            RETURN_FALSE;
+            goto error;
 
+        unlink(file);
+        close(fd);
+        fd = -1;
         VIRT_ADD_ASSOC_STRING(return_value, "file", fileNew);
         VIRT_ADD_ASSOC_STRING(return_value, "mime", "image/png");
-    }
-    else {
+    } else {
+        unlink(file);
+        close(fd);
+        fd = -1;
         VIRT_ADD_ASSOC_STRING(return_value, "file", file);
         VIRT_ADD_ASSOC_STRING(return_value, "mime", mime);
     }
 
     free(mime);
+    return;
+
+ error:
+    free(mime);
+    if (fd != -1) {
+        unlink(file);
+        close(fd);
+    }
+    if (st)
+        virStreamFree(st);
+    RETURN_FALSE;
 }
 
 /*
-- 
2.8.4

--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list



[Index of Archives]     [Virt Tools]     [Libvirt Users]     [Lib OS Info]     [Fedora Users]     [Fedora Desktop]     [Fedora SELinux]     [Big List of Linux Books]     [Yosemite News]     [KDE Users]     [Fedora Tools]