12:29 p.m.
"Daniel P. Berrange" <berrange(a)redhat.com> 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);
--