From 0b34902dc2023319a5a17795aca14eeda365ed5d Mon Sep 17 00:00:00 2001 From: Sebastian Harl Date: Wed, 14 May 2014 20:38:17 +0200 Subject: [PATCH] frontend: Fix invalid command handling when receiving data in chunks. When skipping over invalid command data, make sure not to skip more data than what's currently available. Rather, remember how much data needs to be ignored and do so after actually receiving it. Added a new test which catches these cases and also provides some more low-level communication tests. --- src/frontend/connection-private.h | 4 + src/frontend/connection.c | 26 +++++- t/unit/frontend/connection_test.c | 132 ++++++++++++++++++++++++++++++ 3 files changed, 159 insertions(+), 3 deletions(-) diff --git a/src/frontend/connection-private.h b/src/frontend/connection-private.h index 574049e..ae9cbbc 100644 --- a/src/frontend/connection-private.h +++ b/src/frontend/connection-private.h @@ -64,6 +64,10 @@ struct sdb_conn { uint32_t cmd; uint32_t cmd_len; + /* amount of data to skip, e.g., after receiving invalid commands; if this + * is non-zero, the 'skip_len' first bytes of 'buf' are invalid */ + size_t skip_len; + sdb_strbuf_t *errbuf; /* user information */ diff --git a/src/frontend/connection.c b/src/frontend/connection.c index 923f000..b913939 100644 --- a/src/frontend/connection.c +++ b/src/frontend/connection.c @@ -117,6 +117,7 @@ connection_init(sdb_object_t *obj, va_list ap) conn->cmd = CONNECTION_IDLE; conn->cmd_len = 0; + conn->skip_len = 0; /* update the object name */ snprintf(obj->name + strlen(CONN_FD_PREFIX), @@ -259,6 +260,7 @@ command_handle(sdb_conn_t *conn) int status = -1; assert(conn && (conn->cmd != CONNECTION_IDLE)); + assert(! conn->skip_len); sdb_log(SDB_LOG_DEBUG, "frontend: Handling command %u (len: %u)", conn->cmd, conn->cmd_len); @@ -368,22 +370,33 @@ command_init(sdb_conn_t *conn) assert(conn && (conn->cmd == CONNECTION_IDLE) && (! conn->cmd_len)); + if (conn->skip_len) + return -1; + /* reset */ sdb_strbuf_sprintf(conn->errbuf, ""); conn->cmd = connection_get_int32(conn, 0); conn->cmd_len = connection_get_int32(conn, sizeof(uint32_t)); - len = 2 * sizeof(uint32_t); if (conn->cmd == CONNECTION_IDLE) { const char *errmsg = "Invalid command 0"; sdb_strbuf_sprintf(conn->errbuf, errmsg); sdb_connection_send(conn, CONNECTION_ERROR, (uint32_t)strlen(errmsg), errmsg); - len += conn->cmd_len; + conn->skip_len += conn->cmd_len; conn->cmd_len = 0; } - sdb_strbuf_skip(conn->buf, 0, len); + sdb_strbuf_skip(conn->buf, 0, 2 * sizeof(uint32_t)); + + if (conn->skip_len) { + len = sdb_strbuf_len(conn->buf); + if (len > conn->skip_len) + len = conn->skip_len; + sdb_strbuf_skip(conn->buf, 0, len); + conn->skip_len -= len; + /* connection_read will handle anything else */ + } return 0; } /* command_init */ @@ -412,6 +425,13 @@ connection_read(sdb_conn_t *conn) else if (! status) /* EOF */ break; + if (conn->skip_len) { + size_t len = (size_t)status < conn->skip_len + ? (size_t)status : conn->skip_len; + sdb_strbuf_skip(conn->buf, 0, len); + conn->skip_len -= len; + } + n += status; } diff --git a/t/unit/frontend/connection_test.c b/t/unit/frontend/connection_test.c index da94f06..43322e0 100644 --- a/t/unit/frontend/connection_test.c +++ b/t/unit/frontend/connection_test.c @@ -31,6 +31,7 @@ #include "frontend/connection.h" #include "frontend/connection-private.h" +#include "utils/proto.h" #include "libsysdb_test.h" #include "utils/strbuf.h" @@ -167,6 +168,31 @@ mock_client(void *arg) return NULL; } /* mock_client */ +static void +connection_setup(sdb_conn_t *conn) +{ + ssize_t check, expected; + + expected = 2 * sizeof(uint32_t) + strlen("fakeuser"); + check = sdb_connection_send(conn, CONNECTION_STARTUP, + (uint32_t)strlen("fakeuser"), "fakeuser"); + fail_unless(check == expected, + "sdb_connection_send(STARTUP, fakeuser) = %zi; expected: %zi", + check, expected); + + mock_conn_rewind(conn); + check = sdb_connection_read(conn); + fail_unless(check == expected, + "On startup: sdb_connection_read() = %zi; expected: %zi", + check, expected); + + fail_unless(sdb_strbuf_len(conn->errbuf) == 0, + "sdb_connection_read() left %zu bytes in the error " + "buffer; expected: 0", sdb_strbuf_len(conn->errbuf)); + + mock_conn_truncate(conn); +} /* connection_setup */ + /* * tests */ @@ -200,6 +226,7 @@ START_TEST(test_conn_accept) } END_TEST +/* test connection setup and very basic commands */ START_TEST(test_conn_setup) { sdb_conn_t *conn = mock_conn_create(); @@ -209,6 +236,7 @@ START_TEST(test_conn_setup) const char *msg; const char *err; } golden_data[] = { + /* code == UINT32_MAX => no data will be sent */ { UINT32_MAX, NULL, NULL }, { CONNECTION_IDLE, "fakedata", "Invalid command 0" }, { CONNECTION_PING, NULL, "Authentication required" }, @@ -267,6 +295,109 @@ START_TEST(test_conn_setup) } END_TEST +/* test simple I/O on open connections */ +START_TEST(test_conn_io) +{ + sdb_conn_t *conn = mock_conn_create(); + + struct { + uint32_t code; + uint32_t msg_len; + const char *msg; + size_t buf_len; /* number of bytes we expect in conn->buf */ + const char *err; + } golden_data[] = { + /* code == UINT32_MAX => this is a follow-up package */ + { CONNECTION_PING, 20, "9876543210", 10, NULL }, + { UINT32_MAX, -1, "9876543210", 0, NULL }, + { CONNECTION_IDLE, 20, "9876543210", 0, "Invalid command 0" }, + { UINT32_MAX, -1, "9876543210", 0, "Invalid command 0" }, + { CONNECTION_IDLE, 20, "9876543210", 0, "Invalid command 0" }, + { UINT32_MAX, -1, "9876543210", 0, "Invalid command 0" }, + { CONNECTION_PING, 10, "9876543210", 0, NULL }, + { CONNECTION_PING, 20, "9876543210", 10, NULL }, + { UINT32_MAX, -1, "9876543210", 0, NULL }, + }; + + size_t i; + + connection_setup(conn); + + for (i = 0; i < SDB_STATIC_ARRAY_LEN(golden_data); ++i) { + size_t msg_len = strlen(golden_data[i].msg); + char buffer[2 * sizeof(uint32_t) + msg_len]; + size_t offset = 0; + + ssize_t check; + + mock_conn_truncate(conn); + + if (golden_data[i].code != UINT32_MAX) { + uint32_t tmp; + + tmp = htonl(golden_data[i].code); + memcpy(buffer, &tmp, sizeof(tmp)); + tmp = htonl(golden_data[i].msg_len); + memcpy(buffer + sizeof(tmp), &tmp, sizeof(tmp)); + + msg_len += 2 * sizeof(uint32_t); + offset += 2 * sizeof(uint32_t); + } + + memcpy(buffer + offset, golden_data[i].msg, + strlen(golden_data[i].msg)); + + check = sdb_proto_send(conn->fd, msg_len, buffer); + fail_unless(check == (ssize_t)msg_len, + "sdb_proto_send(%s) = %zi; expected: %zu", + check, msg_len); + + mock_conn_rewind(conn); + check = sdb_connection_read(conn); + fail_unless(check == (ssize_t)msg_len, + "sdb_connection_read() = %zi; expected: %zu", + check, msg_len); + + if (golden_data[i].buf_len) { + /* partial commands need to be stored in the object */ + fail_unless(conn->cmd == golden_data[i].code, + "sdb_connection_read() set partial command " + "to %u; expected: %u", conn->cmd, golden_data[i].code); + fail_unless(conn->cmd_len > golden_data[i].buf_len, + "sdb_connection_read() set partial command length " + "to %u; expected: > %u", conn->cmd_len, + golden_data[i].buf_len); + } + else { + fail_unless(conn->cmd == CONNECTION_IDLE, + "sdb_connection_read() did not reset command; " + "got %u; expected: %u", conn->cmd, CONNECTION_IDLE); + fail_unless(conn->cmd_len == 0, + "sdb_connection_read() did not reset command length; " + "got %u; expected: 0", conn->cmd_len); + } + + fail_unless(sdb_strbuf_len(conn->buf) == golden_data[i].buf_len, + "sdb_connection_read() left %zu bytes in the buffer; " + "expected: %zu", sdb_strbuf_len(conn->buf), + golden_data[i].buf_len); + + if (golden_data[i].err) { + const char *err = sdb_strbuf_string(conn->errbuf); + fail_unless(strcmp(err, golden_data[i].err) == 0, + "sdb_connection_read(): got error '%s'; " + "expected: '%s'", err, golden_data[i].err); + } + else + fail_unless(sdb_strbuf_len(conn->errbuf) == 0, + "sdb_connection_read() left %zu bytes in the error " + "buffer; expected: 0", sdb_strbuf_len(conn->errbuf)); + } + + mock_conn_destroy(conn); +} +END_TEST + Suite * fe_conn_suite(void) { @@ -276,6 +407,7 @@ fe_conn_suite(void) tc = tcase_create("core"); tcase_add_test(tc, test_conn_accept); tcase_add_test(tc, test_conn_setup); + tcase_add_test(tc, test_conn_io); suite_add_tcase(s, tc); return s; -- 2.30.2