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