Fix: sessiond: crash enabling event rules that differ only by loglevel type

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 <bogdan.codres@windriver.com>
Signed-off-by: Jérémie Galarneau <jeremie.galarneau@efficios.com>
Change-Id: I869a0fb7a6554da7d84bc71df6ee91a7e46937cc

diff --git a/src/bin/lttng-sessiond/cmd.cpp b/src/bin/lttng-sessiond/cmd.cpp
index c9924b326190dd4e765ff81af7175bf00a0b1410..60ae7fe6aa7632057962a1172fc2b571032bc075 100644 (file)
--- 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 c985fd8ac9ab5a0d36cb3a008366f788eb3fed2a..202e2939f183402b7917d19803fa3c01a41879f9 100644 (file)
--- 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 fdc00785335322d5c73c0f18d215d17296011233..67af3ec6191a6efc1a9a6df9343a53b5a621ddb3 100644 (file)
--- 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 3db273744d18614ff109ca7d35336f6ec15d40de..95b953e17bda9697d19418f8892f53c42545f27a 100644 (file)
--- 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 5655cd3bcc8fec6d0722bc166c50fe8c8f246f9d..a3184d6d06a3b1ada2f03a646afcf36a41fed700 100644 (file)
--- 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 eb23b337bfd2c365f90e0fa84b1e8ed81ae020e7..2a5703da2ab22336a8a557f4be323d33dfcd7860 100644 (file)
--- 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 a6d48eaf7f80e2001226aba7467394e34793183b..d989bfc7c979c1e71c6c81113eb7b1d6036272d9 100644 (file)
--- 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;