From 1083b49aa3c4914193157ca25978c01f4f8bca4c Mon Sep 17 00:00:00 2001 From: =?utf8?q?J=C3=A9r=C3=A9mie=20Galarneau?= Date: Mon, 17 Jul 2023 15:48:51 -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 Change-Id: Id8090156646e4ef2011738623e0bfee1f400dced Signed-off-by: Jérémie Galarneau --- src/bin/lttng-sessiond/ust-app.c | 50 +++++++++++++------------- src/bin/lttng-sessiond/ust-app.h | 1 + src/common/event-rule/jul-logging.c | 2 +- src/common/event-rule/log4j-logging.c | 2 +- src/common/event-rule/python-logging.c | 2 +- 5 files changed, 30 insertions(+), 27 deletions(-) diff --git a/src/bin/lttng-sessiond/ust-app.c b/src/bin/lttng-sessiond/ust-app.c index e5d6857a3..163767d15 100644 --- a/src/bin/lttng-sessiond/ust-app.c +++ b/src/bin/lttng-sessiond/ust-app.c @@ -116,14 +116,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; assert(node); assert(_key); event = caa_container_of(node, struct ust_app_event, node.node); key = _key; - ev_loglevel_value = event->attr.loglevel; /* Match the 4 elements of the key: name, filter, loglevel, exclusions */ @@ -133,19 +131,10 @@ 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. */ @@ -202,7 +191,9 @@ static void add_unique_ust_app_event(struct ust_app_channel *ua_chan, ht = ua_chan->events; key.name = event->attr.name; key.filter = event->filter; - key.loglevel_type = event->attr.loglevel; + key.loglevel_type = (enum 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,7 +1490,9 @@ error: * Return an ust_app_event object or NULL on error. */ static struct ust_app_event *find_ust_app_event(struct lttng_ht *ht, - const char *name, const struct lttng_bytecode *filter, + 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) { @@ -1514,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 = 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; @@ -4841,9 +4835,11 @@ int ust_app_disable_event_glb(struct ltt_ust_session *usess, } ua_chan = caa_container_of(ua_chan_node, struct 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 == NULL) { DBG2("Event %s not found in channel %s for app pid %d." "Skipping", uevent->attr.name, uchan->name, app->pid); @@ -5001,8 +4997,11 @@ int ust_app_enable_event_glb(struct ltt_ust_session *usess, ua_chan = caa_container_of(ua_chan_node, struct 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 == NULL) { DBG3("UST app enable event %s not found for app PID %d." "Skipping app", uevent->attr.name, app->pid); @@ -5776,7 +5775,10 @@ int ust_app_channel_synchronize_event(struct ust_app_channel *ua_chan, struct ust_app_event *ua_event = NULL; ua_event = find_ust_app_event(ua_chan->events, uevent->attr.name, - uevent->filter, uevent->attr.loglevel, uevent->exclusion); + uevent->filter, + (enum lttng_ust_abi_loglevel_type) + uevent->attr.loglevel_type, + uevent->attr.loglevel, uevent->exclusion); if (!ua_event) { ret = create_ust_app_event(ua_sess, ua_chan, uevent, app); if (ret < 0) { diff --git a/src/bin/lttng-sessiond/ust-app.h b/src/bin/lttng-sessiond/ust-app.h index 4a7a7c92d..568bd09e4 100644 --- a/src/bin/lttng-sessiond/ust-app.h +++ b/src/bin/lttng-sessiond/ust-app.h @@ -42,6 +42,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.c b/src/common/event-rule/jul-logging.c index 5f69f84c7..ddaa7ba69 100644 --- a/src/common/event-rule/jul-logging.c +++ b/src/common/event-rule/jul-logging.c @@ -442,7 +442,7 @@ static struct lttng_event *lttng_event_rule_jul_logging_generate_lttng_event( 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.c b/src/common/event-rule/log4j-logging.c index 755ae26c6..b4d67667d 100644 --- a/src/common/event-rule/log4j-logging.c +++ b/src/common/event-rule/log4j-logging.c @@ -442,7 +442,7 @@ static struct lttng_event *lttng_event_rule_log4j_logging_generate_lttng_event( 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.c b/src/common/event-rule/python-logging.c index 4b5cebaf0..171de4647 100644 --- a/src/common/event-rule/python-logging.c +++ b/src/common/event-rule/python-logging.c @@ -442,7 +442,7 @@ static struct lttng_event *lttng_event_rule_python_logging_generate_lttng_event( 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; -- 2.34.1