From b0a79813263e65e177c2cca7a352c8b364a02b22 Mon Sep 17 00:00:00 2001 From: Francis Deslauriers Date: Wed, 14 Apr 2021 21:19:41 -0400 Subject: [PATCH] Fix: lttng-ctl: appending to dynamic buffer invalidates its data member MIME-Version: 1.0 Content-Type: text/plain; charset=utf8 Content-Transfer-Encoding: 8bit Issue ===== The following commands fail: lttng add-trigger --id T0 --condition on-event -u some-event --action snapshot-session ze-session3 --path /some/path lttng remove-trigger T0 Error: Attempt to create buffer view from another view with invalid length (length > space left after offset in source): source size = 0, offset in source = 0, length = 25 Error: Invalid trigger received as part of command payload Valgrind complains in the following way: ==706109== ==706109== Invalid write of size 4 ==706109== at 0x489FED7: lttng_unregister_trigger (lttng-ctl.c:3281) ==706109== by 0x43C175: cmd_remove_trigger (remove_trigger.c:171) ==706109== by 0x43F56B: handle_command (lttng.c:237) ==706109== by 0x43E9B1: parse_args (lttng.c:421) ==706109== by 0x43E158: main (lttng.c:470) ==706109== Address 0x73d8d20 is 4,688 bytes inside a block of size 16,384 free'd ==706109== at 0x483DFAF: realloc (in /usr/lib/x86_64-linux-gnu/valgrind/vgpreload_memcheck-amd64-linux.so) ==706109== by 0x48C1478: lttng_dynamic_buffer_set_capacity (dynamic-buffer.c:166) ==706109== by 0x48C138C: lttng_dynamic_buffer_append (dynamic-buffer.c:55) ==706109== by 0x48E3325: lttng_snapshot_output_serialize (snapshot.c:120) ==706109== by 0x48B46C3: lttng_action_snapshot_session_serialize (snapshot-session.c:173) ==706109== by 0x48B1FB2: lttng_action_serialize (action.c:130) ==706109== by 0x48B2DFE: lttng_action_group_serialize (group.c:165) ==706109== by 0x48B1FB2: lttng_action_serialize (action.c:130) ==706109== by 0x48ECE66: lttng_trigger_serialize (trigger.c:372) ==706109== by 0x489FEA0: lttng_unregister_trigger (lttng-ctl.c:3275) ==706109== by 0x43C175: cmd_remove_trigger (remove_trigger.c:171) ==706109== by 0x43F56B: handle_command (lttng.c:237) ==706109== Block was alloc'd at ==706109== at 0x483B723: malloc (in /usr/lib/x86_64-linux-gnu/valgrind/vgpreload_memcheck-amd64-linux.so) ==706109== by 0x483E017: realloc (in /usr/lib/x86_64-linux-gnu/valgrind/vgpreload_memcheck-amd64-linux.so) ==706109== by 0x48C1478: lttng_dynamic_buffer_set_capacity (dynamic-buffer.c:166) ==706109== by 0x48C138C: lttng_dynamic_buffer_append (dynamic-buffer.c:55) ==706109== by 0x489FE66: lttng_unregister_trigger (lttng-ctl.c:3263) ==706109== by 0x43C175: cmd_remove_trigger (remove_trigger.c:171) ==706109== by 0x43F56B: handle_command (lttng.c:237) ==706109== by 0x43E9B1: parse_args (lttng.c:421) ==706109== by 0x43E158: main (lttng.c:470) `lttng_unregister_trigger` samples the address of the lsm header in the message payload. However, it does so before calling `lttng_trigger_serialize()` which may increase the underlying buffer's size (and cause a realloc()). Most of the time the message buffer is large enough _or_ its realloc yields the same address which hid the problem. However, I stumbled on a case (a trigger which snapshots to a location) where the realloc ends-up returning a completely different address, causing invalid data to be sent to the session daemon. Solution ======== Sample the lsm header address after the serialization of the trigger. Note ==== An identical fix was done for the `lttng_register_trigger` function in: commit b22f4f54e95ae13edda1d4d5efd1e4845a6319c4 Author: Jérémie Galarneau Date: Thu Feb 18 18:13:19 2021 -0500 Fix: lttng-ctl: appending to dynamic buffer invalidates its data member I reuse the bug explanation for this commit message. Signed-off-by: Francis Deslauriers Signed-off-by: Jérémie Galarneau Change-Id: Ic50c96dcada9e0595b0fab1d2f357c183b53e1de --- src/lib/lttng-ctl/lttng-ctl.c | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/src/lib/lttng-ctl/lttng-ctl.c b/src/lib/lttng-ctl/lttng-ctl.c index 7debaed6b..7efc6bd2e 100644 --- a/src/lib/lttng-ctl/lttng-ctl.c +++ b/src/lib/lttng-ctl/lttng-ctl.c @@ -3344,18 +3344,18 @@ int lttng_unregister_trigger(const struct lttng_trigger *trigger) goto end; } - /* - * This is needed to populate the trigger object size for the command - * header and number of fds sent. - */ - message_lsm = (struct lttcomm_session_msg *) message.buffer.data; - ret = lttng_trigger_serialize(copy, &message); if (ret < 0) { ret = -LTTNG_ERR_UNK; goto end; } + /* + * This is needed to populate the trigger object size for the command + * header and number of fds sent. + */ + message_lsm = (struct lttcomm_session_msg *) message.buffer.data; + message_lsm->u.trigger.length = (uint32_t) message.buffer.size - sizeof(lsm); { -- 2.34.1