From: Alan Jenkins Date: Sat, 8 Jul 2017 10:53:45 +0000 (+0100) Subject: Recently discovered fixes (#803) X-Git-Tag: v1.7.1~120 X-Git-Url: http://git.ipfire.org/cgi-bin/gitweb.cgi?a=commitdiff_plain;h=144ce91c8d05168144563905317039684c7b30d7;p=thirdparty%2Frrdtool-1.x.git 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 7165bea1..5425f237 100644 --- 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 0ac08ef2..1bfbef2a 100644 --- 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 5de12787..1788e2b8 100644 --- 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 84b96df5..7a244cfd 100644 --- 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 0114f058..3eec95ab 100644 --- 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 2239bcc7..c718febf 100644 --- 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 797f89bb..55769eaa 100644 --- 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;