THRIFT-2680 c_glib: ThriftFramedTransport fails when peer unexpectedly closes connection
Patch: Simon South
diff --git a/lib/c_glib/src/thrift/c_glib/transport/thrift_framed_transport.c b/lib/c_glib/src/thrift/c_glib/transport/thrift_framed_transport.c
index a58b875..9810aa6 100644
--- a/lib/c_glib/src/thrift/c_glib/transport/thrift_framed_transport.c
+++ b/lib/c_glib/src/thrift/c_glib/transport/thrift_framed_transport.c
@@ -71,25 +71,32 @@
GError **error)
{
ThriftFramedTransport *t = THRIFT_FRAMED_TRANSPORT (transport);
- gint32 sz, bytes;
+ guint32 sz;
+ gint32 bytes;
+ gboolean result = FALSE;
/* read the size */
- THRIFT_TRANSPORT_GET_CLASS (t->transport)->read (t->transport,
- (guint32 *) &sz,
- sizeof (sz), error);
- sz = ntohl (sz);
+ if (thrift_transport_read (t->transport,
+ &sz,
+ sizeof (sz),
+ error) == sizeof (sz))
+ {
+ sz = ntohl (sz);
- /* create a buffer to hold the data and read that much data */
- guchar tmpdata[sz];
- bytes = THRIFT_TRANSPORT_GET_CLASS (t->transport)->read (t->transport,
- tmpdata,
- sz,
- error);
+ /* create a buffer to hold the data and read that much data */
+ guchar tmpdata[sz];
+ bytes = thrift_transport_read (t->transport, tmpdata, sz, error);
- /* add the data to the buffer */
- g_byte_array_append (t->r_buf, tmpdata, bytes);
+ if (bytes > 0 && (error == NULL || *error == NULL))
+ {
+ /* add the data to the buffer */
+ g_byte_array_append (t->r_buf, tmpdata, bytes);
- return TRUE;
+ result = TRUE;
+ }
+ }
+
+ return result;
}
/* the actual read is "slow" because it calls the underlying transport */
@@ -100,6 +107,7 @@
ThriftFramedTransport *t = THRIFT_FRAMED_TRANSPORT (transport);
guint32 want = len;
guint32 have = t->r_buf->len;
+ gint32 result = -1;
// we shouldn't hit this unless the buffer doesn't have enough to read
assert (t->r_buf->len < want);
@@ -113,17 +121,20 @@
}
// read a frame of input and buffer it
- thrift_framed_transport_read_frame (transport, error);
+ if (thrift_framed_transport_read_frame (transport, error) == TRUE)
+ {
+ // hand over what we have up to what the caller wants
+ guint32 give = want < t->r_buf->len ? want : t->r_buf->len;
- // hand over what we have up to what the caller wants
- guint32 give = want < t->r_buf->len ? want : t->r_buf->len;
+ // copy the data into the buffer
+ memcpy (buf + len - want, t->r_buf->data, give);
+ t->r_buf = g_byte_array_remove_range (t->r_buf, 0, give);
+ want -= give;
- // copy the data into the buffer
- memcpy (buf + len - want, t->r_buf->data, give);
- t->r_buf = g_byte_array_remove_range (t->r_buf, 0, give);
- want -= give;
+ result = len - want;
+ }
- return (len - want);
+ return result;
}
/* implements thrift_transport_read */
diff --git a/lib/c_glib/test/testframedtransport.c b/lib/c_glib/test/testframedtransport.c
index 03c9f9b..843ad93 100755
--- a/lib/c_glib/test/testframedtransport.c
+++ b/lib/c_glib/test/testframedtransport.c
@@ -144,6 +144,92 @@
}
}
+/* test reading from the transport after the peer has unexpectedly
+ closed the connection */
+static void
+test_read_after_peer_close(void)
+{
+ int status;
+ pid_t pid;
+ int port = 51199;
+ GError *err = NULL;
+
+ pid = fork ();
+ g_assert (pid >= 0);
+
+ if (pid == 0)
+ {
+ ThriftServerTransport *server_transport = NULL;
+ ThriftTransport *client_transport = NULL;
+
+ /* child listens */
+ server_transport = g_object_new (THRIFT_TYPE_SERVER_SOCKET,
+ "port", port,
+ NULL);
+ g_assert (server_transport != NULL);
+
+ thrift_server_transport_listen (server_transport, &err);
+ g_assert (err == NULL);
+
+ /* wrap the client transport in a ThriftFramedTransport */
+ client_transport = g_object_new
+ (THRIFT_TYPE_FRAMED_TRANSPORT,
+ "transport", thrift_server_transport_accept (server_transport, &err),
+ "r_buf_size", 0,
+ NULL);
+ g_assert (err == NULL);
+ g_assert (client_transport != NULL);
+
+ /* close the connection immediately after the client connects */
+ thrift_transport_close (client_transport, NULL);
+
+ g_object_unref (client_transport);
+ g_object_unref (server_transport);
+
+ exit (0);
+ } else {
+ ThriftSocket *tsocket = NULL;
+ ThriftTransport *transport = NULL;
+ guchar buf[10]; /* a buffer */
+
+ /* parent connects, wait a bit for the socket to be created */
+ sleep (1);
+
+ tsocket = g_object_new (THRIFT_TYPE_SOCKET,
+ "hostname", "localhost",
+ "port", port,
+ NULL);
+ transport = g_object_new (THRIFT_TYPE_FRAMED_TRANSPORT,
+ "transport", THRIFT_TRANSPORT (tsocket),
+ "w_buf_size", 0,
+ NULL);
+
+ g_assert (thrift_transport_open (transport, NULL) == TRUE);
+ g_assert (thrift_transport_is_open (transport));
+
+ /* attempting to read from the transport after the peer has closed
+ the connection fails gracefully without generating a critical
+ warning or segmentation fault */
+ thrift_transport_read (transport, buf, 10, &err);
+ g_assert (err != NULL);
+
+ g_error_free (err);
+ err = NULL;
+
+ thrift_transport_read_end (transport, &err);
+ g_assert (err == NULL);
+
+ thrift_transport_close (transport, &err);
+ g_assert (err == NULL);
+
+ g_object_unref (transport);
+ g_object_unref (tsocket);
+
+ g_assert (wait (&status) == pid);
+ g_assert (status == 0);
+ }
+}
+
static void
thrift_server (const int port)
{
@@ -191,6 +277,7 @@
g_test_add_func ("/testframedtransport/CreateAndDestroy", test_create_and_destroy);
g_test_add_func ("/testframedtransport/OpenAndClose", test_open_and_close);
g_test_add_func ("/testframedtransport/ReadAndWrite", test_read_and_write);
+ g_test_add_func ("/testframedtransport/ReadAfterPeerClose", test_read_after_peer_close);
return g_test_run ();
}