From 42b3c2ed11c1e62c1691f868a6796983f93c3beb Mon Sep 17 00:00:00 2001

From: Greg Hudson <ghudson@mit.edu>

Date: Wed, 9 Apr 2014 13:19:03 -0400

Subject: [PATCH 01/13] Simplify sendto_kdc.c

* Get rid of the "x" member of conn_state, which used to be a union

  but hasn't been since r14742.

* Define a structure type for the "out" member of conn_state.

* Rename incoming_krb5_message to incoming_message for brevity.

* Make the "pos" member of incoming_message an offset instead of a

  pointer, simplifying several present and future computations.

* Use "in" and "out" aliases to the conn_state in and out members

  where it improves brevity.

* Rename set_conn_state_msg_length to set_transport_message and give

  it a descriptive comment.

* Call set_transport_message from start_connection only, instead of

  once in add_connection and perhaps again in start_connection.  To

  make this possible, pass the original message argument to maybe_send

  and start_connection.

* Use make_data and empty_data helpers where appropriate.

---

 src/lib/krb5/os/sendto_kdc.c | 159 +++++++++++++++++++++----------------------

 1 file changed, 79 insertions(+), 80 deletions(-)

diff --git a/src/lib/krb5/os/sendto_kdc.c b/src/lib/krb5/os/sendto_kdc.c

index 67e2a60..5f781d3 100644

--- a/src/lib/krb5/os/sendto_kdc.c

+++ b/src/lib/krb5/os/sendto_kdc.c

