Problem: leaks on send_zap_request fail, use of goto idiom is fragile.

src/curve_server.cpp

@@ -580,7 +580,7 @@ int zmq::curve_server_t::send_zap_request (const uint8_t *key)
     msg.set_flags (msg_t::more);
     rc = session->write_zap_msg (&msg);
     if (rc != 0)
-        return -1;
+        return send_failure (&msg);
 
     //  Version frame
     rc = msg.init_size (3);
@@ -589,7 +589,7 @@ int zmq::curve_server_t::send_zap_request (const uint8_t *key)
     msg.set_flags (msg_t::more);
     rc = session->write_zap_msg (&msg);
     if (rc != 0)
-        return -1;
+        return send_failure (&msg);
 
     //  Request ID frame
     rc = msg.init_size (1);
@@ -598,7 +598,7 @@ int zmq::curve_server_t::send_zap_request (const uint8_t *key)
     msg.set_flags (msg_t::more);
     rc = session->write_zap_msg (&msg);
     if (rc != 0)
-        return -1;
+        return send_failure (&msg);
 
     //  Domain frame
     rc = msg.init_size (options.zap_domain.length ());
@@ -607,7 +607,7 @@ int zmq::curve_server_t::send_zap_request (const uint8_t *key)
     msg.set_flags (msg_t::more);
     rc = session->write_zap_msg (&msg);
     if (rc != 0)
-        return -1;
+        return send_failure (&msg);
 
     //  Address frame
     rc = msg.init_size (peer_address.length ());
@@ -616,7 +616,7 @@ int zmq::curve_server_t::send_zap_request (const uint8_t *key)
     msg.set_flags (msg_t::more);
     rc = session->write_zap_msg (&msg);
     if (rc != 0)
-        return -1;
+        return send_failure (&msg);
 
     //  Identity frame
     rc = msg.init_size (options.identity_size);
@@ -625,7 +625,7 @@ int zmq::curve_server_t::send_zap_request (const uint8_t *key)
     msg.set_flags (msg_t::more);
     rc = session->write_zap_msg (&msg);
     if (rc != 0)
-        return -1;
+        return send_failure (&msg);
 
     //  Mechanism frame
     rc = msg.init_size (5);
@@ -634,7 +634,7 @@ int zmq::curve_server_t::send_zap_request (const uint8_t *key)
     msg.set_flags (msg_t::more);
     rc = session->write_zap_msg (&msg);
     if (rc != 0)
-        return -1;
+        return send_failure (&msg);
 
     //  Credentials frame
     rc = msg.init_size (crypto_box_PUBLICKEYBYTES);
@@ -642,7 +642,7 @@ int zmq::curve_server_t::send_zap_request (const uint8_t *key)
     memcpy (msg.data (), key, crypto_box_PUBLICKEYBYTES);
     rc = session->write_zap_msg (&msg);
     if (rc != 0)
-        return -1;
+        return send_failure (&msg);
 
     return 0;
 }
@@ -661,26 +661,21 @@ int zmq::curve_server_t::receive_and_process_zap_reply ()
     for (int i = 0; i < 7; i++) {
         rc = session->read_zap_msg (&msg [i]);
         if (rc == -1)
-            break;
+            return send_failure (msg);
         if ((msg [i].flags () & msg_t::more) == (i < 6? 0: msg_t::more)) {
             //  Temporary support for security debugging
             puts ("CURVE I: ZAP handler sent incomplete reply message");
             errno = EPROTO;
-            rc = -1;
-            break;
+            return send_failure (msg);
         }
     }
 
-    if (rc != 0)
-        goto error;
-
     //  Address delimiter frame
     if (msg [0].size () > 0) {
         //  Temporary support for security debugging
         puts ("CURVE I: ZAP handler sent malformed reply message");
         errno = EPROTO;
-        rc = -1;
-        goto error;
+        return send_failure (msg);
     }
 
     //  Version frame
@@ -688,8 +683,7 @@ int zmq::curve_server_t::receive_and_process_zap_reply ()
         //  Temporary support for security debugging
         puts ("CURVE I: ZAP handler sent bad version number");
         errno = EPROTO;
-        rc = -1;
-        goto error;
+        return send_failure (msg);
     }
 
     //  Request id frame
