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

Re: [Libvir] Re: PATCH: BZ 427107: fix crash wrt auth callback



"Daniel P. Berrange" <berrange 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);
--


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