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

[Martin Kletzander]

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 at 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;
>>  }
>>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: Digital signature
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20141110/fd7d6ed8/attachment-0001.sig>