ceph plugin: Rewrite parse_keys() and compact_ds_name().

This rewrite removes the logic to add the original length to the end of
the buffer in case of truncation. This logic was guarded by:
  tmp[DATA_MAX_NAME_LEN - 1] = '\0';
  if(strlen(tmp) > DATA_MAX_NAME_LEN - 1) { ... }
and was therefore dead code.

Fixes: #1350

diff --git a/src/ceph.c b/src/ceph.c
index d928a7ba4b5db1d71e87449f635783e44000f621..f48e056e68a0c5ca78f3476419732bb057f1ba52 100644 (file)
--- a/src/ceph.c
+++ b/src/ceph.c
@@ -1,6 +1,7 @@
 /**
  * collectd - src/ceph.c
  * Copyright (C) 2011  New Dream Network
+ * Copyright (C) 2015  Florian octo Forster
  *
  * This program is free software; you can redistribute it and/or modify it
  * under the terms of the GNU General Public License as published by the
@@ -16,9 +17,10 @@
  * 51 Franklin St, Fifth Floor, Boston, MA  02110-1301 USA
  *
  * Authors:
- *   Colin McCabe <cmccabe@alumni.cmu.edu>
- *   Dennis Zou <yunzou@cisco.com>
- *   Dan Ryder <daryder@cisco.com>
+ *   Colin McCabe <cmccabe at alumni.cmu.edu>
+ *   Dennis Zou <yunzou at cisco.com>
+ *   Dan Ryder <daryder at cisco.com>
+ *   Florian octo Forster <octo at collectd.org>
  **/
 
 #define _DEFAULT_SOURCE
@@ -420,140 +422,140 @@ static void ceph_daemon_free(struct ceph_daemon *d)
     sfree(d);
 }
 
