]> git.ipfire.org Git - thirdparty/rrdtool-1.x.git/commit
Recently discovered fixes (#803)
authorAlan Jenkins <alan.christopher.jenkins@gmail.com>
Sat, 8 Jul 2017 10:53:45 +0000 (11:53 +0100)
committerTobias Oetiker <tobi@oetiker.ch>
Sat, 8 Jul 2017 10:53:45 +0000 (12:53 +0200)
commit144ce91c8d05168144563905317039684c7b30d7
tree64e8624b8e756341761d8cdbd10ee72f8b97f04d
parent248671a941dad879cc90dfe081a1ea77cc34aec1
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
src/rrd_cgi.c
src/rrd_client.c
src/rrd_flushcached.c
src/rrd_list.c
src/rrd_open.c
src/rrd_tune.c
src/rrd_update.c