@@ -697,8 +691,7 @@ int zmq::curve_server_t::receive_and_process_zap_reply ()
         //  Temporary support for security debugging
         puts ("CURVE I: ZAP handler sent bad request ID");
         errno = EPROTO;
-        rc = -1;
-        goto error;
+        return send_failure (msg);
     }
 
     //  Status code frame
@@ -706,8 +699,7 @@ int zmq::curve_server_t::receive_and_process_zap_reply ()
         //  Temporary support for security debugging
         puts ("CURVE I: ZAP handler rejected client authentication");
         errno = EACCES;
-        rc = -1;
-        goto error;
+        return send_failure (msg);
     }
 
     //  Save status code
@@ -720,13 +712,16 @@ int zmq::curve_server_t::receive_and_process_zap_reply ()
     rc = parse_metadata (static_cast <const unsigned char*> (msg [6].data ()),
                          msg [6].size (), true);
 
-error:
+    if (rc != 0)
+        return send_failure (msg);
+
+    //  Close all reply frames
     for (int i = 0; i < 7; i++) {
         const int rc2 = msg [i].close ();
         errno_assert (rc2 == 0);
     }
 
-    return rc;
+    return 0;
 }
 
 #endif

src/gssapi_server.cpp

@@ -161,7 +161,7 @@ int zmq::gssapi_server_t::send_zap_request ()
     msg.set_flags (msg_t::more);
     rc = session->write_zap_msg (&msg);
     if (rc != 0)
-        return -1;
+        return send_failure (&msg);
 
     //  Version frame
     rc = msg.init_size (3);
@@ -170,7 +170,7 @@ int zmq::gssapi_server_t::send_zap_request ()
     msg.set_flags (msg_t::more);
     rc = session->write_zap_msg (&msg);
     if (rc != 0)
-        return -1;
+        return send_failure (&msg);
 
     //  Request ID frame
     rc = msg.init_size (1);
@@ -179,7 +179,7 @@ int zmq::gssapi_server_t::send_zap_request ()
     msg.set_flags (msg_t::more);
     rc = session->write_zap_msg (&msg);
     if (rc != 0)
-        return -1;
+        return send_failure (&msg);
 
     //  Domain frame
     rc = msg.init_size (options.zap_domain.length ());
@@ -188,7 +188,7 @@ int zmq::gssapi_server_t::send_zap_request ()
     msg.set_flags (msg_t::more);
     rc = session->write_zap_msg (&msg);
     if (rc != 0)
-        return -1;
+        return send_failure (&msg);
 
     //  Address frame
     rc = msg.init_size (peer_address.length ());
@@ -197,7 +197,7 @@ int zmq::gssapi_server_t::send_zap_request ()
     msg.set_flags (msg_t::more);
     rc = session->write_zap_msg (&msg);
     if (rc != 0)
-        return -1;
+        return send_failure (&msg);
 
     //  Identity frame
     rc = msg.init_size (options.identity_size);
@@ -206,7 +206,7 @@ int zmq::gssapi_server_t::send_zap_request ()
     msg.set_flags (msg_t::more);
     rc = session->write_zap_msg (&msg);
     if (rc != 0)
-        return -1;
+        return send_failure (&msg);
 
     //  Mechanism frame
     rc = msg.init_size (6);
@@ -215,7 +215,7 @@ int zmq::gssapi_server_t::send_zap_request ()
     msg.set_flags (msg_t::more);
     rc = session->write_zap_msg (&msg);
     if (rc != 0)
-        return -1;
+        return send_failure (&msg);
 
     //  Principal frame
     gss_buffer_desc principal;
@@ -227,7 +227,7 @@ int zmq::gssapi_server_t::send_zap_request ()
     rc = session->write_zap_msg (&msg);
     gss_release_buffer(&min_stat, &principal);
     if (rc != 0)
-        return -1;
+        return send_failure (&msg);
 
     return 0;
 }
@@ -246,43 +246,35 @@ int zmq::gssapi_server_t::receive_and_process_zap_reply ()
     for (int i = 0; i < 7; i++) {
         rc = session->read_zap_msg (&msg [i]);
         if (rc == -1)
-            break;
+            return send_failure (msg);
         if ((msg [i].flags () & msg_t::more) == (i < 6? 0: msg_t::more)) {
             errno = EPROTO;
-            rc = -1;
-            break;
+            return send_failure (msg);
         }
     }
 
