Re: [libvirt] [PATCH] phyp: Fix NULL dereference in phypConnectOpen
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(a)redhat.com>
---
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.
John
- VIR_FORCE_CLOSE(phyp_driver->sock);
-
return VIR_DRV_OPEN_ERROR;
}