From dcd24bbf7dbc74e3584d1d0d52715e749023c452 Mon Sep 17 00:00:00 2001 From: =?utf8?q?J=C3=A9r=C3=A9mie=20Galarneau?= Date: Wed, 14 Jun 2023 19:03:12 -0400 Subject: [PATCH] Fix: sessiond: crash enabling event rules that differ only by loglevel type MIME-Version: 1.0 Content-Type: text/plain; charset=utf8 Content-Transfer-Encoding: 8bit Issue observed -------------- Summarizing bug #1373, an assertion fails when enabling two event-rules that only differ by their log level selection type (all, range, or single). This issue can be reproduced by launching an instrumented application (which remains active over the duration of this test) and running: $ lttng create test_session $ lttng enable-channel --userspace test_channel $ lttng start test_session $ lttng enable-event --userspace --session test_session --channel test_channel 'l*' --loglevel-only=TRACE_DEBUG_LINE $ lttng enable-event --userspace --session test_session --channel test_channel 'l*' --loglevel=TRACE_DEBUG_LINE Cause ----- A number of sites conflate log level values and type. A clean-up had been performed previously as of 2106efa08 and through follow-up commits. However, some instances were apparently missed at the time. As such, add_unique_ust_app_event mixed loglevel values and types when initializing the key used for the unique insertion. The log level type, for its part, is completely ignored. Going back to the reproducer above, the first insertion succeeds as expected. The second insertion fails since there is already an app event rule with the `TRACE_DEBUG_LINE` log level type. Moreover, the matching function only matches on the log level type (which is really the value), which is also a bug. The "matching" function is invoked on the key of the second event rule and the first event rule since the hashing is only performed on the event-rule's name pattern, resulting in a collision. Solution -------- Both the log level value and log level types are used to perform the matching within the ust-app module. This implies extending the ust_app_ht_key to store the log level value. To simplify the matching logic (which attempted to handle 0 and -1 having the same meaning when the "ALL" log level type was used), the log level value is normalized to '-1' throughout. Fixes #1373 Reported-by: Bogdan Codres Signed-off-by: Jérémie Galarneau Change-Id: I869a0fb7a6554da7d84bc71df6ee91a7e46937cc --- src/bin/lttng-sessiond/cmd.cpp | 7 ++++ src/bin/lttng-sessiond/ust-app.cpp | 52 ++++++++++++------------ src/bin/lttng-sessiond/ust-app.hpp | 1 + src/common/event-rule/jul-logging.cpp | 2 +- src/common/event-rule/log4j-logging.cpp | 2 +- src/common/event-rule/python-logging.cpp | 2 +- src/common/event.cpp | 3 +- 7 files changed, 39 insertions(+), 30 deletions(-) diff --git a/src/bin/lttng-sessiond/cmd.cpp b/src/bin/lttng-sessiond/cmd.cpp index c9924b326..60ae7fe6a 100644 --- a/src/bin/lttng-sessiond/cmd.cpp +++ b/src/bin/lttng-sessiond/cmd.cpp @@ -2095,6 +2095,12 @@ static int _cmd_enable_event(struct ltt_session *session, } } + /* Normalize loglevel value to simplify comparisons. */ + if (event->loglevel_type == LTTNG_EVENT_LOGLEVEL_ALL) { + /* Ignore the user-specified value; it has no meaning. */ + event->loglevel = -1; + } + DBG("Enable event command for event \'%s\'", event->name); lttng::urcu::read_lock_guard read_lock; @@ -2338,6 +2344,7 @@ static int _cmd_enable_event(struct ltt_session *session, memset(&uevent, 0, sizeof(uevent)); uevent.type = LTTNG_EVENT_TRACEPOINT; uevent.loglevel_type = LTTNG_EVENT_LOGLEVEL_ALL; + uevent.loglevel = -1; default_event_name = event_get_default_agent_ust_name(domain->type); if (!default_event_name) { ret = LTTNG_ERR_FATAL; diff --git a/src/bin/lttng-sessiond/ust-app.cpp b/src/bin/lttng-sessiond/ust-app.cpp index c985fd8ac..202e2939f 100644 --- a/src/bin/lttng-sessiond/ust-app.cpp +++ b/src/bin/lttng-sessiond/ust-app.cpp @@ -180,14 +180,12 @@ static int ht_match_ust_app_event(struct cds_lfht_node *node, const void *_key) { struct ust_app_event *event; const struct ust_app_ht_key *key; - int ev_loglevel_value; LTTNG_ASSERT(node); LTTNG_ASSERT(_key); event = caa_container_of(node, struct ust_app_event, node.node); key = (ust_app_ht_key *) _key; - ev_loglevel_value = event->attr.loglevel; /* Match the 4 elements of the key: name, filter, loglevel, exclusions */ @@ -197,18 +195,12 @@ static int ht_match_ust_app_event(struct cds_lfht_node *node, const void *_key) } /* Event loglevel. */ - if (ev_loglevel_value != key->loglevel_type) { - if (event->attr.loglevel_type == LTTNG_UST_ABI_LOGLEVEL_ALL && - key->loglevel_type == 0 && ev_loglevel_value == -1) { - /* - * Match is accepted. This is because on event creation, the - * loglevel is set to -1 if the event loglevel type is ALL so 0 and - * -1 are accepted for this loglevel type since 0 is the one set by - * the API when receiving an enable event. - */ - } else { - goto no_match; - } + if (!loglevels_match(event->attr.loglevel_type, + event->attr.loglevel, + key->loglevel_type, + key->loglevel_value, + LTTNG_UST_ABI_LOGLEVEL_ALL)) { + goto no_match; } /* One of the filters is NULL, fail. */ @@ -263,7 +255,8 @@ static void add_unique_ust_app_event(struct ust_app_channel *ua_chan, struct ust ht = ua_chan->events; key.name = event->attr.name; key.filter = event->filter; - key.loglevel_type = (lttng_ust_abi_loglevel_type) event->attr.loglevel; + key.loglevel_type = (lttng_ust_abi_loglevel_type) event->attr.loglevel_type; + key.loglevel_value = event->attr.loglevel; key.exclusion = event->exclusion; node_ptr = cds_lfht_add_unique(ht->ht, @@ -1499,6 +1492,7 @@ error: static struct ust_app_event *find_ust_app_event(struct lttng_ht *ht, const char *name, const struct lttng_bytecode *filter, + lttng_ust_abi_loglevel_type loglevel_type, int loglevel_value, const struct lttng_event_exclusion *exclusion) { @@ -1513,7 +1507,8 @@ static struct ust_app_event *find_ust_app_event(struct lttng_ht *ht, /* Setup key for event lookup. */ key.name = name; key.filter = filter; - key.loglevel_type = (lttng_ust_abi_loglevel_type) loglevel_value; + key.loglevel_type = loglevel_type; + key.loglevel_value = loglevel_value; /* lttng_event_exclusion and lttng_ust_event_exclusion structures are similar */ key.exclusion = exclusion; @@ -5001,11 +4996,13 @@ int ust_app_disable_event_glb(struct ltt_ust_session *usess, } ua_chan = lttng::utils::container_of(ua_chan_node, &ust_app_channel::node); - ua_event = find_ust_app_event(ua_chan->events, - uevent->attr.name, - uevent->filter, - uevent->attr.loglevel, - uevent->exclusion); + ua_event = find_ust_app_event( + ua_chan->events, + uevent->attr.name, + uevent->filter, + (enum lttng_ust_abi_loglevel_type) uevent->attr.loglevel_type, + uevent->attr.loglevel, + uevent->exclusion); if (ua_event == nullptr) { DBG2("Event %s not found in channel %s for app pid %d." "Skipping", @@ -5164,11 +5161,13 @@ int ust_app_enable_event_glb(struct ltt_ust_session *usess, ua_chan = lttng::utils::container_of(ua_chan_node, &ust_app_channel::node); /* Get event node */ - ua_event = find_ust_app_event(ua_chan->events, - uevent->attr.name, - uevent->filter, - uevent->attr.loglevel, - uevent->exclusion); + ua_event = find_ust_app_event( + ua_chan->events, + uevent->attr.name, + uevent->filter, + (enum lttng_ust_abi_loglevel_type) uevent->attr.loglevel_type, + uevent->attr.loglevel, + uevent->exclusion); if (ua_event == nullptr) { DBG3("UST app enable event %s not found for app PID %d." "Skipping app", @@ -5952,6 +5951,7 @@ static int ust_app_channel_synchronize_event(struct ust_app_channel *ua_chan, ua_event = find_ust_app_event(ua_chan->events, uevent->attr.name, uevent->filter, + (enum lttng_ust_abi_loglevel_type) uevent->attr.loglevel_type, uevent->attr.loglevel, uevent->exclusion); if (!ua_event) { diff --git a/src/bin/lttng-sessiond/ust-app.hpp b/src/bin/lttng-sessiond/ust-app.hpp index fdc007853..67af3ec61 100644 --- a/src/bin/lttng-sessiond/ust-app.hpp +++ b/src/bin/lttng-sessiond/ust-app.hpp @@ -45,6 +45,7 @@ struct ust_app_ht_key { const char *name; const struct lttng_bytecode *filter; enum lttng_ust_abi_loglevel_type loglevel_type; + int loglevel_value; const struct lttng_event_exclusion *exclusion; }; diff --git a/src/common/event-rule/jul-logging.cpp b/src/common/event-rule/jul-logging.cpp index 3db273744..95b953e17 100644 --- a/src/common/event-rule/jul-logging.cpp +++ b/src/common/event-rule/jul-logging.cpp @@ -413,7 +413,7 @@ lttng_event_rule_jul_logging_generate_lttng_event(const struct lttng_event_rule status = lttng_event_rule_jul_logging_get_log_level_rule(rule, &log_level_rule); if (status == LTTNG_EVENT_RULE_STATUS_UNSET) { loglevel_type = LTTNG_EVENT_LOGLEVEL_ALL; - loglevel_value = 0; + loglevel_value = -1; } else if (status == LTTNG_EVENT_RULE_STATUS_OK) { enum lttng_log_level_rule_status llr_status; diff --git a/src/common/event-rule/log4j-logging.cpp b/src/common/event-rule/log4j-logging.cpp index 5655cd3bc..a3184d6d0 100644 --- a/src/common/event-rule/log4j-logging.cpp +++ b/src/common/event-rule/log4j-logging.cpp @@ -413,7 +413,7 @@ lttng_event_rule_log4j_logging_generate_lttng_event(const struct lttng_event_rul status = lttng_event_rule_log4j_logging_get_log_level_rule(rule, &log_level_rule); if (status == LTTNG_EVENT_RULE_STATUS_UNSET) { loglevel_type = LTTNG_EVENT_LOGLEVEL_ALL; - loglevel_value = 0; + loglevel_value = -1; } else if (status == LTTNG_EVENT_RULE_STATUS_OK) { enum lttng_log_level_rule_status llr_status; diff --git a/src/common/event-rule/python-logging.cpp b/src/common/event-rule/python-logging.cpp index eb23b337b..2a5703da2 100644 --- a/src/common/event-rule/python-logging.cpp +++ b/src/common/event-rule/python-logging.cpp @@ -413,7 +413,7 @@ lttng_event_rule_python_logging_generate_lttng_event(const struct lttng_event_ru status = lttng_event_rule_python_logging_get_log_level_rule(rule, &log_level_rule); if (status == LTTNG_EVENT_RULE_STATUS_UNSET) { loglevel_type = LTTNG_EVENT_LOGLEVEL_ALL; - loglevel_value = 0; + loglevel_value = -1; } else if (status == LTTNG_EVENT_RULE_STATUS_OK) { enum lttng_log_level_rule_status llr_status; diff --git a/src/common/event.cpp b/src/common/event.cpp index a6d48eaf7..d989bfc7c 100644 --- a/src/common/event.cpp +++ b/src/common/event.cpp @@ -367,7 +367,8 @@ ssize_t lttng_event_create_from_payload(struct lttng_payload_view *view, local_event->type = (enum lttng_event_type) event_comm->event_type; local_event->loglevel_type = (enum lttng_loglevel_type) event_comm->loglevel_type; - local_event->loglevel = event_comm->loglevel; + local_event->loglevel = + local_event->loglevel_type == LTTNG_EVENT_LOGLEVEL_ALL ? -1 : event_comm->loglevel; local_event->enabled = !!event_comm->enabled; local_event->pid = event_comm->pid; local_event->flags = (enum lttng_event_flag) event_comm->flags; -- 2.34.1