Recently discovered fixes (#803)

* fix incorrect use of snprintf in rrd_cgi

DS_NAM_SIZE is used to size a buffer, but there is no relation to ds_nam.
Fortunately this lead to the buffer being over-sized.

Replace with code which does not rely on calculating the size of a
formatted string.  (If the error message is around the maximum possible
size supported by librrd, we might drop a closing ']'.  That won't cause
us a problem).

We don't have a shim for asprintf().  rrd_asprintf() and friends are for
writing e.g. XML etc without locale-specific formatting.

* refactor - header_len in rrd_open() is only valid for new files

this would be confusing and undesirable if it was used for anything other
than calculating newfile_size.  Fortunately it is not.  The variable can
easily be scoped to avoid confusion.

* rrd_open() should position "to the first cdp in the first rra"

but RRD_READVALUES was buggy.  It left the fd positioned at the end of the
file, but with rrd_file->pos still set to just after the header.
(rados, which doesn't use the fd, would be unafffected by this bug, leading
to differing behaviour).

The comment for rrd_open implies we should rrd_seek() back to just after
the header.  So let's resolve the inconsistency that way.

The old code with d_offset was completely pointless.  It was used to
restore rrd_file->pos, but rrd_file->pos was not changed, even by the
__rrd_read macro magic.

* rrd_open(rrd, ... RRD_CREAT) requires non-NULL rrd->stat_head

We're only going to do something horrible later involving `(size_t) -1`.
Just dereference NULL and trigger the undefined behaviour now, rather
than confuse readers about how RRD_CREAT works when rrd->stat_head is not
provided.

(But if anyone wants to expand argument checking and make it not
conditional on DEBUG, I have no objection to that either :).

* remove unecessary strdup/free in rrd_flushcached()

* rrd_get_error() does not return NULL when there is no message

* don't silently lose error messages to ENOMEM

If we run out of memory, that's a catastrophic error; we want to report it
in some way.  (Note there's a good chance the error message we lost was
caused by ENOMEM, directly or indirectly).

* rrd_client: request() never sets `res` on failure, don't bother checking it

When request() returns non-zero (failure), it does not set `ret_response`.
So there is no point trying to extract an error message from ret_response
when request() returns non-zero.

If rrdcached returned an error response (ret_response->status < 0), then
response_read() will already have passed it to rrd_set_error().

* rrd_client: add missing checks for error responses

diff --git a/src/rrd_cgi.c b/src/rrd_cgi.c
index 7165bea1563340a8976c6ea61aaabde4349c9b44..5425f23773aa2ef09c074bc44ec79d9a19c784d5 100644 (file)
--- a/src/rrd_cgi.c
+++ b/src/rrd_cgi.c
@@ -754,13 +754,12 @@ static char *includefile(
 
         readfile(filename, &buffer, 0);
         if (rrd_test_error()) {
-            const size_t len = strlen(rrd_get_error()) + DS_NAM_SIZE;
-            char *err = (char *) malloc(len);
-
-            snprintf(err, len, "[ERROR: %s]", rrd_get_error());
+            char err[4096];
+            snprintf(err, sizeof(err), "[ERROR %s]", rrd_get_error());
             rrd_clear_error();
+
             free(buffer);
-            return err;
+            return stralloc(err);
         } else {
             return buffer;
         }
@@ -922,11 +921,11 @@ static char *drawgraph(
         return stralloc(calcpr[0]);
     } else {
         if (rrd_test_error()) {
-            const size_t len = strlen(rrd_get_error()) + DS_NAM_SIZE;
-            char *err = (char *) malloc(len);
-            snprintf(err, len, "[ERROR: %s]", rrd_get_error());
+            char err[4096];
+            snprintf(err, sizeof(err), "[ERROR %s]", rrd_get_error());
             rrd_clear_error();
-            return err;
+
+            return stralloc(err);
         }
     }
     return NULL;
@@ -965,12 +964,12 @@ static char *printtimelast(
 
         last = rrd_last(argc, (char **) args - 1);
         if (rrd_test_error()) {
-            const size_t len = strlen(rrd_get_error()) + DS_NAM_SIZE;
-            char *err = (char *) malloc(len);
-            snprintf(err, len, "[ERROR: %s]", rrd_get_error());
+            char err[4096];
+            snprintf(err, sizeof(err), "[ERROR %s]", rrd_get_error());
             rrd_clear_error();
+
             free(buf);
-            return err;
+            return stralloc(err);
         }
         tm_last = *localtime(&last);
         strftime(buf, 254, args[1], &tm_last);

diff --git a/src/rrd_client.c b/src/rrd_client.c
index 0ac08ef24024ba877bdff29d77266959da7326e1..1bfbef2a9a7f04d6e41e7b3e8a8260b523a93f48 100644 (file)
--- a/src/rrd_client.c
+++ b/src/rrd_client.c
@@ -1235,13 +1235,9 @@ rrd_info_t *rrd_client_info(rrd_client_t *client, const char *filename) /* {{{ *
   res = NULL;
   status = request(client, buffer, buffer_size, &res);
 
-  if (status != 0) {
-    if (res && res->message) {
-      rrd_set_error ("rrdcached@%s: %s", client->sd_path, res->message);
-      response_free(res);
-    }
-    return (NULL);
-  }
+  if (status != 0 || res->status < 0)
+    goto out_free_res;
+
   data = cd = NULL;
   for( l=0 ; l < res->lines_num ; l++ ) {
     /* first extract the keyword */
@@ -1284,6 +1280,8 @@ rrd_info_t *rrd_client_info(rrd_client_t *client, const char *filename) /* {{{ *
     cd = rrd_info_push(cd, sprintf_alloc("%s",k), itype, info);
        if(!data) data = cd;
   }
+
+out_free_res:
   response_free (res);
 
   return (data);
@@ -1350,13 +1348,10 @@ char *rrd_client_list(rrd_client_t *client, int recursive, const char *dirname)
   buffer[buffer_size - 1] = '\n';
 
   res = NULL;
-
   status = request(client, buffer, buffer_size, &res);
 
-  if (status != 0) {
-    rrd_set_error ("rrdcached@%s: %s", client->sd_path, res->message);
+  if (status != 0 || res->status < 0)
     goto out_free_res;
-  }
 
   /* Handle the case where the list is empty, allocate
    * a single byte zeroed string.
@@ -1465,10 +1460,13 @@ time_t rrd_client_last(rrd_client_t *client, const char *filename) /* {{{ */
   res = NULL;
   status = request(client, buffer, buffer_size, &res);
 
-  if (status != 0) {
-    rrd_set_error ("rrdcached@%s: %s", client->sd_path, res->message);
+  if (status != 0)
+    return (-1);
+  if (res->status < 0) {
+    response_free (res);
     return (-1);
   }
+
   lastup = atol(res->message);
   response_free (res);
 
@@ -1540,10 +1538,13 @@ time_t rrd_client_first (rrd_client_t *client, const char *filename, int rrainde
   res = NULL;
   status = request(client, buffer, buffer_size, &res);
 
-  if (status != 0) {
-    rrd_set_error ("rrdcached@%s: %s", client->sd_path, res->message);
+  if (status != 0)
+    return (-1);
+  if (res->status < 0) {
+    response_free (res);
     return (-1);
   }
+
   firstup = atol(res->message);
   response_free (res);
 
@@ -1671,12 +1672,12 @@ int rrd_client_create_r2(rrd_client_t *client, const char *filename, /* {{{ */
   res = NULL;
   status = request(client, buffer, buffer_size, &res);
 
-  if (status != 0) {
-    rrd_set_error ("rrdcached@%s: %s", client->sd_path, res->message);
+  if (status != 0)
     return (-1);
-  }
+
+  status = res->status;
   response_free (res);
-  return(0);
+  return(status);
 } /* }}} int rrd_client_create_r2 */
 int rrdc_create_r2(const char *filename, /* {{{ */
     unsigned long pdp_step,
@@ -1790,12 +1791,11 @@ int rrd_client_fetch (rrd_client_t *client, const char *filename, /* {{{ */
     return (status);
   }
   status = res->status;
-  if (status < 0)
-  {
-    rrd_set_error ("rrdcached@%s: %s", client->sd_path, res->message);
-    response_free (res);
+  if (status < 0) {
+    response_free(res);
     return (status);
   }
+
   /* }}} Send request */
 
   ds_names = NULL;

diff --git a/src/rrd_flushcached.c b/src/rrd_flushcached.c
index 5de127876bcc1ed1c5b91af489943f7b59bcca62..1788e2b87a795aedd08fe5bc45320d88dad71283 100644 (file)
--- a/src/rrd_flushcached.c
+++ b/src/rrd_flushcached.c
@@ -92,14 +92,13 @@ int rrd_flushcached (int argc, char **argv)
             char *error;
             int   remaining;
 
-            error     = strdup(rrd_get_error());
+            error     = rrd_get_error();
             remaining = options.argc - options.optind - 1;
 
             rrd_set_error("Flushing of file \"%s\" failed: %s. Skipping "
                     "remaining %i file%s.", options.argv[i],
-                    ((! error) || (*error == '\0')) ? "unknown error" : error,
+                    (*error == '\0') ? "unknown error" : error,
                     remaining, (remaining == 1) ? "" : "s");
-            free(error);
             break;
         }
     }

diff --git a/src/rrd_list.c b/src/rrd_list.c
index 84b96df5bb780872308901d515f7a97befe62833..7a244cfd8e2e40c0eabaeac2aa14ee112b23ccf6 100644 (file)
--- a/src/rrd_list.c
+++ b/src/rrd_list.c
@@ -232,7 +232,6 @@ char *rrd_list(int argc, char **argv)
        };
        struct optparse options;
        int    opt;
-       char    *err = NULL;
 
         optparse_init(&options, argc, argv);
 
@@ -311,10 +310,8 @@ char *rrd_list(int argc, char **argv)
        } else {
                if (opt_daemon) {
                        fprintf(stderr, "Error connecting to rrdcached");
-                       err = rrd_get_error();
-
-                       if (err)
-                               fprintf(stderr, ": %s", err);
+                       if (rrd_test_error())
+                               fprintf(stderr, ": %s", rrd_get_error());
                        fprintf(stderr, "\n");
 
                        free(opt_daemon);

diff --git a/src/rrd_open.c b/src/rrd_open.c
index 0114f05839f2596bfd1da8c8f93d7d11f5e8226c..3eec95ab75fed8bd21f724805a90afd1a18e1ddc 100644 (file)
--- a/src/rrd_open.c
+++ b/src/rrd_open.c
@@ -160,11 +160,12 @@ rrd_file_t *rrd_open(
     rrd_file_t *rrd_file = NULL;
     rrd_simple_file_t *rrd_simple_file = NULL;
     size_t     newfile_size = 0;
-    size_t header_len, value_cnt, data_len;
 
     /* Are we creating a new file? */
-    if((rdwr & RRD_CREAT) && (rrd->stat_head != NULL))
+    if(rdwr & RRD_CREAT)
     {
+        size_t header_len, value_cnt, data_len;
+
         header_len = rrd_get_header_size(rrd);
 
         value_cnt = 0;
@@ -506,15 +507,12 @@ read_check:
         goto out_close;
       }
       if (rdwr & RRD_READVALUES) {
-         long d_offset = offset;
-         
-         __rrd_read(rrd->rrd_value, rrd_value_t,
-                    row_cnt * rrd->stat_head->ds_cnt);
+        __rrd_read(rrd->rrd_value, rrd_value_t,
+                   row_cnt * rrd->stat_head->ds_cnt);
 
-         rrd_file->header_len = d_offset;
-         rrd_file->pos = d_offset;
+        if (rrd_seek(rrd_file, rrd_file->header_len, SEEK_SET) != 0)
+          goto out_close;
       }
-      
     }
 
   out_done:

diff --git a/src/rrd_tune.c b/src/rrd_tune.c
index 2239bcc739213fbfcf1f093150bd1c40b6953ccc..c718febf5a6a49447bf4b377829c895dca096a44 100644 (file)
--- a/src/rrd_tune.c
+++ b/src/rrd_tune.c
@@ -417,11 +417,11 @@ done:
        rrdc_forget(in_filename);
        rrd_clear_error();
         
-        if (e && *e) {
+        if (e) {
             rrd_set_error(e);
-        }
-        if (e) free(e);
-        
+            free(e);
+        } else
+            rrd_set_error("error message was lost (out of memory)");
     }
     if (rrd_file) {
        rrd_close(rrd_file);

diff --git a/src/rrd_update.c b/src/rrd_update.c
index 797f89bb6439c74a39974dbb4c0afbdac56863c0..55769eaaae9c359944fcc769886221b81a69b9a1 100644 (file)
--- a/src/rrd_update.c
+++ b/src/rrd_update.c
@@ -895,7 +895,8 @@ static int _rrd_updatex(
                 if ((save_error = strdup(rrd_get_error())) != NULL) {
                     rrd_set_error("%s: %s", filename, save_error);
                     free(save_error);
-                }
+                } else
+                    rrd_set_error("error message was lost (out of memory)");
             }
             free(arg_copy);
             break;