segmentation fault when kMG option contains more prefixes

[Vitezslav Crhonek]

Hi,

I tried to set kMG option as in example in documentation
(http://oss.oetiker.ch/mrtg/doc/mrtg-reference.en.html),
but the mrtg failed then with:

# env LANG=C mrtg /etc/mrtg/mrtg.cfg
Monday, 16 May 2011 at 6:48: ERROR: Skipping webupdates because rateup
did not return anything sensible
Monday, 16 May 2011 at 6:48: WARNING: rateup died from Signal 11
with Exit Value 0 when doing router 'localhost_2'
Signal was 11, Returncode was 0

("kMG[localhost_2]: n,u,m,,k,M,G,T,P" in mrtg.cfg file)

Proposed patch fixing the issue follows.

Please let me know what do you think about it.

Best regards
Vitezslav Crhonek

--- rateup.c.test 2011-02-20 23:33:38.000000000 +0100
+++ rateup.c 2011-05-16 17:02:06.935338826 +0200
@@ -111,7 +111,7 @@

char *short_si_def[] = { "", "k", "M", "G", "T" };
int kMGnumber = 4;
-char **short_si = short_si_def;
+char **short_si;
char *longup = NULL;
char *shortup = NULL;
char *pngtitle = NULL;
@@ -484,19 +484,22 @@

if (kMG)
{
- if (short_si[0] != kMG)
+ short_si = calloc (strlen(kMG), sizeof(char *)); /* allocated
more than enough */
+ short_si_out = kMG;
+ kMGnumber = 0;
+ short_si[0] = kMG;
+ while ((short_si_out = strchr (short_si_out, ',')) != NULL)
{
- short_si_out = kMG;
- kMGnumber = 0;
- short_si[0] = kMG;
- while ((short_si_out = strchr (short_si_out, ',')) != NULL)
- {
- short_si_out[0] = '\0';
- short_si_out++;
- short_si[++kMGnumber] = short_si_out;
- }
- }
+ short_si_out[0] = '\0';
+ short_si_out++;
+ short_si[++kMGnumber] = short_si_out;
+ }
}
+ else
+ {
+ short_si = short_si_def;
+ kMGnumber = 4;
+ }

/* mangle the 0.25*maxv value so, that we get a number with either */
/* one or two digits != 0 and these digits should be at the */
@@ -1125,6 +1128,8 @@
gdImageDestroy (brush_outp);
free (lhist);
free (graph_label);
+ if (kMG)
+ free(short_si); /* I'm not sure if this is ideal place for freeing */

#ifdef WIN32
/* got to remove the target under win32

[Vitezslav Crhonek]

Post by Vitezslav Crhonek
Hi,
I tried to set kMG option as in example in documentation
(http://oss.oetiker.ch/mrtg/doc/mrtg-reference.en.html),
# env LANG=C mrtg /etc/mrtg/mrtg.cfg
Monday, 16 May 2011 at 6:48: ERROR: Skipping webupdates because rateup
did not return anything sensible
Monday, 16 May 2011 at 6:48: WARNING: rateup died from Signal 11
with Exit Value 0 when doing router 'localhost_2'
Signal was 11, Returncode was 0
("kMG[localhost_2]: n,u,m,,k,M,G,T,P" in mrtg.cfg file)
Proposed patch fixing the issue follows.
Please let me know what do you think about it.
Best regards
Vitezslav Crhonek

Hello again,

I discovered that this part of code was changed in 2006:

Changes 2.14.6, 2006-09-06
--------------------------
From: Tobi
* timestamps in log files to be YYYY-MM-DD HH:MM:SS
* fixed rateup to propely support kMG option
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

Previously, memory for short_si was allocated correctly, so there wasn't
Segmentation fault threat. Therefore I took the code before that fix and
modified it to:
- add '\0' character to short_si_out[0] in the for loop (which resolves
initial issue of kMG option content overleaping into the graph picture)
- add free(short_si) at the end of corresponding function (I believe it
should be freed and it was not - at least I didn't find it)

Now it should be all OK, please review and let me know your opinion.
New patch is below.

Best regards,
Vitezslav Crhonek

--- rateup.c.test 2011-02-20 23:33:38.000000000 +0100
+++ rateup.c 2011-05-17 12:20:17.287887662 +0200
@@ -488,15 +488,24 @@
{
short_si_out = kMG;
kMGnumber = 0;
- short_si[0] = kMG;
while ((short_si_out = strchr (short_si_out, ',')) != NULL)
- {
- short_si_out[0] = '\0';
- short_si_out++;
- short_si[++kMGnumber] = short_si_out;
- }
+ {
+ short_si_out++;
+ kMGnumber++;
+ }
+
+ short_si = calloc(kMGnumber + 1, sizeof(*short_si));
+ short_si_out = kMG;
+ for (kMGnumber = 0; ; kMGnumber++)
+ {
+ short_si[kMGnumber] = short_si_out;
+ short_si_out = strchr(short_si_out, ',');
+ if (short_si_out == NULL) break;
+ short_si_out[0] = '\0';
+ short_si_out++;
+ }
}
- }
+ }

/* mangle the 0.25*maxv value so, that we get a number with either */
/* one or two digits != 0 and these digits should be at the */
@@ -1125,6 +1134,8 @@
gdImageDestroy (brush_outp);
free (lhist);
free (graph_label);
+ if (kMG)
+ free(short_si);

#ifdef WIN32
/* got to remove the target under win32