Mario
Attached is a diff to the file that I used. It's against version 4.3.30.
Cheers Jeremy
On Sat, 1 Jun 2024 at 06:29, Mario <rower.master at gmail.com> wrote:
Hello Jeremy/JC,
I?m facing the same issue with switches interfaces graphs not loading in trends or causing internal server error. Do you have a patch to fix this? Or maybe I need to downgrade xymon 4.3.30 version?
Thanks & regards, Mario
On Wed, Sep 13, 2023 at 4:59?AM Jeremy Laidman <jeremy at laidman.org> wrote:
Ugh, just saw your reply in my spam folder!
On Fri, 1 Sept 2023 at 23:40, J.C. Cleaver <cleaver at terabithia.org> wrote:
On Tue, August 29, 2023 23:40, Jeremy Laidman wrote:
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
Based on my looking over it, I would confirm that this seems to be a bug. Specifically, it looks like we missed this part of the fix in https://sourceforge.net/p/xymon/code/8069/?page=7#diff-1 while trying to get ahead of possible buffer overflows for various CVE's (https://nvd.nist.gov/vuln/detail/CVE-2019-13486, I suppose). I can't see a reason not to add the length in here without a guard somewhere else for the length.
Furthermore, other uses of this macro pretty much all include the length of the incoming string or limit the concat to a specific size, which obviously isn't ideal for URLs here.
If you have a reproducer and your patch fixes it, I suspect that will be sufficient and we can add it in. Can you send a coredump from a crash off list by any chance (using the debuginfo RPMs) just to double-check?
Sorry, can you define "reproducer"? Is that a coding term I haven't yet come across?
I have core dumps, but I'll see about using the debuginfo RPMs (assuming they're on Terabithia) and get a core dump from an install of that.
Thanks
Xymon mailing list Xymon at xymon.com http://lists.xymon.com/mailman/listinfo/xymon