Release v1.13.0

This release introduces some changes in behavior for the crash probe and
journal probe:

* The crash probe now ensures core files are kept whenever backtraces
  are scrubbed from telemetry records, or when errors occur during runtime
  operation. This improves the developer experience when running ones own
  programs installed at arbitrary locations on the filesystem and also
  avoids data loss; either the crash backtrace is sent via telemetry, or
  when unable to send the backtrace, the core file is kept.

* The journal probe, by default, now only sends telemetry for journal
  log messages indicating systemd service failures. The previous behavior
  of sending any log messages above a certain log level is now wrapped
  behind the "opt-in-no-privacy-filters" config setting.

Bug fixes include:

* Fixing a segfault crash in the configuration code that occurred
  whenever an incomplete configuration file was encountered.

* Fixing a performance issue in 'telem-record-gen' to avoid extraneous
  strlen() calls in the classification input validation code.

Signed-off-by: Patrick McCarty <patrick.mccarty@intel.com>

configure.ac

@@ -2,7 +2,7 @@
 # Process this file with autoconf to produce a configure script.
 
 AC_PREREQ([2.69])
-AC_INIT([telemetrics-client], [1.12.2], [https://clearlinux.org/])
+AC_INIT([telemetrics-client], [1.13.0], [https://clearlinux.org/])
 AC_CONFIG_AUX_DIR([build-aux])
 AM_INIT_AUTOMAKE([1.14 -Wall -Werror -Wno-extra-portability foreign subdir-objects])
 AM_SILENT_RULES([yes])

src/configuration.c

@@ -97,8 +97,15 @@ bool read_config_from_file(char *config_file, struct configuration *config)
                 return false;
         } else {
                 for (int i = CONF_STR_MIN + 1; i < CONF_STR_MAX; i++) {
-                        config->strValues[i] = strdup(nc_hashmap_get(nc_hashmap_get(keyfile, "settings"), config_key_str[i]));
-                        if (config->strValues[i] == NULL) {
+                        char *ptr;
+                        ptr = nc_hashmap_get(nc_hashmap_get(keyfile, "settings"), config_key_str[i]);
+                        if (ptr) {
+                                config->strValues[i] = strdup(ptr);
+                                if (config->strValues[i] == NULL) {
+                                        return false;
+                                }
+                        } else {
+                                fprintf(stderr, "ERR: missing key with string value: %s\n", config_key_str[i]);
                                 return false;
                         }
                 }
@@ -108,6 +115,7 @@ bool read_config_from_file(char *config_file, struct configuration *config)
                         if (ptr) {
                                 config->intValues[i] = strtoll(ptr, NULL, 10);
                         } else {
+                                fprintf(stderr, "ERR: missing key with integer value: %s\n", config_key_int[i]);
                                 return false;
                         }
                 }
@@ -125,6 +133,7 @@ bool read_config_from_file(char *config_file, struct configuration *config)
                                         config->boolValues[i] = true;
                                 }
                         } else {
+                                fprintf(stderr, "ERR: missing key with boolean value: %s\n", config_key_bool[i]);
                                 return false;
                         }
                 }

src/probes/crash_probe.c

@@ -68,6 +68,10 @@ static char clr_build_class[40] = "org.clearlinux/crash/clr-build";
 static char error_class[30] = "org.clearlinux/crash/error";
 static char unknown_class[30] = "org.clearlinux/crash/unknown";
 
+static char temp_core[] = "/tmp/corefile-XXXXXX";
+static bool keep_core = false;
+
+
 static const Dwfl_Callbacks cb =
 {
         .find_elf = dwfl_build_id_find_elf,
@@ -147,23 +151,14 @@ static int temp_core_file(void)
         int tmp;
         ssize_t ret;
 
-        char core[PATH_MAX] = "/tmp/corefile-XXXXXX";
-
         /* mkstemp() opens the file with O_EXCL and 0600 permissions, so no need
          * to change umask or manipulate the fd to meet those requirements.
          */
-        if ((tmp = mkstemp(core)) < 0) {
+        if ((tmp = mkstemp(temp_core)) < 0) {
                 telem_perror("Failed to create temp core file");
                 return -1;
         }
 
-#ifndef DEBUG
-        if (unlink(core) < 0) {
-                telem_perror("Failed to unlink temp core file");
-                return -1;
-        }
-#endif
-
         while (true) {
                 // Use Linux-specific splice(2) here;
                 // simplifies copying data from pipe->file
@@ -416,6 +411,18 @@ fail:
         return -1;
 }
 
+static bool startswith(const char *full, const char *prefix)
+{
+        while (*prefix) {
+                if (*prefix != *full) {
+                        return false;
+                }
+                full++;
+                prefix++;
+        }
+        return true;
+}
+
 static bool in_clr_build(char *fullpath)
 {
         // Global override for privacy filters
@@ -427,8 +434,8 @@ static bool in_clr_build(char *fullpath)
          * The build environment for Clear Linux packages is set up by 'mock',
          * and the chroot in which rpmbuild builds the packages has this prefix.
          */
-        if ((strstr(fullpath, "/builddir/build/BUILD/")) ||
-            (strstr(fullpath, "!builddir!build!BUILD!"))) {
+        if (startswith(fullpath, "/builddir/build/BUILD/") ||
+            startswith(fullpath, "!builddir!build!BUILD!")) {
                 return true;
         }
 
@@ -444,13 +451,13 @@ static bool is_banned_path(char *fullpath)
 
         // Anything outside of /usr/, or in /usr/local/, we consider third-party
 
-        if ((strncmp(fullpath, "/usr/", 5) != 0) &&
-            (strncmp(fullpath, "!usr!", 5) != 0)) {
+        if (!startswith(fullpath, "/usr/") &&
+            !startswith(fullpath, "!usr!")) {
                 return true;
         }
 
-        if ((strncmp(fullpath, "/usr/local/", 11) == 0) ||
-            (strncmp(fullpath, "!usr!local!", 11) == 0)) {
+        if (startswith(fullpath, "/usr/local/") ||
+            startswith(fullpath, "!usr!local!")) {
                 return true;
         }
 
@@ -563,28 +570,6 @@ int main(int argc, char **argv)
                 exit(EXIT_FAILURE);
         }
 
-        if (proc_path && in_clr_build(proc_path)) {
-                telem_log(LOG_NOTICE, "Ignoring core (from mock build)\n");
-
-                backtrace = nc_string_dup("Crash from Clear package build\n");
-
-                if (!send_data(&backtrace, unknown_severity, clr_build_class)) {
-                        goto fail;
-                }
-                goto success;
-        }
-
-        if (proc_path && is_banned_path(proc_path)) {
-                telem_log(LOG_NOTICE, "Ignoring core (third-party binary)\n");
-
-                backtrace = nc_string_dup("Crash from third party\n");
-
-                if (!send_data(&backtrace, unknown_severity, unknown_class)) {
-                        goto fail;
-                }
-                goto success;
-        }
-
         if (core_file) {
                 core_fd = open(core_file, O_RDONLY);
                 if (core_fd == -1) {
@@ -617,6 +602,32 @@ int main(int argc, char **argv)
                 }
         }
 
+        if (proc_path && in_clr_build(proc_path)) {
+                telem_log(LOG_NOTICE, "Ignoring core (from mock build)\n");
+
+                backtrace = nc_string_dup("Crash from Clear package build\n");
+
+                keep_core = true;
+
+                if (!send_data(&backtrace, unknown_severity, clr_build_class)) {
+                        goto fail;
+                }
+                goto success;
+        }
+
+        if (proc_path && is_banned_path(proc_path)) {
+                telem_log(LOG_NOTICE, "Ignoring core (third-party binary)\n");
+
+                backtrace = nc_string_dup("Crash from third party\n");
+
+                keep_core = true;
+
+                if (!send_data(&backtrace, unknown_severity, unknown_class)) {
+                        goto fail;
+                }
+                goto success;
+        }
+
         elf_version(EV_CURRENT);
 
         if (prepare_corefile(&e_core, core_fd) < 0) {
@@ -667,6 +678,11 @@ success:
 
         ret = EXIT_SUCCESS;
 fail:
+        // Do not remove the core file if any errors occur
+        if (ret == EXIT_FAILURE) {
+                keep_core = true;
+        }
+
         free(core_file);
         free(proc_name);
         free(proc_path);
@@ -695,6 +711,12 @@ fail:
                 close(core_fd);
         }
 
+        // Remove the core file by default, except when the --core-file option
+        // is specified, or when keep_core is overridden to true.
+        if (!core_file && !keep_core) {
+                unlink(temp_core);
+        }
+
         return ret;
 }
 

src/probes/journal.c

@@ -38,6 +38,7 @@
 #include "config.h"
 #include "log.h"
 #include "telemetry.h"
+#include "probe.h"
 #include "nica/nc-string.h"
 #define BOOT_ID_LEN 33
 
@@ -227,9 +228,14 @@ static bool add_filters(sd_journal *journal)
         /* The semantics of how journal entry matching works is described in
          * detail in sd_journal_add_match(3).
          *
-         * The matches declared here correspond to the logical expression:
+         * When the privacy filter override is enabled, the matches declared
+         * here correspond to this logical expression:
          *
-         * BOOTID && ((P0 || P1 || P2 || P3) || EXITED)
+         *   BOOTID && ((P0 || P1 || P2 || P3) || EXITED)
+         *
+         * Otherwise, the expression is:
+         *
+         *   BOOTID && EXITED
          *
          * BOOTID is short for _BOOT_ID=VAL, where VAL is the boot ID for the
          * current boot. P0, P1, etc stand for PRIORITY=0, etc. And EXITED is
@@ -239,12 +245,20 @@ static bool add_filters(sd_journal *journal)
         JOURNAL_MATCH(data);
         free(data);
         JOURNAL_AND;
-        // The four highest log levels, all indicating errors
-        JOURNAL_MATCH("PRIORITY=0");
-        JOURNAL_MATCH("PRIORITY=1");
-        JOURNAL_MATCH("PRIORITY=2");
-        JOURNAL_MATCH("PRIORITY=3");
-        JOURNAL_OR;
+
+        // Filter messages with the four highest log levels, all indicating
+        // errors, but only when the privacy filters override is in effect;
+        // because the log messages contain arbitrary strings, and this probe
+        // does not yet keep a whitelist of allowed patterns or a blacklist of
+        // banned patterns.
+        if (access(TM_PRIVACY_FILTERS_OVERRIDE, F_OK) == 0) {
+                JOURNAL_MATCH("PRIORITY=0");
+                JOURNAL_MATCH("PRIORITY=1");
+                JOURNAL_MATCH("PRIORITY=2");
+                JOURNAL_MATCH("PRIORITY=3");
+                JOURNAL_OR;
+        }
+
         // Only set for service-level error conditions
         JOURNAL_MATCH("EXIT_CODE=exited");
 

src/probes/klog_main.c

@@ -23,7 +23,6 @@ int main(void)
         char *bufp = NULL;
         size_t buflen = 0;
         int bytes = 0;
-        int loopcount = 0;
 
         oops_parser_init(write_oops_to_file);
 
@@ -45,13 +44,13 @@ int main(void)
                 // Gets the size of the kernel ring buffer
                 log_size = klogctl(SYSLOG_ACTION_SIZE_BUFFER, NULL, 0);
                 if (log_size < 0) {
-                        telem_log(LOG_ERR, "Cannot read size of kernel ring buffer\n");
+                        telem_log(LOG_ERR, "Cannot read size of kernel ring buffer: %s\n", strerror(errno));
                         return 1;
                 }
 
                 buflen = (size_t)log_size;
 
-                if (loopcount++ > 0 && buflen > MAX_BUF) {
+                if (buflen > MAX_BUF) {
                         buflen = MAX_BUF;
                 }
 
@@ -59,7 +58,7 @@ int main(void)
                 bufp = (char *)calloc(buflen, sizeof(char));
 
                 malloc_trim(0);
-                bytes = klogctl(SYSLOG_ACTION_READ, bufp, log_size);
+                bytes = klogctl(SYSLOG_ACTION_READ, bufp, (int)buflen);
                 if (bytes < 0) {
                         telem_log(LOG_ERR, "Cannot read contents of kernel ring buffer: %s\n", strerror(errno));
                         return 1;

src/probes/telem_record_gen.c

@@ -150,7 +150,7 @@ fail:
 
 int validate_opts(void)
 {
-        size_t i;
+        size_t len;
         int ret = 0;
 
         /* classification */
@@ -159,15 +159,15 @@ int validate_opts(void)
                 return ret;
 
         }
-        i = strlen(opt_class);
+        len = strlen(opt_class);
 
-        if ((i == 0) || (i > 120)) {
+        if ((len == 0) || (len > 120)) {
                 fprintf(stderr, "Error: Valid size for classification "
                         "is 1-120 chars\n");
                 return ret;
         }
 
-        for (int c = 0; c < strlen(opt_class); c++) {
+        for (int c = 0; c < len; c++) {
                 if (isascii(opt_class[c]) == 0) {
                         fprintf(stderr, "Error: Non-ascii characters detected "
                                 "in classification - aborting\n");

src/telemdaemon.c

@@ -462,6 +462,11 @@ bool post_record_http(char *headers[], char *body, bool spool)
         const char *cert_file = get_cainfo_config();
         const char *tid_header = get_tidheader_config();
 
+        // Initialize the libcurl global environment once per POST. This lets us
+        // clean up the environment after each POST so that when the daemon is
+        // sitting idle, it will be consuming as little memory as possible.
+        curl_global_init(CURL_GLOBAL_ALL);
+
         curl = curl_easy_init();
         if (!curl) {
                 telem_log(LOG_ERR, "curl_easy_init(): Unable to start libcurl"
@@ -532,6 +537,9 @@ bool post_record_http(char *headers[], char *body, bool spool)
 
         curl_slist_free_all(custom_headers);
         curl_easy_cleanup(curl);
+
+        curl_global_cleanup();
+
         return res ? false : true;
 }