-    if (rc != 0)
-        goto error;
-
     //  Address delimiter frame
     if (msg [0].size () > 0) {
-        rc = -1;
         errno = EPROTO;
-        goto error;
+        return send_failure (msg);
     }
 
     //  Version frame
     if (msg [1].size () != 3 || memcmp (msg [1].data (), "1.0", 3)) {
-        rc = -1;
         errno = EPROTO;
-        goto error;
+        return send_failure (msg);
     }
 
     //  Request id frame
     if (msg [2].size () != 1 || memcmp (msg [2].data (), "1", 1)) {
-        rc = -1;
         errno = EPROTO;
-        goto error;
+        return send_failure (msg);
     }
 
     //  Status code frame
     if (msg [3].size () != 3 || memcmp (msg [3].data (), "200", 3)) {
-        rc = -1;
         errno = EACCES;
-        goto error;
+        return send_failure (msg);
     }
 
     //  Save user id
@@ -292,13 +284,16 @@ int zmq::gssapi_server_t::receive_and_process_zap_reply ()
     rc = parse_metadata (static_cast <const unsigned char*> (msg [6].data ()),
                          msg [6].size (), true);
 
-error:
+    if (rc != 0)
+        return send_failure (msg);
+
+    //  Close all reply frames
     for (int i = 0; i < 7; i++) {
         const int rc2 = msg [i].close ();
         errno_assert (rc2 == 0);
     }
 
-    return rc;
+    return 0;
 }
 
 

src/msg.hpp

@@ -34,6 +34,7 @@
 #include <stdio.h>
 
 #include "config.hpp"
+#include "err.hpp"
 #include "fd.hpp"
 #include "atomic_counter.hpp"
 #include "metadata.hpp"
@@ -246,6 +247,20 @@ namespace zmq
         } u;
     };
 
+    inline int send_failure (zmq::msg_t *msg)
+    {
+        const int rc = msg->close ();
+        errno_assert (rc == 0);
+        return -1;
+    }
+
+    inline int send_failure (zmq::msg_t msg[], int count)
+    {
+        for (int i = 0; i < count; i++)
+            send_failure (&msg [i]);
+
+        return -1;
+    }
 }
 
 #endif

src/null_mechanism.cpp

@@ -225,7 +225,7 @@ int zmq::null_mechanism_t::send_zap_request ()
     msg.set_flags (msg_t::more);
     rc = session->write_zap_msg (&msg);
     if (rc != 0)
-        return -1;
+        return send_failure (&msg);
 
     //  Version frame
     rc = msg.init_size (3);
@@ -234,7 +234,7 @@ int zmq::null_mechanism_t::send_zap_request ()
     msg.set_flags (msg_t::more);
     rc = session->write_zap_msg (&msg);
     if (rc != 0)
-        return -1;
+        return send_failure (&msg);
 
     //  Request id frame
     rc = msg.init_size (1);
@@ -243,7 +243,7 @@ int zmq::null_mechanism_t::send_zap_request ()
     msg.set_flags (msg_t::more);
     rc = session->write_zap_msg (&msg);
     if (rc != 0)
-        return -1;
+        return send_failure (&msg);
 
     //  Domain frame
     rc = msg.init_size (options.zap_domain.length ());
@@ -252,7 +252,7 @@ int zmq::null_mechanism_t::send_zap_request ()
     msg.set_flags (msg_t::more);
     rc = session->write_zap_msg (&msg);
     if (rc != 0)
-        return -1;
+        return send_failure (&msg);
 
     //  Address frame
     rc = msg.init_size (peer_address.length ());
@@ -261,7 +261,7 @@ int zmq::null_mechanism_t::send_zap_request ()
     msg.set_flags (msg_t::more);
     rc = session->write_zap_msg (&msg);
     if (rc != 0)
-        return -1;
+        return send_failure (&msg);
 
     //  Identity frame
     rc = msg.init_size (options.identity_size);
@@ -270,7 +270,7 @@ int zmq::null_mechanism_t::send_zap_request ()
     msg.set_flags (msg_t::more);
     rc = session->write_zap_msg (&msg);
     if (rc != 0)
