[mrtg-developers] segmentation fault when kMG option contains more prefixes

[Vitezslav Crhonek]

On 05/16/2011 05:17 PM, Vitezslav Crhonek wrote:
> 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
>

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