"Daniel P. Berrange" <berrange@xxxxxxxxxx> wrote: > On Wed, Jan 02, 2008 at 12:31:56PM +0000, Daniel P. Berrange wrote: >> If the application does not supply an authentication callback, and tries to >> connect to a server with PolicyKit auth turned on it will try to deference >> a NULL pointer with predictably crashtastic results: >> >> https://bugzilla.redhat.com/show_bug.cgi?id=427107 >> >> This patch has been tested by bug reporter to fix the issue > > Here is a second patch which fixes the same issue in the SASL client code > too Hi Dan, Looks correct. You can apply these to create an equivalent patch with the following changes: I saw that you removed one dead-code "return -1". This removes another: [PATCH] Remove dead-code "return -1" after goto. I found those two if/else blocks with all the gotos hard to read. This reorganizes and also avoids some duplication and shortens some longer-than-80-byte lines. [PATCH] Factor out two __virRaiseError,goto pairs. I know there are lots of other "if (var) free(var);" sequences in libvirt, but that "if (var)" part has been unnecessary for ages: [PATCH] Don't bother to check for non-NULL before calling free. ===================================================================== Date: Fri, 11 Jan 2008 18:53:57 +0100 Subject: [PATCH] Remove dead-code "return -1" after goto. diff --git a/src/remote_internal.c b/src/remote_internal.c index 88e9a72..88960ec 100644 --- a/src/remote_internal.c +++ b/src/remote_internal.c @@ -3249,7 +3249,6 @@ remoteAuthSASL (virConnectPtr conn, struct private_data *priv, int in_open, VIR_ERR_AUTH_FAILED, VIR_ERR_ERROR, NULL, NULL, NULL, 0, 0, "Failed to collect auth credentials"); goto cleanup; - return -1; } remoteAuthFillInteract(cred, interact); goto restep; ===================================================================== Date: Fri, 11 Jan 2008 19:03:31 +0100 Subject: [PATCH] Factor out two __virRaiseError,goto pairs. diff --git a/src/remote_internal.c b/src/remote_internal.c index 88960ec..bca3ad3 100644 --- a/src/remote_internal.c +++ b/src/remote_internal.c @@ -3152,6 +3152,7 @@ remoteAuthSASL (virConnectPtr conn, struct private_data *priv, int in_open, /* Need to gather some credentials from the client */ if (err == SASL_INTERACT) { + const char *msg; if (cred) { remoteAuthFreeCredentials(cred, ncred); cred = NULL; @@ -3166,20 +3167,18 @@ remoteAuthSASL (virConnectPtr conn, struct private_data *priv, int in_open, } /* Run the authentication callback */ if (auth && auth->cb) { - if ((*(auth->cb))(cred, ncred, auth->cbdata) < 0) { - __virRaiseError (in_open ? NULL : conn, NULL, NULL, VIR_FROM_REMOTE, - VIR_ERR_AUTH_FAILED, VIR_ERR_ERROR, NULL, NULL, NULL, 0, 0, - "Failed to collect auth credentials"); - goto cleanup; + if ((*(auth->cb))(cred, ncred, auth->cbdata) >= 0) { + remoteAuthFillInteract(cred, interact); + goto restart; } - remoteAuthFillInteract(cred, interact); - goto restart; + msg = "Failed to collect auth credentials"; } else { - __virRaiseError (in_open ? NULL : conn, NULL, NULL, VIR_FROM_REMOTE, - VIR_ERR_AUTH_FAILED, VIR_ERR_ERROR, NULL, NULL, NULL, 0, 0, - "No authentication callback available"); - goto cleanup; + msg = "No authentication callback available"; } + __virRaiseError (in_open ? NULL : conn, NULL, NULL, VIR_FROM_REMOTE, + VIR_ERR_AUTH_FAILED, VIR_ERR_ERROR, NULL, NULL, NULL, + 0, 0, msg); + goto cleanup; } free(iret.mechlist); @@ -3231,6 +3230,7 @@ remoteAuthSASL (virConnectPtr conn, struct private_data *priv, int in_open, } /* Need to gather some credentials from the client */ if (err == SASL_INTERACT) { + const char *msg; if (cred) { remoteAuthFreeCredentials(cred, ncred); cred = NULL; @@ -3244,20 +3244,18 @@ remoteAuthSASL (virConnectPtr conn, struct private_data *priv, int in_open, } /* Run the authentication callback */ if (auth && auth->cb) { - if ((*(auth->cb))(cred, ncred, auth->cbdata) < 0) { - __virRaiseError (in_open ? NULL : conn, NULL, NULL, VIR_FROM_REMOTE, - VIR_ERR_AUTH_FAILED, VIR_ERR_ERROR, NULL, NULL, NULL, 0, 0, - "Failed to collect auth credentials"); - goto cleanup; + if ((*(auth->cb))(cred, ncred, auth->cbdata) >= 0) { + remoteAuthFillInteract(cred, interact); + goto restep; } - remoteAuthFillInteract(cred, interact); - goto restep; + msg = "Failed to collect auth credentials"; } else { - __virRaiseError (in_open ? NULL : conn, NULL, NULL, VIR_FROM_REMOTE, - VIR_ERR_AUTH_FAILED, VIR_ERR_ERROR, NULL, NULL, NULL, 0, 0, - "No authentication callback available"); - goto cleanup; + msg = "No authentication callback available"; } + __virRaiseError (in_open ? NULL : conn, NULL, NULL, VIR_FROM_REMOTE, + VIR_ERR_AUTH_FAILED, VIR_ERR_ERROR, NULL, NULL, NULL, + 0, 0, msg); + goto cleanup; } if (serverin) { ===================================================================== Date: Fri, 11 Jan 2008 19:05:45 +0100 Subject: [PATCH] Don't bother to check for non-NULL before calling free. diff --git a/src/remote_internal.c b/src/remote_internal.c index bca3ad3..88978d3 100644 --- a/src/remote_internal.c +++ b/src/remote_internal.c @@ -3326,8 +3326,7 @@ remoteAuthSASL (virConnectPtr conn, struct private_data *priv, int in_open, if (remoteAddr) free(remoteAddr); if (serverin) free(serverin); - if (saslcb) - free(saslcb); + free(saslcb); remoteAuthFreeCredentials(cred, ncred); if (ret != 0 && saslconn) sasl_dispose(&saslconn); -- -- Libvir-list mailing list Libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list