-        return -1;
+        return send_failure (&msg);
 
     //  Mechanism frame
     rc = msg.init_size (4);
@@ -278,7 +278,7 @@ int zmq::null_mechanism_t::send_zap_request ()
     memcpy (msg.data (), "NULL", 4);
     rc = session->write_zap_msg (&msg);
     if (rc != 0)
-        return -1;
+        return send_failure (&msg);
 
     return 0;
 }
@@ -297,26 +297,21 @@ int zmq::null_mechanism_t::receive_and_process_zap_reply ()
     for (int i = 0; i < 7; i++) {
         rc = session->read_zap_msg (&msg [i]);
         if (rc == -1)
-            break;
+            return send_failure (msg);
         if ((msg [i].flags () & msg_t::more) == (i < 6? 0: msg_t::more)) {
             //  Temporary support for security debugging
             puts ("NULL I: ZAP handler sent incomplete reply message");
             errno = EPROTO;
-            rc = -1;
-            break;
+            return send_failure (msg);
         }
     }
 
-    if (rc != 0)
-        goto error;
-
     //  Address delimiter frame
     if (msg [0].size () > 0) {
         //  Temporary support for security debugging
         puts ("NULL I: ZAP handler sent malformed reply message");
         errno = EPROTO;
-        rc = -1;
-        goto error;
+        return send_failure (msg);
     }
 
     //  Version frame
@@ -324,8 +319,7 @@ int zmq::null_mechanism_t::receive_and_process_zap_reply ()
         //  Temporary support for security debugging
         puts ("NULL I: ZAP handler sent bad version number");
         errno = EPROTO;
-        rc = -1;
-        goto error;
+        return send_failure (msg);
     }
 
     //  Request id frame
@@ -333,8 +327,7 @@ int zmq::null_mechanism_t::receive_and_process_zap_reply ()
         //  Temporary support for security debugging
         puts ("NULL I: ZAP handler sent bad request ID");
         errno = EPROTO;
-        rc = -1;
-        goto error;
+        return send_failure (msg);
     }
 
     //  Status code frame
@@ -342,8 +335,7 @@ int zmq::null_mechanism_t::receive_and_process_zap_reply ()
         //  Temporary support for security debugging
         puts ("NULL I: ZAP handler rejected client authentication");
         errno = EPROTO;
-        rc = -1;
-        goto error;
+        return send_failure (msg);
     }
 
     //  Save status code
@@ -356,11 +348,14 @@ int zmq::null_mechanism_t::receive_and_process_zap_reply ()
     rc = parse_metadata (static_cast <const unsigned char*> (msg [6].data ()),
                          msg [6].size (), true);
 
-error:
+    if (rc != 0)
+        return send_failure (msg);
+
+    //  Close all reply frames
     for (int i = 0; i < 7; i++) {
         const int rc2 = msg [i].close ();
         errno_assert (rc2 == 0);
     }
 
-    return rc;
+    return 0;
 }

src/plain_server.cpp

