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 <mklet...@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.
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; }
signature.asc
Description: Digital signature
-- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
