When we get part of a TCP response, we currently busy-wait until the
rest of the response arrives. Bad Sameo; no biscuit.
We *also* busy-wait for ever if the server closes the TCP connection
without giving us all the data we were expecting. In that case, recv()
returns zero and we just sit in our loop repeating the call over and
over again.
We were also failing to handle a closed connection or short read when
looking for the 2-byte length header.
This untested patch should fix all this. In the very unlikely case of a
short read on the initial 2-byte header, we do will still end up
busy-waiting for the second byte — but that's still better than what we
were doing before, when we'd busy-wait *and* discard the first byte.
And, in fact, we would busy-wait for ever there was any kind of error on
the socket during that header read.
It might make sense to discard a partial response which gets truncated
due to an error, rather than returning what we have managed to receive.
A full response might come in from a different server. Or a better
option might be to keep the *longest* of the partial responses for a
given request, and feed that back to the client if all servers fail to
give us a full reply. I've left the behaviour as it was for now.
---
plugins/dnsproxy.c | 84 ++++++++++++++++++++++++++++++++++------------------
1 files changed, 55 insertions(+), 29 deletions(-)
diff --git a/plugins/dnsproxy.c b/plugins/dnsproxy.c
index a241399..fa34ffa 100644
--- a/plugins/dnsproxy.c
+++ b/plugins/dnsproxy.c
@@ -75,6 +75,12 @@ struct domain_hdr {
#error "Unknown byte order"
#endif
+struct partial_reply {
+ uint16_t len;
+ uint16_t received;
+ unsigned char buf[];
+};
+
struct server_data {
char *interface;
char *domain;
@@ -85,6 +91,7 @@ struct server_data {
guint timeout;
gboolean enabled;
gboolean connected;
+ struct partial_reply *incoming_reply;
};
struct request_data {
@@ -441,6 +448,7 @@ static void destroy_server(struct server_data *server)
if (server->protocol == IPPROTO_UDP)
connman_info("Removing DNS server %s", server->server);
+ g_free(server->incoming_reply);
g_free(server->server);
g_free(server->domain);
g_free(server->interface);
@@ -484,9 +492,14 @@ static gboolean tcp_server_event(GIOChannel *channel,
GIOCondition condition,
if (condition & (G_IO_NVAL | G_IO_ERR | G_IO_HUP)) {
GSList *list;
-
+ hangup:
DBG("TCP server channel closed");
+ /* Discard any partial response which is buffered; better
+ to get a proper response from a working server. */
+ g_free(server->incoming_reply);
+ server->incoming_reply = NULL;
+
for (list = request_list; list; list = list->next) {
struct request_data *req = list->data;
struct domain_hdr *hdr;
@@ -546,49 +559,62 @@ static gboolean tcp_server_event(GIOChannel *channel,
GIOCondition condition,
}
} else if (condition & G_IO_IN) {
- int len, bytes_recv, total_bytes_recv;
- unsigned char reply_len_buf[2];
- uint16_t reply_len;
- unsigned char *reply;
+ struct partial_reply *reply = server->incoming_reply;
+ int bytes_recv;
- len = recv(sk, reply_len_buf, 2, 0);
- if (len < 2)
- return TRUE;
+ if (!reply) {
+ unsigned char reply_len_buf[2];
+ uint16_t reply_len;
- reply_len = reply_len_buf[1] | reply_len_buf[0] << 8;
+ bytes_recv = recv(sk, reply_len_buf, 2, MSG_PEEK);
+ if (!bytes_recv) {
+ goto hangup;
+ } else if (bytes_recv < 0) {
+ if (errno == EAGAIN || errno == EWOULDBLOCK)
+ return TRUE;
- DBG("TCP reply %d bytes", reply_len);
+ connman_error("DNS proxy error %s",
+ strerror(errno));
+ goto hangup;
+ } else if (bytes_recv < 2)
+ return TRUE;
- reply = g_try_malloc0(reply_len + 2);
- if (reply == NULL)
- return TRUE;
+ reply_len = reply_len_buf[1] | reply_len_buf[0] << 8;
+ reply_len += 2;
- reply[0] = reply_len_buf[0];
- reply[1] = reply_len_buf[1];
+ DBG("TCP reply %d bytes", reply_len);
- total_bytes_recv = bytes_recv = 0;
- while (total_bytes_recv < reply_len) {
- bytes_recv = recv(sk, reply + 2 + total_bytes_recv,
- reply_len - total_bytes_recv, 0);
- if (bytes_recv < 0) {
+ reply = g_try_malloc(sizeof(*reply) + reply_len);
+ if (!reply)
+ return TRUE;
+
+ reply->len = reply_len;
+ reply->received = 0;
+
+ server->incoming_reply = reply;
+ }
+
+ while (reply->received < reply->len) {
+ bytes_recv = recv(sk, reply + reply->received,
+ reply->len - reply->received, 0);
+ if (!bytes_recv) {
+ connman_error("DNS proxy TCP disconnect");
+ break;
+ } else if (bytes_recv < 0) {
if (errno == EAGAIN || errno == EWOULDBLOCK)
- continue;
+ return TRUE;
connman_error("DNS proxy error %s",
- strerror(errno));
-
+ strerror(errno));
break;
}
-
- total_bytes_recv += bytes_recv;
+ reply->received += bytes_recv;
}
- if (total_bytes_recv > reply_len)
- total_bytes_recv = reply_len;
-
- forward_dns_reply(reply, total_bytes_recv + 2, IPPROTO_TCP);
+ forward_dns_reply(reply->buf, reply->received, IPPROTO_TCP);
g_free(reply);
+ server->incoming_reply = NULL;
destroy_server(server);
--
1.7.3.2
--
David Woodhouse Open Source Technology Centre
[email protected] Intel Corporation
_______________________________________________
connman mailing list
[email protected]
http://lists.connman.net/listinfo/connman
