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&gt; > --- > 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; > } >