-/**
- * Compact ds name by removing special characters and trimming length to
- * DATA_MAX_NAME_LEN if necessary
- */
-static void compact_ds_name(char *source, char *dest)
+/* compact_ds_name removed the special characters ":", "_", "-" and "+" from the
+ * intput string. Characters following these special characters are capitalized.
+ * Trailing "+" and "-" characters are replaces with the strings "Plus" and
+ * "Minus". */
+static int compact_ds_name (char *buffer, size_t buffer_size, char const *src)
 {
-    int keys_num = 0, i;
-    char *save_ptr = NULL, *tmp_ptr = source;
-    char *keys[16];
-    char len_str[3];
-    char tmp[DATA_MAX_NAME_LEN];
-    size_t key_chars_remaining = (DATA_MAX_NAME_LEN-1);
-    int reserved = 0;
-    int offset = 0;
-    memset(tmp, 0, sizeof(tmp));
-    if(source == NULL || dest == NULL || source[0] == '\0' || dest[0] != '\0')
-    {
-        return;
-    }
-    size_t src_len = strlen(source);
-    snprintf(len_str, sizeof(len_str), "%zu", src_len);
-    unsigned char append_status = 0x0;
-    append_status |= (source[src_len - 1] == '-') ? 0x1 : 0x0;
-    append_status |= (source[src_len - 1] == '+') ? 0x2 : 0x0;
-    while ((keys[keys_num] = strtok_r(tmp_ptr, ":_-+", &save_ptr)) != NULL)
-    {
-        tmp_ptr = NULL;
-        /** capitalize 1st char **/
-        keys[keys_num][0] = toupper(keys[keys_num][0]);
-        keys_num++;
-        if(keys_num >= 16)
-        {
-            break;
-        }
-    }
-    /** concatenate each part of source string **/
-    for(i = 0; i < keys_num; i++)
-    {
-        strncat(tmp, keys[i], key_chars_remaining);
-        key_chars_remaining -= strlen(keys[i]);
-    }
-    tmp[DATA_MAX_NAME_LEN - 1] = '\0';
-    /** to coordinate limitation of length of type_instance
-     *  we will truncate ds_name
-     *  when the its length is more than
-     *  DATA_MAX_NAME_LEN
-     */
-    if(strlen(tmp) > DATA_MAX_NAME_LEN - 1)
+    char *src_copy;
+    size_t src_len;
+    char *ptr = buffer;
+    size_t ptr_size = buffer_size;
+    _Bool append_plus = 0;
+    _Bool append_minus = 0;
+
+    if ((buffer == NULL) || (buffer_size <= strlen ("Minus")) || (src == NULL))
+      return EINVAL;
+
+    src_copy = strdup (src);
+    src_len = strlen(src);
+
+    /* Remove trailing "+" and "-". */
+    if (src_copy[src_len - 1] == '+')
     {
-        append_status |= 0x4;
-        /** we should reserve space for
-         * len_str
-         */
-        reserved += 2;
+        append_plus = 1;
+        src_len--;
+        src_copy[src_len] = 0;
     }
-    if(append_status & 0x1)
+    else if (src_copy[src_len - 1] == '-')
     {
-        /** we should reserve space for
-         * "Minus"
-         */
-        reserved += 5;
+        append_minus = 1;
+        src_len--;
+        src_copy[src_len] = 0;
     }
-    if(append_status & 0x2)
+
+    /* Split at special chars, capitalize first character, append to buffer. */
+    char *dummy = src_copy;
+    char *token;
+    char *save_ptr = NULL;
+    while ((token = strtok_r (dummy, ":_-+", &save_ptr)) != NULL)
     {
-        /** we should reserve space for
-         * "Plus"
-         */
-        reserved += 4;
+        size_t len;
+
+        dummy = NULL;
+
+        token[0] = toupper ((int) token[0]);
+
+        assert (ptr_size > 1);
+
+        len = strlen (token);
+        if (len >= ptr_size)
+            len = ptr_size - 1;
+
+        assert (len > 0);
+        assert (len < ptr_size);
+
+        sstrncpy (ptr, token, len + 1);
+        ptr += len;
+        ptr_size -= len;
+
+        assert (*ptr == 0);
+        if (ptr_size <= 1)
+            break;
     }
-    snprintf(dest, DATA_MAX_NAME_LEN - reserved, "%s", tmp);
-    offset = strlen(dest);
-    switch (append_status)
+
+    /* Append "Plus" or "Minus" if "+" or "-" has been stripped above. */
+    if (append_plus || append_minus)
     {
-        case 0x1:
-            memcpy(dest + offset, "Minus", 5);
-            break;
-        case 0x2:
-            memcpy(dest + offset, "Plus", 5);
-            break;
-        case 0x4:
-            memcpy(dest + offset, len_str, 2);
-            break;
-        case 0x5:
-            memcpy(dest + offset, "Minus", 5);
-            memcpy(dest + offset + 5, len_str, 2);
-            break;
-        case 0x6:
-            memcpy(dest + offset, "Plus", 4);
-            memcpy(dest + offset + 4, len_str, 2);
-            break;
-        default:
-            break;
+        char const *append = "Plus";
+        if (append_minus)
+            append = "Minus";
+
+        size_t offset = buffer_size - (strlen (append) + 1);
+        if (offset > strlen (buffer))
+            offset = strlen (buffer);
+
+        sstrncpy (buffer + offset, append, buffer_size - offset);
     }
+
+    sfree (src_copy);
+    return 0;
+}
+
+static _Bool has_suffix (char const *str, char const *suffix)
+{
+    size_t str_len = strlen (str);
+    size_t suffix_len = strlen (suffix);
+    size_t offset;
+
+    if (suffix_len > str_len)
+        return 0;
+    offset = str_len - suffix_len;
+
+    if (strcmp (str + offset, suffix) == 0)
+        return 1;
+
+    return 0;
+}
+
+/* count_parts returns the number of elements a "foo.bar.baz" style key has. */
+static size_t count_parts (char const *key)
+{
+    char const *ptr;
+    size_t parts_num = 0;
+
+    for (ptr = key; ptr != NULL; ptr = strchr (ptr + 1, '.'))
+        parts_num++;
+
+    return parts_num;
 }
 
 /**
  * Parse key to remove "type" if this is for schema and initiate compaction
  */
