Re: Bug#573638: rrdtool: rrdcgi crashes at printlasttime()

Hi Robert,

in 2012:2013 I put in the patch below ... this should provide a
more generic fix for the problem. rrd_cgi functions all used to
fail horribly when a librrd function tried to touch ARGV[0] (mostly
to print an errror message including the name name of the calling
program) ...

obviously an even better fix would be to remove the whole argv
magic from rrd_cgi ... but this patch would be rather substantial.

cheers
tobi

--- rrd_cgi.c   (revision 2012)
+++ rrd_cgi.c   (working copy)
@@ -1053,7 +1053,7 @@
     int       curarg_contains_rrd_directives;

     /* local array of arguments while parsing */
-    int       argc = 0;
+    int       argc = 1;
     char    **argv;

 #ifdef DEBUG_PARSER
@@ -1069,6 +1069,7 @@
     if (!argv) {
         return NULL;
     }
+    argv[0] = "rrdcgi";

     /* skip leading blanks */
     while (isspace((int) *line)) {
@@ -1172,7 +1173,7 @@
         argv[argc - 1] = rrd_expand_vars(stralloc(argv[argc - 1]));
     }
 #ifdef DEBUG_PARSER
-    if (argc > 0) {
+    if (argc > 1) {
         int       n;

         printf("<-- arguments found [%d]\n", argc);
@@ -1186,9 +1187,18 @@
 #endif

     /* update caller's notion of the argument array and it's size */
-    *arguments = argv;
-    *argument_count = argc;

+    /* note this is a bit of a hack since the rrd_cgi code used to just put
+       its arguments into a normal array starting at 0 ... since the rrd_*
+       commands expect and argc/argv array we used to just shift everything
+       by -1 ... this in turn exploded when a rrd_* function tried to print
+       argv[0] ... hence we are now doing everything in argv style but hand
+       over seemingly the old array ... but doing argv-1 will actually end
+       up in a 'good' place now. */
+
+    *arguments = argv+1;
+    *argument_count = argc-1;
+
     if (Quote) {
         return NULL;
     }
@@ -1241,7 +1251,7 @@
     if (end) {
         /* got arguments, call function for 'tag' with arguments */
         val = func(argc, (const char **) args);
-        free(args);
+        free(args-1);
     } else {
         /* unable to parse arguments, undo 0-termination by scanargs */
         for (; argc > 0; argc--) {

at Today Sebastian Harl wrote:

> Hi Robert,
>
> On Thu, Mar 11, 2010 at 01:34:01PM +0100, Robert Luberda wrote:
> > iptotal.cgi (from the iptotal package) contains the following line
> > <RRD::TIME::LAST /var/lib/iptotal/iptotal.rrd %c>
> > which causes rrdcgi to crash with the following backtrace:
> >
> > (gdb) bt
> > #0  strlen () at ../sysdeps/i386/i486/strlen.S:40
> > #1  0xb73a681e in _IO_vfprintf_internal (s=0xbfa4086c,
> >     format=0xb781edd0 "Usage: rrdtool %s [--daemon <addr>] <file>",
> > ap=0xbfa40988 "\021\001\202?")
> >     at vfprintf.c:1601
> > #2  0xb73c56b4 in _IO_vsnprintf (string=0xb78269c0 "Usage: rrdtool ",
> > maxlen=4096,
> >     format=0xb781edd0 "Usage: rrdtool %s [--daemon <addr>] <file>",
> > args=0xbfa40984 "\211")
> >     at vsnprintf.c:120
> > #3  0xb78140c4 in rrd_set_error () from /usr/lib/librrd.so.4
> > #4  0xb7805be4 in rrd_last () from /usr/lib/librrd.so.4
> > #5  0x0804b211 in printtimelast ()
> > #6  0x0804aa83 in ?? ()
> > #7  0x0804c265 in ?? ()
> > #8  0xb737bb55 in __libc_start_main (main=0x804bf70, argc=2,
> > ubp_av=0xbfa40bb4, init=0x804c5c0,
> >     fini=0x804c5b0, rtld_fini=0xb78629b0 <_dl_fini>,
> > stack_end=0xbfa40bac) at libc-start.c:222
>
> Thanks for reporting this!
>
> > Afer some investigation, I found that the problem is in the line 991
> > of rrd_cgi.c:
> >
> >   last = rrd_last(argc + 1, (char **) args - 1);
> >
> > The first argument of rrd_last() should obviously be argc (which is 2),
> > not argc + 1.  Also please note that second argument of the function
> > refers to address before the start of the array, which seems to
> > be a very bad programming style, and which in fact is a root cause of the
> > crash as rrd_last() tries to display argv[0] in an error message.
>
> Ouch! What an ugly hack ?
>
> > The attached patch fixes the problem.
>
> Thanks for tracing that back and providing a patch! Imho, the patch
> looks fine. With this E-mail, I'm forwarding the issue and the patch
> upstream, hoping for inclusion in the upstream SVN. I'll upload a fixed
> package to Debian soonish.
>
> Cheers,
> Sebastian
>
>

-- 
Tobi Oetiker, OETIKER+PARTNER AG, Aarweg 15 CH-4600 Olten, Switzerland
http://it.oetiker.ch [email protected] ++41 62 775 9902 / sb: -9900