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

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


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