-static int parse_keys(const char *key_str, char *ds_name)
+static int parse_keys (char *buffer, size_t buffer_size, const char *key_str)
 {
-    char *ptr, *rptr;
-    size_t ds_name_len = 0;
-    /**
-     * allow up to 100 characters before compaction - compact_ds_name will not
-     * allow more than DATA_MAX_NAME_LEN chars
-     */
-    int max_str_len = 100;
-    char tmp_ds_name[max_str_len];
-    memset(tmp_ds_name, 0, sizeof(tmp_ds_name));
-    if(ds_name == NULL || key_str == NULL ||  key_str[0] == '\0' ||
-                                                            ds_name[0] != '\0')
-    {
-        return -1;
-    }
-    if((ptr = strchr(key_str, '.')) == NULL
-            || (rptr = strrchr(key_str, '.')) == NULL)
+    char tmp[2 * buffer_size];
+
+    if (buffer == NULL || buffer_size == 0 || key_str == NULL || strlen (key_str) == 0)
+        return EINVAL;
+
+    if ((count_parts (key_str) > 2) && has_suffix (key_str, ".type"))
     {
-        memcpy(tmp_ds_name, key_str, max_str_len - 1);
-        goto compact;
-    }
+        /* strip ".type" suffix iff the key has more than two parts. */
+        size_t sz = strlen (key_str) - strlen (".type") + 1;
 
-    ds_name_len = (rptr - ptr) > max_str_len ? max_str_len : (rptr - ptr);
-    if((ds_name_len == 0) || strncmp(rptr + 1, "type", 4))
-    { /** copy whole key **/
-        memcpy(tmp_ds_name, key_str, max_str_len - 1);
+        if (sz > sizeof (tmp))
+            sz = sizeof (tmp);
+        sstrncpy (tmp, key_str, sz);
     }
     else
-    {/** more than two keys **/
-        memcpy(tmp_ds_name, key_str, ((rptr - key_str) > (max_str_len - 1) ?
-                (max_str_len - 1) : (rptr - key_str)));
+    {
+        sstrncpy (tmp, key_str, sizeof (tmp));
     }
 
-    compact: compact_ds_name(tmp_ds_name, ds_name);
-    return 0;
+    return compact_ds_name (buffer, buffer_size, tmp);
 }
 
 /**
@@ -605,7 +607,7 @@ static int ceph_daemon_add_ds_entry(struct ceph_daemon *d, const char *name,
             ((pc_type & PERFCOUNTER_LATENCY) ? DSET_LATENCY : DSET_BYTES);
     d->ds_types[d->ds_num] = type;
 
-    if(parse_keys(name, ds_name))
+    if (parse_keys(ds_name, sizeof (ds_name), name))
     {
         return 1;
     }
@@ -957,7 +959,7 @@ static int node_handler_fetch_data(void *arg, const char *val, const char *key)
     char ds_name[DATA_MAX_NAME_LEN];
     memset(ds_name, 0, sizeof(ds_name));
 
-    if(parse_keys(key, ds_name))
+    if (parse_keys (ds_name, sizeof (ds_name), key))
     {
         return 1;
     }

diff --git a/src/ceph_test.c b/src/ceph_test.c
index 0eee5bc5858021511318882c2e9e452e010295c8..ffe2c7f6955fbb38afc53034b79722526e67cf1a 100644 (file)
--- a/src/ceph_test.c
+++ b/src/ceph_test.c
@@ -46,7 +46,7 @@ DEF_TEST(parse_keys)
 
     memset (got, 0, sizeof (got));
 
-    parse_keys (cases[i].str, got);
+    CHECK_ZERO (parse_keys (got, sizeof (got), cases[i].str));
 
     CHECK_ZERO (strcmp (got, cases[i].want));
   }