@@ -289,7 +289,7 @@ int zmq::plain_server_t::send_zap_request (const std::string &username,
     msg.set_flags (msg_t::more);
     rc = session->write_zap_msg (&msg);
     if (rc != 0)
-        return -1;
+        return send_failure (&msg);
 
     //  Version frame
     rc = msg.init_size (3);
@@ -298,7 +298,7 @@ int zmq::plain_server_t::send_zap_request (const std::string &username,
     msg.set_flags (msg_t::more);
     rc = session->write_zap_msg (&msg);
     if (rc != 0)
-        return -1;
+        return send_failure (&msg);
 
     //  Request id frame
     rc = msg.init_size (1);
@@ -307,7 +307,7 @@ int zmq::plain_server_t::send_zap_request (const std::string &username,
     msg.set_flags (msg_t::more);
     rc = session->write_zap_msg (&msg);
     if (rc != 0)
-        return -1;
+        return send_failure (&msg);
 
     //  Domain frame
     rc = msg.init_size (options.zap_domain.length ());
@@ -316,7 +316,7 @@ int zmq::plain_server_t::send_zap_request (const std::string &username,
     msg.set_flags (msg_t::more);
     rc = session->write_zap_msg (&msg);
     if (rc != 0)
-        return -1;
+        return send_failure (&msg);
 
     //  Address frame
     rc = msg.init_size (peer_address.length ());
@@ -325,7 +325,7 @@ int zmq::plain_server_t::send_zap_request (const std::string &username,
     msg.set_flags (msg_t::more);
     rc = session->write_zap_msg (&msg);
     if (rc != 0)
-        return -1;
+        return send_failure (&msg);
 
     //  Identity frame
     rc = msg.init_size (options.identity_size);
@@ -334,7 +334,7 @@ int zmq::plain_server_t::send_zap_request (const std::string &username,
     msg.set_flags (msg_t::more);
     rc = session->write_zap_msg (&msg);
     if (rc != 0)
-        return -1;
+        return send_failure (&msg);
 
     //  Mechanism frame
     rc = msg.init_size (5);
@@ -343,7 +343,7 @@ int zmq::plain_server_t::send_zap_request (const std::string &username,
     msg.set_flags (msg_t::more);
     rc = session->write_zap_msg (&msg);
     if (rc != 0)
-        return -1;
+        return send_failure (&msg);
 
     //  Username frame
     rc = msg.init_size (username.length ());
@@ -352,7 +352,7 @@ int zmq::plain_server_t::send_zap_request (const std::string &username,
     msg.set_flags (msg_t::more);
     rc = session->write_zap_msg (&msg);
     if (rc != 0)
-        return -1;
+        return send_failure (&msg);
 
     //  Password frame
     rc = msg.init_size (password.length ());
@@ -360,7 +360,7 @@ int zmq::plain_server_t::send_zap_request (const std::string &username,
     memcpy (msg.data (), password.c_str (), password.length ());
     rc = session->write_zap_msg (&msg);
     if (rc != 0)
-        return -1;
+        return send_failure (&msg);
 
     return 0;
 }
@@ -379,26 +379,21 @@ int zmq::plain_server_t::receive_and_process_zap_reply ()
     for (int i = 0; i < 7; i++) {
         rc = session->read_zap_msg (&msg [i]);
         if (rc == -1)
-            break;
+            return send_failure (msg);
         if ((msg [i].flags () & msg_t::more) == (i < 6? 0: msg_t::more)) {
             //  Temporary support for security debugging
             puts ("PLAIN I: ZAP handler sent incomplete reply message");
             errno = EPROTO;
-            rc = -1;
-            break;
+            return send_failure (msg);
         }
     }
 
-    if (rc != 0)
-        goto error;
-
     //  Address delimiter frame
     if (msg [0].size () > 0) {
         //  Temporary support for security debugging
         puts ("PLAIN I: ZAP handler sent malformed reply message");
         errno = EPROTO;
-        rc = -1;
-        goto error;
+        return send_failure (msg);
     }
 
     //  Version frame
@@ -406,17 +401,15 @@ int zmq::plain_server_t::receive_and_process_zap_reply ()
         //  Temporary support for security debugging
         puts ("PLAIN I: ZAP handler sent bad version number");
         errno = EPROTO;
-        rc = -1;
-        goto error;
+        return send_failure (msg);
     }
 
     //  Request id frame
     if (msg [2].size () != 1 || memcmp (msg [2].data (), "1", 1)) {
         //  Temporary support for security debugging
         puts ("PLAIN I: ZAP handler sent bad request ID");
-        rc = -1;
         errno = EPROTO;
-        goto error;
+        return send_failure (msg);
     }
 
     //  Status code frame
@@ -424,8 +417,7 @@ int zmq::plain_server_t::receive_and_process_zap_reply ()
         //  Temporary support for security debugging
         puts ("PLAIN I: ZAP handler rejected client authentication");
         errno = EACCES;
-        rc = -1;
-        goto error;
+        return send_failure (msg);
     }
 
     //  Save status code
@@ -438,11 +430,14 @@ int zmq::plain_server_t::receive_and_process_zap_reply ()
     rc = parse_metadata (static_cast <const unsigned char*> (msg [6].data ()),
                          msg [6].size (), true);
 
-error:
+    if (rc != 0)
+        return send_failure (msg);
+
+    //  Close all reply frames
     for (int i = 0; i < 7; i++) {
         const int rc2 = msg [i].close ();
         errno_assert (rc2 == 0);
     }
 
-    return rc;
+    return 0;
 }