@@ -76,30 +76,30 @@ static const char *const state_strings[] = {

 /* connection states */

 enum conn_states { INITIALIZING, CONNECTING, WRITING, READING, FAILED };

-struct incoming_krb5_message {

+struct incoming_message {

     size_t bufsizebytes_read;

     size_t bufsize;

+    size_t pos;

     char *buf;

-    char *pos;

     unsigned char bufsizebytes[4];

     size_t n_left;

 };

+struct outgoing_message {

+    sg_buf sgbuf[2];

+    sg_buf *sgp;

+    int sg_count;

+    unsigned char msg_len_buf[4];

+};

+

 struct conn_state {

     SOCKET fd;

     enum conn_states state;

     int (*service)(krb5_context context, struct conn_state *,

                    struct select_state *, int);

     struct remote_address addr;

-    struct {

-        struct {

-            sg_buf sgbuf[2];

-            sg_buf *sgp;

-            int sg_count;

-            unsigned char msg_len_buf[4];

-        } out;

-        struct incoming_krb5_message in;

-    } x;

+    struct incoming_message in;

+    struct outgoing_message out;

     krb5_data callback_buffer;

     size_t server_index;

     struct conn_state *next;

@@ -461,30 +461,31 @@ static int service_tcp_fd(krb5_context context, struct conn_state *conn,

 static int service_udp_fd(krb5_context context, struct conn_state *conn,

                           struct select_state *selstate, int ssflags);

+/* Set up the actual message we will send across the underlying transport to

+ * communicate the payload message, using one or both of state->out.sgbuf. */

 static void

-set_conn_state_msg_length (struct conn_state *state, const krb5_data *message)

+set_transport_message(struct conn_state *state, const krb5_data *message)

 {

-    if (!message || message->length == 0)

+    struct outgoing_message *out = &state->out;

+

+    if (message == NULL || message->length == 0)

         return;

     if (state->addr.type == SOCK_STREAM) {

-        store_32_be(message->length, state->x.out.msg_len_buf);

-        SG_SET(&state->x.out.sgbuf[0], state->x.out.msg_len_buf, 4);

-        SG_SET(&state->x.out.sgbuf[1], message->data, message->length);

-        state->x.out.sg_count = 2;

-

+        store_32_be(message->length, out->msg_len_buf);

+        SG_SET(&out->sgbuf[0], out->msg_len_buf, 4);

+        SG_SET(&out->sgbuf[1], message->data, message->length);

+        out->sg_count = 2;

     } else {

-

-        SG_SET(&state->x.out.sgbuf[0], message->data, message->length);

-        SG_SET(&state->x.out.sgbuf[1], 0, 0);

-        state->x.out.sg_count = 1;

-

+        SG_SET(&out->sgbuf[0], message->data, message->length);

+        SG_SET(&out->sgbuf[1], NULL, 0);

+        out->sg_count = 1;

     }

 }

 static krb5_error_code

 add_connection(struct conn_state **conns, struct addrinfo *ai,

-               size_t server_index, const krb5_data *message, char **udpbufp)

+               size_t server_index, char **udpbufp)

 {

     struct conn_state *state, **tailptr;

@@ -492,28 +493,26 @@ add_connection(struct conn_state **conns, struct addrinfo *ai,

     if (state == NULL)

         return ENOMEM;

     state->state = INITIALIZING;

-    state->x.out.sgp = state->x.out.sgbuf;

+    state->out.sgp = state->out.sgbuf;

     state->addr.type = ai->ai_socktype;

     state->addr.family = ai->ai_family;

     state->addr.len = ai->ai_addrlen;

     memcpy(&state->addr.saddr, ai->ai_addr, ai->ai_addrlen);

     state->fd = INVALID_SOCKET;

     state->server_index = server_index;

-    SG_SET(&state->x.out.sgbuf[1], 0, 0);

+    SG_SET(&state->out.sgbuf[1], NULL, 0);

     if (ai->ai_socktype == SOCK_STREAM) {

         state->service = service_tcp_fd;

-        set_conn_state_msg_length (state, message);

     } else {

         state->service = service_udp_fd;

-        set_conn_state_msg_length (state, message);

         if (*udpbufp == NULL) {

             *udpbufp = malloc(MAX_DGRAM_SIZE);

             if (*udpbufp == 0)

                 return ENOMEM;

         }

-        state->x.in.buf = *udpbufp;

-        state->x.in.bufsize = MAX_DGRAM_SIZE;

+        state->in.buf = *udpbufp;

+        state->in.bufsize = MAX_DGRAM_SIZE;

     }

     /* Chain the new state onto the tail of the list. */

@@ -597,7 +596,7 @@ resolve_server(krb5_context context, const struct serverlist *servers,

         ai.ai_family = entry->family;

         ai.ai_addrlen = entry->addrlen;

         ai.ai_addr = (struct sockaddr *)&entry->addr;

-        return add_connection(conns, &ai, ind, message, udpbufp);

+        return add_connection(conns, &ai, ind, udpbufp);

     }

     memset(&hint, 0, sizeof(hint));

@@ -617,12 +616,12 @@ resolve_server(krb5_context context, const struct serverlist *servers,

     /* Add each address with the preferred socktype. */

     retval = 0;

     for (a = addrs; a != 0 && retval == 0; a = a->ai_next)

-        retval = add_connection(conns, a, ind, message, udpbufp);

+        retval = add_connection(conns, a, ind, udpbufp);

     if (retval == 0 && entry->socktype == 0 && socktype2 != 0) {

         /* Add each address again with the non-preferred socktype. */

         for (a = addrs; a != 0 && retval == 0; a = a->ai_next) {

             a->ai_socktype = socktype2;

-            retval = add_connection(conns, a, ind, message, udpbufp);

+            retval = add_connection(conns, a, ind, udpbufp);

         }

     }

     freeaddrinfo(addrs);

@@ -631,7 +630,7 @@ resolve_server(krb5_context context, const struct serverlist *servers,

 static int

 start_connection(krb5_context context, struct conn_state *state,

-                 struct select_state *selstate,

+                 const krb5_data *message, struct select_state *selstate,

                  struct sendto_callback_info *callback_info)

 {

     int fd, e;

@@ -689,13 +688,14 @@ start_connection(krb5_context context, struct conn_state *state,

             return -3;

         }

-        set_conn_state_msg_length(state, &state->callback_buffer);

+        message = &state->callback_buffer;

     }

+    set_transport_message(state, message);

     if (state->addr.type == SOCK_DGRAM) {

         /* Send it now.  */

         ssize_t ret;

-        sg_buf *sg = &state->x.out.sgbuf[0];

+        sg_buf *sg = &state->out.sgbuf[0];

         TRACE_SENDTO_KDC_UDP_SEND_INITIAL(context, &state->addr);

         ret = send(state->fd, SG_BUF(sg), SG_LEN(sg), 0);

@@ -731,14 +731,16 @@ start_connection(krb5_context context, struct conn_state *state,

    next connection.  */

 static int

 maybe_send(krb5_context context, struct conn_state *conn,

-           struct select_state *selstate,

+           const krb5_data *message, struct select_state *selstate,

            struct sendto_callback_info *callback_info)

 {

     sg_buf *sg;

     ssize_t ret;

-    if (conn->state == INITIALIZING)

-        return start_connection(context, conn, selstate, callback_info);

+    if (conn->state == INITIALIZING) {

+        return start_connection(context, conn, message, selstate,

+                                callback_info);

+    }

     /* Did we already shut down this channel?  */

     if (conn->state == FAILED) {

@@ -752,7 +754,7 @@ maybe_send(krb5_context context, struct conn_state *conn,

     }

     /* UDP - retransmit after a previous attempt timed out. */

-    sg = &conn->x.out.sgbuf[0];

+    sg = &conn->out.sgbuf[0];

     TRACE_SENDTO_KDC_UDP_SEND_RETRY(context, &conn->addr);

     ret = send(conn->fd, SG_BUF(sg), SG_LEN(sg), 0);

     if (ret < 0 || (size_t) ret != SG_LEN(sg)) {

@@ -803,6 +805,8 @@ service_tcp_fd(krb5_context context, struct conn_state *conn,

     int e = 0;

     ssize_t nwritten, nread;

     SOCKET_WRITEV_TEMP tmp;

+    struct incoming_message *in = &conn->in;

+    struct outgoing_message *out = &conn->out;

     /* Check for a socket exception. */

     if (ssflags & SSF_EXCEPTION)

@@ -825,68 +829,68 @@ service_tcp_fd(krb5_context context, struct conn_state *conn,

         /* Fall through. */

     case WRITING:

         TRACE_SENDTO_KDC_TCP_SEND(context, &conn->addr);

-        nwritten = SOCKET_WRITEV(conn->fd, conn->x.out.sgp,

-                                 conn->x.out.sg_count, tmp);

+        nwritten = SOCKET_WRITEV(conn->fd, out->sgp, out->sg_count, tmp);

         if (nwritten < 0) {

             TRACE_SENDTO_KDC_TCP_ERROR_SEND(context, &conn->addr,

                                             SOCKET_ERRNO);

             goto kill_conn;

         }

         while (nwritten) {

-            sg_buf *sgp = conn->x.out.sgp;

+            sg_buf *sgp = out->sgp;

             if ((size_t) nwritten < SG_LEN(sgp)) {

                 SG_ADVANCE(sgp, (size_t) nwritten);

                 nwritten = 0;

             } else {

                 nwritten -= SG_LEN(sgp);

-                conn->x.out.sgp++;

-                conn->x.out.sg_count--;

+                out->sgp++;

+                out->sg_count--;

             }

         }

-        if (conn->x.out.sg_count == 0) {

+        if (out->sg_count == 0) {

             /* Done writing, switch to reading. */

             cm_read(selstate, conn->fd);

             conn->state = READING;

-            conn->x.in.bufsizebytes_read = 0;

-            conn->x.in.bufsize = 0;

-            conn->x.in.buf = 0;

-            conn->x.in.pos = 0;

-            conn->x.in.n_left = 0;

+            in->bufsizebytes_read = 0;

+            in->bufsize = 0;

+            in->pos = 0;

+            in->buf = NULL;

+            in->n_left = 0;

         }

         return 0;

     case READING:

-        if (conn->x.in.bufsizebytes_read == 4) {

+        if (in->bufsizebytes_read == 4) {

             /* Reading data.  */

-            nread = SOCKET_READ(conn->fd, conn->x.in.pos, conn->x.in.n_left);

+            nread = SOCKET_READ(conn->fd, &in->buf[in->pos], in->n_left);

             if (nread <= 0) {

                 e = nread ? SOCKET_ERRNO : ECONNRESET;

                 TRACE_SENDTO_KDC_TCP_ERROR_RECV(context, &conn->addr, e);

                 goto kill_conn;

             }

-            conn->x.in.n_left -= nread;

-            conn->x.in.pos += nread;

-            if (conn->x.in.n_left <= 0)

+            in->n_left -= nread;

+            in->pos += nread;

+            if (in->n_left <= 0)

                 return 1;

         } else {

             /* Reading length.  */

             nread = SOCKET_READ(conn->fd,

-                                conn->x.in.bufsizebytes + conn->x.in.bufsizebytes_read,

-                                4 - conn->x.in.bufsizebytes_read);

+                                in->bufsizebytes + in->bufsizebytes_read,

+                                4 - in->bufsizebytes_read);

             if (nread <= 0) {

                 e = nread ? SOCKET_ERRNO : ECONNRESET;

                 TRACE_SENDTO_KDC_TCP_ERROR_RECV_LEN(context, &conn->addr, e);

                 goto kill_conn;

             }

-            conn->x.in.bufsizebytes_read += nread;

-            if (conn->x.in.bufsizebytes_read == 4) {

-                unsigned long len = load_32_be (conn->x.in.bufsizebytes);

+            in->bufsizebytes_read += nread;

+            if (in->bufsizebytes_read == 4) {

+                unsigned long len = load_32_be(in->bufsizebytes);

                 /* Arbitrary 1M cap.  */

                 if (len > 1 * 1024 * 1024)

                     goto kill_conn;

-                conn->x.in.bufsize = conn->x.in.n_left = len;

-                conn->x.in.buf = conn->x.in.pos = malloc(len);

-                if (conn->x.in.buf == 0)

+                in->bufsize = in->n_left = len;

+                in->pos = 0;

+                in->buf = malloc(len);

+                if (in->buf == NULL)

                     goto kill_conn;

             }

         }

@@ -915,13 +919,13 @@ service_udp_fd(krb5_context context, struct conn_state *conn,

     if (conn->state != READING)

         abort();

-    nread = recv(conn->fd, conn->x.in.buf, conn->x.in.bufsize, 0);

+    nread = recv(conn->fd, conn->in.buf, conn->in.bufsize, 0);

     if (nread < 0) {

         TRACE_SENDTO_KDC_UDP_ERROR_RECV(context, &conn->addr, SOCKET_ERRNO);

         kill_conn(conn, selstate);

         return 0;

     }

-    conn->x.in.pos = conn->x.in.buf + nread;

+    conn->in.pos = nread;

     return 1;

 }

@@ -986,10 +990,7 @@ service_fds(krb5_context context, struct select_state *selstate,

                 int stop = 1;

                 if (msg_handler != NULL) {

-                    krb5_data reply;

-

-                    reply.data = state->x.in.buf;

-                    reply.length = state->x.in.pos - state->x.in.buf;

+                    krb5_data reply = make_data(state->in.buf, state->in.pos);

                     stop = (msg_handler(context, &reply, msg_handler_data) != 0);

                 }

@@ -1051,8 +1052,7 @@ k5_sendto(krb5_context context, const krb5_data *message,

     char *udpbuf = NULL;

     krb5_boolean done = FALSE;

-    reply->data = 0;

-    reply->length = 0;

+    *reply = empty_data();

     /* One for use here, listing all our fds in use, and one for

      * temporary use in service_fds, for the fds of interest.  */

@@ -1077,7 +1077,7 @@ k5_sendto(krb5_context context, const krb5_data *message,

             /* Contact each new connection whose socktype matches socktype1. */

             if (state->addr.type != socktype1)

                 continue;

-            if (maybe_send(context, state, sel_state, callback_info))

+            if (maybe_send(context, state, message, sel_state, callback_info))

                 continue;

             done = service_fds(context, sel_state, 1000, conns, seltemp,

                                msg_handler, msg_handler_data, &winner);

@@ -1089,7 +1089,7 @@ k5_sendto(krb5_context context, const krb5_data *message,

     for (state = conns; state != NULL && !done; state = state->next) {

         if (state->addr.type != socktype2)

             continue;

-        if (maybe_send(context, state, sel_state, callback_info))

+        if (maybe_send(context, state, message, sel_state, callback_info))

             continue;

         done = service_fds(context, sel_state, 1000, conns, seltemp,

                            msg_handler, msg_handler_data, &winner);

@@ -1105,7 +1105,7 @@ k5_sendto(krb5_context context, const krb5_data *message,

     delay = 4000;

     for (pass = 1; pass < MAX_PASS && !done; pass++) {

         for (state = conns; state != NULL && !done; state = state->next) {

-            if (maybe_send(context, state, sel_state, callback_info))

+            if (maybe_send(context, state, message, sel_state, callback_info))

                 continue;

             done = service_fds(context, sel_state, 1000, conns, seltemp,

                                msg_handler, msg_handler_data, &winner);

@@ -1127,10 +1127,9 @@ k5_sendto(krb5_context context, const krb5_data *message,

         goto cleanup;

     }

     /* Success!  */

-    reply->data = winner->x.in.buf;

-    reply->length = winner->x.in.pos - winner->x.in.buf;

+    *reply = make_data(winner->in.buf, winner->in.pos);

     retval = 0;

-    winner->x.in.buf = NULL;

+    winner->in.buf = NULL;

     if (server_used != NULL)

         *server_used = winner->server_index;

     if (remoteaddr != NULL && remoteaddrlen != 0 && *remoteaddrlen > 0)

@@ -1142,8 +1141,8 @@ cleanup:

         next = state->next;

         if (state->fd != INVALID_SOCKET)

             closesocket(state->fd);

-        if (state->state == READING && state->x.in.buf != udpbuf)

-            free(state->x.in.buf);

+        if (state->state == READING && state->in.buf != udpbuf)

+            free(state->in.buf);

         if (callback_info) {

             callback_info->pfn_cleanup(callback_info->data,

                                        &state->callback_buffer);

-- 

2.1.0