[Xymon] Possible bug in xymon svcstatus causing segfault (buffer overrun?)

[Jeremy Laidman]

Hi all

I'm not a C programmer by any stretch, so I'm seeking confirmation that my
working is sound.

At our site we run 4.3.30-1 from Terabithia RPM. For most hosts, the
"trends" page displays just fine, but for some hosts, the webserver gives a
CGI error, and when I manually run the CGI directly it crashes with a seg
fault. This seems to be more likely for hosts with a lot of entries on the
trends page, but some with more trends graphs are OK than others with fewer.

I believe I've tracked the issue down to the last few lines of code in
svcstatus-trends.c, where a URL is constructed for one graph entry, before
being added to the list of trends in a buffer.

My reading of the code suggests that it first pre-allocates (using
malloc()) a 16k buffer for the trends data, then sequentially appends trend
strings (which include a URL) into the buffer, until the length of a string
would exceed the size of the buffer. In that case, an extra 4k of memory is
added to the buffer (using realloc()), and then the string is added to the
buffer.

The problem with the code, as I see it, is that the string might be more
than 4k long, in which case, adding string would cause a buffer overrun. I
don't really know what's in each string, so I don't know if it's ever
likely to be 4k, but we're seeing the process dumping core immediately
after this bit of code, so that's the suspicion.

Here's the bit of code, which is attempting to add the string "onelink" to
the buffer, after seeing if it's going to exceed the buffer, and extending
the buffer if required:

                        if ((buflen + strlen(onelink)) >=
allrrdlinks_buflen) {
                                SBUF_REALLOC(allrrdlinks,
allrrdlinks_buflen+4096);
                                allrrdlinksend = allrrdlinks + buflen;
                        }
                        allrrdlinksend += snprintf(allrrdlinksend,
(allrrdlinks_buflen - (allrrdlinksend - allrrdlinks)), "%s", onelink);

Elsewhere in other code, the SBUF_REALLOC macro is used to add extra
bufferspace by calculating a new buffer size of [existing buffer size plus
4k plus the length of the string being added]. So if the string to be added
had 20 bytes, SBUF_REALLOC would be used to add 4k+20 bytes to the buffer
size. Notably, the size of the string is included in the calculation. For
example this bit of code elsewhere in the same file does a similar buffer
extension:

                        if ((strlen(rrdlink) + strlen(partlink) + 1) >=
rrdlink_buflen) {
                                SBUF_REALLOC(rrdlink, rrdlink_buflen +
strlen(partlink) + 4096);
                        }
                        strncat(rrdlink, partlink, (rrdlink_buflen -
strlen(rrdlink)));

Note the extra "+ strlen(<stringvar>)" bit in the extra memory being
allocated that isn't there in the first block of code above. So my thinking
is that the SBUF_REALLOC() needs to take the string's size into account,
like so:

                                SBUF_REALLOC(allrrdlinks,
allrrdlinks_buflen+strlen(onelink)+4096);

Is anyone with C skills able to confirm that this does indeed look like a
bug, and also that my proposed change is a suitable fix? After applying
this change and recompiling, we haven't yet seen a seg fault from this bit
of code, so I'm hoping it's all fixed, and without negative side effects.

Cheers
Jeremy
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.xymon.com/pipermail/xymon/attachments/20230830/decc90c5/attachment.htm>