From 55b1a9b760e214f9218a284ff96e5fb093401be4 Mon Sep 17 00:00:00 2001 From: hippoz <10706925-hippoz@users.noreply.gitlab.com> Date: Wed, 11 Jan 2023 18:44:38 +0200 Subject: [PATCH] fix message bound issues --- server.c | 54 +++++++++++++++++++++++++++++++++++++++++++++--------- wire.c | 1 + 2 files changed, 46 insertions(+), 9 deletions(-) diff --git a/server.c b/server.c index c138c30..458989c 100644 --- a/server.c +++ b/server.c @@ -288,6 +288,11 @@ ssize_t bus_client_recv(bus_t *s, int i, void *buf, size_t n) int bus_broadcast_message(bus_t *s, wire_message_t *msg, wire_context_t *ctx, char *sender_unique_name) { + uint32_t body_end = ctx->byte_cursor + msg->body_length; + if (body_end >= ctx->data_len) { + return -1; + } + int left = s->clients_count; for (int i = 0; i < BUS_MAX_CLIENTS && left > 0; i++) { if (s->clients[i].match_count <= 0) { @@ -320,6 +325,10 @@ int bus_broadcast_message(bus_t *s, wire_message_t *msg, wire_context_t *ctx, ch } } } + + // We rely on the byte_cursor being at the end of the message upon processing. + ctx->byte_cursor = body_end; + return 0; } @@ -330,12 +339,12 @@ int bus_unicast_message(bus_t *s, wire_message_t *msg, wire_context_t *ctx, char return -1; } - uint8_t reply_data[4096]; - memset(reply_data, 0, 4096); + uint8_t reply_data[8192]; + memset(reply_data, 0, 8192); wire_context_t reply_ctx = { .byte_cursor = 0, .data = reply_data, - .data_len = 4096, + .data_len = 8192, }; TRYST(wire_compose_unicast_reply(&reply_ctx, ctx, msg, sender_unique_name)); @@ -511,6 +520,8 @@ int handle_remove_match(bus_t *s, int i, wire_message_t *msg, wire_context_t *ct int handle_get_connection_stats(bus_t *s, int i, wire_message_t *msg, wire_context_t *ctx, wire_context_t *reply_ctx) { (void)ctx; + TRYPTR(wire_get_name_string(ctx)); + // TODO: stub (always returns empty array) printf("FIXME: STUB: GetConnectionStats\n"); @@ -628,6 +639,11 @@ int bus_client_process_message(bus_t *s, int i, wire_context_t *ctx) return -1; } + uint32_t body_end = ctx->byte_cursor + msg.body_length; + if (body_end >= ctx->data_len) { + return -1; + } + wire_message_field_t *destination_field = &msg.fields[DBUS_HEADER_FIELD_DESTINATION]; wire_message_field_t *member_field = &msg.fields[DBUS_HEADER_FIELD_MEMBER]; @@ -645,12 +661,12 @@ int bus_client_process_message(bus_t *s, int i, wire_context_t *ctx) const bus_method_handler_t *handler = &bus_method_handlers[j]; if (strcmp(handler->name, member_field->t.str) == 0) { TRYST(handler->handler(s, i, &msg, ctx, &reply_ctx)); - return 0; + goto end_nonfatal; } } printf("FIXME: daemon method '%s' is not implemented or invalid\n", member_field->t.str); _process_message_reply_error("org.freedesktop.DBus.Error.UnknownMethod"); - return 0; + goto end_nonfatal; } // message needs to be routed @@ -663,15 +679,32 @@ int bus_client_process_message(bus_t *s, int i, wire_context_t *ctx) if (destination_field->present) { if (bus_unicast_message(s, &msg, ctx, destination_field->t.str, s->names[client->unique_name_index].name) < 0) { _process_message_reply_error("xyz.hippoz.jitterbug.UnicastFailed"); - return 0; + goto end_nonfatal; } } else { if (bus_broadcast_message(s, &msg, ctx, s->names[client->unique_name_index].name) < 0) { _process_message_reply_error("xyz.hippoz.jitterbug.BroadcastFailed"); - return 0; + goto end_nonfatal; } } +end_nonfatal: + if (ctx->byte_cursor != body_end) { + // The code above did not properly consume the entire message while parsing. + // This is bad, because we expect the byte_cursor after this function to be + // exactly at the end of a message. We rely on this invariant for message + // boundries. We can obviously just set the byte_cursor to the body_end + // that we have calculated and move on, however, not consuming the entire + // message is nonetheless a bug that should be fixed, or an indicator of + // another bug or something going wrong. + // In this case, I've decided to set the byte_cursor to the end of the + // body and print an error message. In the future, we may want to have + // separate debug builds, and maybe exit the program if we are in a + // debug build and we encounter something like this. + fprintf(stderr, "notice: parsing code did not consume the entire message (expected: %d, actual: %d)\n", body_end, ctx->byte_cursor); + ctx->byte_cursor = body_end; + } + return 0; } @@ -685,7 +718,8 @@ int bus_client_drain_messages(bus_t *s, int ci, uint8_t *data, uint32_t data_len TRYST(bus_client_process_message(s, ci, &ctx)); - for (int i = 0; i < 10; i++) { + int i = 0; + for (; i < 10; i++) { if (ctx.byte_cursor >= ctx.data_len || ctx.data[ctx.byte_cursor] != 'l') { // no more messages break; @@ -696,6 +730,8 @@ int bus_client_drain_messages(bus_t *s, int ci, uint8_t *data, uint32_t data_len TRYST(bus_client_process_message(s, ci, &ctx)); } + printf("processed %d messages - cursor:%d; data_len:%ld\n", i + 1, ctx.byte_cursor, ctx.data_len); + return 0; } @@ -706,7 +742,7 @@ int bus_turn(bus_t *s) static const char auth_ok[] = "OK aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa\r\n"; static const char auth_list[] = "REJECTED EXTERNAL\r\n"; static const char auth_data[] = "DATA\r\n"; - static const int data_buffer_len = 8192; + static const int data_buffer_len = 16384; // We can keep a padding of null bytes for the data buffer. In the event that, for // example, a string function is called on a char array without a proper ending null diff --git a/wire.c b/wire.c index 7382521..a36b092 100644 --- a/wire.c +++ b/wire.c @@ -383,6 +383,7 @@ int wire_compose_unicast_reply(wire_context_t *c, wire_context_t *msg_c, wire_me memcpy(&c->data[c->byte_cursor], &msg_c->data[msg_c->byte_cursor], msg->body_length); c->byte_cursor += msg->body_length; + msg_c->byte_cursor += msg->body_length; return 0; }