Re: [PATCH] phyp: Fix NULL dereference in phypConnectOpen

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

 



On Sun, Nov 09, 2014 at 10:10:47AM -0500, John Ferlan wrote:


On 11/07/2014 01:56 PM, Martin Kletzander wrote:
Coverity found out that commit cd490086 caused a possible NULL pointer
dereference.  This is due to the fact, that phyp_driver might be
NULL (if VIR_ALLOC() fails), but connection_data, which kept the socket
before the mentioned commit, could not be NULL.


It would also be NULL after the "VIR_FREE(phyp_driver);" in the failure
path!

Signed-off-by: Martin Kletzander <mkletzan@xxxxxxxxxx>
---
 src/phyp/phyp_driver.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/src/phyp/phyp_driver.c b/src/phyp/phyp_driver.c
index 7c8bc5c..0d3ad53 100644
--- a/src/phyp/phyp_driver.c
+++ b/src/phyp/phyp_driver.c
@@ -1222,6 +1222,7 @@ phypConnectOpen(virConnectPtr conn,
     if (phyp_driver != NULL) {
         virObjectUnref(phyp_driver->caps);
         virObjectUnref(phyp_driver->xmlopt);
+        VIR_FORCE_CLOSE(phyp_driver->sock);
         VIR_FREE(phyp_driver);
     }

@@ -1232,8 +1233,6 @@ phypConnectOpen(virConnectPtr conn,
         libssh2_session_free(session);
     }

Because of the:

1230 	    if (session != NULL) {
1231 	        libssh2_session_disconnect(session, "Disconnecting...");
1232 	        libssh2_session_free(session);
1233 	    }


here where session is :

1181 	    if ((session = openSSHSession(conn, auth, &internal_socket))
== NULL) {
1182 	        virReportError(VIR_ERR_INTERNAL_ERROR,
1183 	                       "%s", _("Error while opening SSH session."));
1184 	        goto failure;
1185 	    }
1186
1187 	    phyp_driver->session = session;
1188 	    phyp_driver->sock = internal_socket;


Shouldn't the session be disconnected and freed before the socket is
closed? Or essentially in the reverse order of how things were allocated.

"Theoretically", the disconnect and free calls could use
"phyp_driver->session" too.  I assume "session" was used only because
the author knew phyp_driver->session was already free'd...  I'd think
referencing the ssh session after the socket was closed probably could
have strange/inconsistent results.


You're right, I haven't seen that the libssh session is using the
socket that we're closing.  v2 coming up.

Martin

John

-    VIR_FORCE_CLOSE(phyp_driver->sock);
-
     return VIR_DRV_OPEN_ERROR;
 }

Attachment: signature.asc
Description: Digital signature

--
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]