From b2d6883965b19c6b6cb47acac952a7ab3de88c11 Mon Sep 17 00:00:00 2001 From: Jonathan Rajotte Date: Wed, 12 Jan 2022 18:18:08 -0500 Subject: [PATCH] Fix: liblttng-ctl comm: lttng_event_field is not packed MIME-Version: 1.0 Content-Type: text/plain; charset=utf8 Content-Transfer-Encoding: 8bit Observed issue ============== For MI testing where the lttng-sessiond is 64 bit and the lttng CLI is 32 bit, the tracepoint field listing fails with partial garbage output. The size of the struct differs between bitness for x86-64 and x86 leading to serialization/deserialization problem across client (liblttng-ctl) and lttng-sessiond. sizeof(struct lttng_event_field): x86: 1136 x86-64: 1144 The struct cannot be marked as LTTNG_PACKED since it is part of the API. Solution ======== Adopt a similar pattern to the new APIs with a "serialize" & "create_from_buffer" approach. The only particularity is that we need to flatten the event_field on listing. Most of the complexity is moved to `src/common/event.c` Known drawbacks ========= None. Signed-off-by: Jonathan Rajotte Signed-off-by: Jérémie Galarneau Change-Id: I280d9809d110237574e2606ee93a7aeba41e704e --- include/lttng/event-internal.h | 26 +++ src/bin/lttng-sessiond/client.cpp | 39 ++-- src/bin/lttng-sessiond/cmd.cpp | 59 +++++- src/bin/lttng-sessiond/cmd.h | 4 +- src/common/event.cpp | 341 ++++++++++++++++++++++++++++++ src/lib/lttng-ctl/lttng-ctl.cpp | 61 +++++- 6 files changed, 495 insertions(+), 35 deletions(-) diff --git a/include/lttng/event-internal.h b/include/lttng/event-internal.h index 6eb7ab37a..52f7f7857 100644 --- a/include/lttng/event-internal.h +++ b/include/lttng/event-internal.h @@ -121,6 +121,20 @@ struct lttng_event_context_app_comm { char payload[]; } LTTNG_PACKED; +struct lttng_event_field_comm { + uint8_t type; + uint8_t nowrite; + /* Includes terminator `\0`. */ + uint32_t name_len; + uint32_t event_len; + + /* + * - name [name_len] + * - lttng_event object + */ + char payload[]; +} LTTNG_PACKED; + struct lttng_event_extended { /* * exclusions and filter_expression are only set when the lttng_event @@ -167,4 +181,16 @@ enum lttng_error_code lttng_events_create_and_flatten_from_payload( unsigned int count, struct lttng_event **events); +ssize_t lttng_event_field_create_from_payload( + struct lttng_payload_view *view, + struct lttng_event_field **field); + +int lttng_event_field_serialize(const struct lttng_event_field *field, + struct lttng_payload *payload); + +enum lttng_error_code lttng_event_fields_create_and_flatten_from_payload( + struct lttng_payload_view *view, + unsigned int count, + struct lttng_event_field **fields); + #endif /* LTTNG_EVENT_INTERNAL_H */ diff --git a/src/bin/lttng-sessiond/client.cpp b/src/bin/lttng-sessiond/client.cpp index abd26fcb9..16f1b1bcd 100644 --- a/src/bin/lttng-sessiond/client.cpp +++ b/src/bin/lttng-sessiond/client.cpp @@ -1642,30 +1642,33 @@ skip_domain: } case LTTNG_LIST_TRACEPOINT_FIELDS: { - struct lttng_event_field *fields; - ssize_t nb_fields; + enum lttng_error_code ret_code; + size_t original_payload_size; + size_t payload_size; + const size_t command_header_size = sizeof(struct lttcomm_list_command_header); + + ret = setup_empty_lttng_msg(cmd_ctx); + if (ret) { + ret = LTTNG_ERR_NOMEM; + goto setup_error; + } + + original_payload_size = cmd_ctx->reply_payload.buffer.size; + ERR("original payload size = %i", (int) original_payload_size); session_lock_list(); - nb_fields = cmd_list_tracepoint_fields(cmd_ctx->lsm.domain.type, - &fields); + ret_code = cmd_list_tracepoint_fields( + cmd_ctx->lsm.domain.type, &cmd_ctx->reply_payload); session_unlock_list(); - if (nb_fields < 0) { - /* Return value is a negative lttng_error_code. */ - ret = -nb_fields; + if (ret_code != LTTNG_OK) { + ret = (int) ret_code; goto error; } - /* - * Setup lttng message with payload size set to the event list size in - * bytes and then copy list into the llm payload. - */ - ret = setup_lttng_msg_no_cmd_header(cmd_ctx, fields, - sizeof(struct lttng_event_field) * nb_fields); - free(fields); - - if (ret < 0) { - goto setup_error; - } + payload_size = cmd_ctx->reply_payload.buffer.size - + command_header_size - original_payload_size; + ERR("payload size = %i", (int) payload_size); + update_lttng_msg(cmd_ctx, command_header_size, payload_size); ret = LTTNG_OK; break; diff --git a/src/bin/lttng-sessiond/cmd.cpp b/src/bin/lttng-sessiond/cmd.cpp index f4ce9a882..0627ba5c8 100644 --- a/src/bin/lttng-sessiond/cmd.cpp +++ b/src/bin/lttng-sessiond/cmd.cpp @@ -2559,31 +2559,70 @@ error: /* * Command LTTNG_LIST_TRACEPOINT_FIELDS processed by the client thread. */ -ssize_t cmd_list_tracepoint_fields(enum lttng_domain_type domain, - struct lttng_event_field **fields) +enum lttng_error_code cmd_list_tracepoint_fields(enum lttng_domain_type domain, + struct lttng_payload *reply) { + enum lttng_error_code ret_code; int ret; - ssize_t nb_fields = 0; + unsigned int i, nb_fields; + struct lttng_event_field *fields = NULL; + struct lttcomm_list_command_header reply_command_header = {}; + size_t reply_command_header_offset; + + assert(reply); + + /* Reserve space for command reply header. */ + reply_command_header_offset = reply->buffer.size; + ret = lttng_dynamic_buffer_set_size(&reply->buffer, + reply_command_header_offset + + sizeof(struct lttcomm_list_command_header)); + if (ret) { + ret_code = LTTNG_ERR_NOMEM; + goto error; + } switch (domain) { case LTTNG_DOMAIN_UST: - nb_fields = ust_app_list_event_fields(fields); - if (nb_fields < 0) { - ret = LTTNG_ERR_UST_LIST_FAIL; + ret = ust_app_list_event_fields(&fields); + if (ret < 0) { + ret_code = LTTNG_ERR_UST_LIST_FAIL; goto error; } + break; case LTTNG_DOMAIN_KERNEL: default: /* fall-through */ - ret = LTTNG_ERR_UND; + ret_code = LTTNG_ERR_UND; goto error; } - return nb_fields; + nb_fields = ret; + + for (i = 0; i < nb_fields; i++) { + ret = lttng_event_field_serialize(&fields[i], reply); + if (ret) { + ret_code = LTTNG_ERR_NOMEM; + goto error; + } + } + + if (nb_fields > UINT32_MAX) { + ERR("Tracepoint field count would overflow the tracepoint field listing command's reply"); + ret_code = LTTNG_ERR_OVERFLOW; + goto error; + } + + /* Update command reply header. */ + reply_command_header.count = (uint32_t) nb_fields; + + memcpy(reply->buffer.data + reply_command_header_offset, &reply_command_header, + sizeof(reply_command_header)); + + ret_code = LTTNG_OK; error: - /* Return negative value to differentiate return code */ - return -ret; + free(fields); + return ret_code; } enum lttng_error_code cmd_list_syscalls( diff --git a/src/bin/lttng-sessiond/cmd.h b/src/bin/lttng-sessiond/cmd.h index 69389e5fa..bd2d93974 100644 --- a/src/bin/lttng-sessiond/cmd.h +++ b/src/bin/lttng-sessiond/cmd.h @@ -121,8 +121,8 @@ ssize_t cmd_list_domains(struct ltt_session *session, struct lttng_domain **domains); void cmd_list_lttng_sessions(struct lttng_session *sessions, size_t session_count, uid_t uid, gid_t gid); -ssize_t cmd_list_tracepoint_fields(enum lttng_domain_type domain, - struct lttng_event_field **fields); +enum lttng_error_code cmd_list_tracepoint_fields(enum lttng_domain_type domain, + struct lttng_payload *reply); enum lttng_error_code cmd_list_tracepoints(enum lttng_domain_type domain, struct lttng_payload *reply_payload); ssize_t cmd_snapshot_list_outputs(struct ltt_session *session, diff --git a/src/common/event.cpp b/src/common/event.cpp index 502ccdcf2..c3a2c0eb7 100644 --- a/src/common/event.cpp +++ b/src/common/event.cpp @@ -1278,6 +1278,192 @@ void lttng_event_context_destroy(struct lttng_event_context *context) free(context); } +/* + * This is a specialized populate for lttng_event_field since it ignores + * the extension field of the lttng_event struct and simply copies what it can + * to the internal struct lttng_event of a lttng_event_field. + */ +static void lttng_event_field_populate_lttng_event_from_event( + const struct lttng_event *src, struct lttng_event *destination) +{ + memcpy(destination, src, sizeof(*destination)); + + /* Remove all possible dynamic data from the destination event rule. */ + destination->extended.ptr = NULL; +} + +ssize_t lttng_event_field_create_from_payload( + struct lttng_payload_view *view, + struct lttng_event_field **field) +{ + ssize_t ret, offset = 0; + struct lttng_event_field *local_event_field = NULL; + struct lttng_event *event = NULL; + const struct lttng_event_field_comm *comm; + const char* name = NULL; + + assert(field); + assert(view); + + { + const struct lttng_buffer_view comm_view = + lttng_buffer_view_from_view( + &view->buffer, offset, + sizeof(*comm)); + + if (!lttng_buffer_view_is_valid(&comm_view)) { + ret = -1; + goto end; + } + + /* lttng_event_field_comm header */ + comm = (const lttng_event_field_comm *) comm_view.data; + offset += sizeof(*comm); + } + + local_event_field = (struct lttng_event_field *) + zmalloc(sizeof(*local_event_field)); + if (!local_event_field) { + ret = -1; + goto end; + } + + local_event_field->type = (lttng_event_field_type) comm->type; + local_event_field->nowrite = comm->nowrite; + + /* Field name */ + { + const struct lttng_buffer_view name_view = + lttng_buffer_view_from_view( + &view->buffer, offset, + comm->name_len); + + if (!lttng_buffer_view_is_valid(&name_view)) { + ret = -1; + goto end; + } + + name = name_view.data; + + if (!lttng_buffer_view_contains_string(&name_view, + name_view.data, comm->name_len)) { + ret = -1; + goto end; + } + + if (comm->name_len > LTTNG_SYMBOL_NAME_LEN - 1) { + /* Name is too long.*/ + ret = -1; + goto end; + } + + offset += comm->name_len; + } + + /* Event */ + { + struct lttng_payload_view event_view = + lttng_payload_view_from_view( + view, offset, + comm->event_len); + + if (!lttng_payload_view_is_valid(&event_view)) { + ret = -1; + goto end; + } + + ret = lttng_event_create_from_payload(&event_view, &event, NULL, + NULL, NULL); + if (ret != comm->event_len) { + ret = -1; + goto end; + } + + offset += ret; + } + + assert(name); + assert(event); + + if (lttng_strncpy(local_event_field->field_name, name , LTTNG_SYMBOL_NAME_LEN)) { + ret = -1; + goto end; + } + + lttng_event_field_populate_lttng_event_from_event( + event, &local_event_field->event); + + *field = local_event_field; + local_event_field = NULL; + ret = offset; +end: + lttng_event_destroy(event); + free(local_event_field); + return ret; +} + +int lttng_event_field_serialize(const struct lttng_event_field *field, + struct lttng_payload *payload) +{ + int ret; + size_t header_offset, size_before_event; + size_t name_len; + struct lttng_event_field_comm event_field_comm = { 0 }; + struct lttng_event_field_comm *header; + + assert(field); + assert(payload); + + /* Save the header location for later in-place header update. */ + header_offset = payload->buffer.size; + + name_len = strnlen(field->field_name, LTTNG_SYMBOL_NAME_LEN); + if (name_len == LTTNG_SYMBOL_NAME_LEN) { + /* Event name is not NULL-terminated. */ + ret = -1; + goto end; + } + + /* Add null termination. */ + name_len += 1; + + event_field_comm.type = field->type; + event_field_comm.nowrite = (uint8_t)field->nowrite; + event_field_comm.name_len = name_len; + + /* Header */ + ret = lttng_dynamic_buffer_append( + &payload->buffer, &event_field_comm, + sizeof(event_field_comm)); + if (ret) { + goto end; + } + + /* Field name */ + ret = lttng_dynamic_buffer_append(&payload->buffer, field->field_name, + name_len); + if (ret) { + goto end; + } + + size_before_event = payload->buffer.size; + ret = lttng_event_serialize( + &field->event, 0, NULL, NULL, 0, 0, payload); + if (ret) { + ret = -1; + goto end; + } + + /* Update the event len. */ + header = (struct lttng_event_field_comm *) + ((char *) payload->buffer.data + + header_offset); + header->event_len = payload->buffer.size - size_before_event; + +end: + return ret; +} + static enum lttng_error_code compute_flattened_size( struct lttng_dynamic_pointer_array *events, size_t *size) { @@ -1595,3 +1781,158 @@ end: lttng_dynamic_pointer_array_reset(&local_events); return ret; } + +static enum lttng_error_code flatten_lttng_event_fields( + struct lttng_dynamic_pointer_array *event_fields, + struct lttng_event_field **flattened_event_fields) +{ + int ret, i; + enum lttng_error_code ret_code; + size_t storage_req = 0; + struct lttng_dynamic_buffer local_flattened_event_fields; + int nb_event_field; + + assert(event_fields); + assert(flattened_event_fields); + + lttng_dynamic_buffer_init(&local_flattened_event_fields); + nb_event_field = lttng_dynamic_pointer_array_get_count(event_fields); + + /* + * Here even if the event field contains a `struct lttng_event` that + * could contain dynamic data, in reality it is not the case. + * Dynamic data is not present. Here the flattening is mostly a direct + * memcpy. This is less than ideal but this code is still better than + * direct usage of an unpacked lttng_event_field array. + */ + storage_req += sizeof(struct lttng_event_field) * nb_event_field; + + lttng_dynamic_buffer_init(&local_flattened_event_fields); + + /* + * We must ensure that "local_flattened_event_fields" is never resized + * so as to preserve the validity of the flattened objects. + */ + ret = lttng_dynamic_buffer_set_capacity( + &local_flattened_event_fields, storage_req); + if (ret) { + ret_code = LTTNG_ERR_NOMEM; + goto end; + } + + for (i = 0; i < nb_event_field; i++) { + const struct lttng_event_field *element = + (const struct lttng_event_field *) + lttng_dynamic_pointer_array_get_pointer( + event_fields, i); + + if (!element) { + ret_code = LTTNG_ERR_FATAL; + goto end; + } + ret = lttng_dynamic_buffer_append(&local_flattened_event_fields, + element, sizeof(struct lttng_event_field)); + if (ret) { + ret_code = LTTNG_ERR_NOMEM; + goto end; + } + } + + /* Don't reset local_flattened_channels buffer as we return its content. */ + *flattened_event_fields = (struct lttng_event_field *) local_flattened_event_fields.data; + lttng_dynamic_buffer_init(&local_flattened_event_fields); + ret_code = LTTNG_OK; +end: + lttng_dynamic_buffer_reset(&local_flattened_event_fields); + return ret_code; +} + +static enum lttng_error_code event_field_list_create_from_payload( + struct lttng_payload_view *view, + unsigned int count, + struct lttng_dynamic_pointer_array **event_field_list) +{ + enum lttng_error_code ret_code; + int ret, offset = 0; + unsigned int i; + struct lttng_dynamic_pointer_array *list = NULL; + + assert(view); + assert(event_field_list); + + list = (struct lttng_dynamic_pointer_array *) zmalloc(sizeof(*list)); + if (!list) { + ret_code = LTTNG_ERR_NOMEM; + goto end; + } + + lttng_dynamic_pointer_array_init(list, free); + + for (i = 0; i < count; i++) { + ssize_t event_field_size; + struct lttng_event_field *field = NULL; + struct lttng_payload_view event_field_view = + lttng_payload_view_from_view(view, offset, -1); + + event_field_size = lttng_event_field_create_from_payload( + &event_field_view, &field); + if (event_field_size < 0) { + ret_code = LTTNG_ERR_INVALID; + goto end; + } + + /* Lifetime and management of the object is now bound to the array. */ + ret = lttng_dynamic_pointer_array_add_pointer(list, field); + if (ret) { + free(field); + ret_code = LTTNG_ERR_NOMEM; + goto end; + } + + offset += event_field_size; + } + + if (view->buffer.size != offset) { + ret_code = LTTNG_ERR_INVALID; + goto end; + } + + *event_field_list = list; + list = NULL; + ret_code = LTTNG_OK; + +end: + if (list) { + lttng_dynamic_pointer_array_reset(list); + free(list); + } + + return ret_code; +} + +enum lttng_error_code lttng_event_fields_create_and_flatten_from_payload( + struct lttng_payload_view *view, + unsigned int count, + struct lttng_event_field **fields) +{ + enum lttng_error_code ret_code; + struct lttng_dynamic_pointer_array *local_event_fields = NULL; + + ret_code = event_field_list_create_from_payload( + view, count, &local_event_fields); + if (ret_code != LTTNG_OK) { + goto end; + } + + ret_code = flatten_lttng_event_fields(local_event_fields, fields); + if (ret_code != LTTNG_OK) { + goto end; + } +end: + if (local_event_fields) { + lttng_dynamic_pointer_array_reset(local_event_fields); + free(local_event_fields); + } + + return ret_code; +} diff --git a/src/lib/lttng-ctl/lttng-ctl.cpp b/src/lib/lttng-ctl/lttng-ctl.cpp index e671bae40..c7fb313f9 100644 --- a/src/lib/lttng-ctl/lttng-ctl.cpp +++ b/src/lib/lttng-ctl/lttng-ctl.cpp @@ -1715,23 +1715,74 @@ end: int lttng_list_tracepoint_fields(struct lttng_handle *handle, struct lttng_event_field **fields) { + enum lttng_error_code ret_code; int ret; struct lttcomm_session_msg lsm; + const struct lttcomm_list_command_header *cmd_header = NULL; + unsigned int nb_event_fields = 0; + struct lttng_payload reply; if (handle == NULL) { - return -LTTNG_ERR_INVALID; + ret = -LTTNG_ERR_INVALID; + goto end; } + lttng_payload_init(&reply); + memset(&lsm, 0, sizeof(lsm)); lsm.cmd_type = LTTNG_LIST_TRACEPOINT_FIELDS; COPY_DOMAIN_PACKED(lsm.domain, handle->domain); - ret = lttng_ctl_ask_sessiond(&lsm, (void **) fields); - if (ret < 0) { - return ret; + { + lttng_payload_view message_view = + lttng_payload_view_init_from_buffer( + (const char *) &lsm, 0, + sizeof(lsm)); + + ret = lttng_ctl_ask_sessiond_payload(&message_view, &reply); + if (ret < 0) { + goto end; + } + } + + { + const lttng_buffer_view cmd_header_view = + lttng_buffer_view_from_dynamic_buffer( + &reply.buffer, 0, sizeof(*cmd_header)); + + if (!lttng_buffer_view_is_valid(&cmd_header_view)) { + ret = -LTTNG_ERR_INVALID_PROTOCOL; + goto end; + } + + cmd_header = (struct lttcomm_list_command_header *) + cmd_header_view.data; + } + + if (cmd_header->count > INT_MAX) { + ret = -LTTNG_ERR_OVERFLOW; + goto end; + } + + nb_event_fields = cmd_header->count; + + { + lttng_payload_view reply_view = + lttng_payload_view_from_payload(&reply, + sizeof(*cmd_header), -1); + + ret_code = lttng_event_fields_create_and_flatten_from_payload( + &reply_view, nb_event_fields, fields); + if (ret_code != LTTNG_OK) { + ret = -ret_code; + goto end; + } } - return ret / sizeof(struct lttng_event_field); + ret = nb_event_fields; + +end: + return ret; } /* -- 2.34.1