segfaulting bug in rrdtool/rrdcached

There's a bug in the current HEAD of rrdtool (and I suppose going back to
mid-2007, from the svn blame output) which causes it to segfault if you
point it at an rrdcached socket which isn't writable. I've attached a patch
against trunk, and reproduction steps are below:

cd ~
mkdir rrds/
rrdtool create rrds/test.rrd DS:data:GAUGE:360:U:U RRA:MAX:0.5:1:120 -s 1
rrdtool update rrds/test.rrd N:0
rrdtool xport --start $(( $(date +%s) - 120)) --end $(date +%s)
DEF:ds0=$HOME/rrds/test.rrd:data:MAX XPORT:ds0     *(this one should work)*
rrdtool xport --start $(( $(date +%s) - 120)) --end $(date +%s) --daemon
$HOME/this_path_does_not_exist.sock DEF:ds0=$HOME/rrds/test.rrd:data:MAX
XPORT:ds0    *(this one should segfault)*

rrdtool is assuming that rrd_xport will always return -1 on failure;
however, rrd_xport returns errno (which is, generally, not -1) if
rrd_client fails. I figured it was easier to change rrdtool than to change
everything in rrd_client. For good measure, I also changed the checks on
the calls to rrd_fetch and rrd_graph. I'm not sure if they're susceptible
to the same problem, but, well, better to check for the one thing you do
what you want than to enumerate all the possible things you don't want.

This segfault is caused by an uninitialized variable use (in particular,
legend_v and col_cnt end up being used and passed to printf uninitialized).
Nothing offhand jumped out at me as easily-exploitable to do code
injection, but I only spent five or so minutes looking at it, so there very
well may be a security problem hiding behind this.

Cheers,
-- 
James Brown
Systems Engineer
Yelp, Inc.

_______________________________________________
rrd-developers mailing list
[email protected]
https://lists.oetiker.ch/cgi-bin/listinfo/rrd-developers

Index: src/rrd_tool.c
===================================================================
--- src/rrd_tool.c	(revision 2248)
+++ src/rrd_tool.c	(working copy)
@@ -685,7 +685,7 @@
 
         if (rrd_fetch
             (argc - 1, &argv[1], &start, &end, &step, &ds_cnt, &ds_namv,
-             &data) != -1) {
+             &data) == 0) {
             datai = data;
             printf("           ");
             for (i = 0; i < ds_cnt; i++)
@@ -722,7 +722,7 @@
 
         if (rrd_xport
             (argc - 1, &argv[1], &xxsize, &start, &end, &step, &col_cnt,
-             &legend_v, &data) != -1) {
+             &legend_v, &data) == 0) {
             char *old_locale = setlocale(LC_NUMERIC,NULL);
             setlocale(LC_NUMERIC, "C");
             row_cnt = (end - start) / step;
@@ -805,7 +805,7 @@
         }
         if (rrd_graph
             (argc - 1, &argv[1], &calcpr, &xsize, &ysize, NULL, &ymin,
-             &ymax) != -1) {
+             &ymax) == 0) {
             if (!tostdout && !imginfo)
                 printf("%dx%d\n", xsize, ysize);
             if (calcpr) {