Segfault in confreport-critical.sh / confreport.cgi in 4.3.24
Hi,
Via corekeeper (https://packages.debian.org/stable/corekeeper) I became aware of a segfault of confreport.cgi. I'm able to reproduce it (at least) in a fresh browser instance (all cookies removed), going to "Find host", searching for a non-existing host and then clicking on "Config Report (Critical)". It says "172 hosts included" (i.e. not just a single host as it happens if I searched for an existing host before) and then outputs most of the page, but ends in the middle of the alerts listing at line 8049 in the middle of a table:
<tr><td><font COLOR="#000000" FACE="Tahoma, Arial, Helvetica">079xxxxxxx at example.com (R)</font></td><td align=center>5m 1s </td><td align=center>-</td><td align=center>4h </td><td align=center>-</td><td>purple</td></tr> <tr><td valign=top rowspan=4 >ports</td><td><font COLOR="#000000" FACE="Tahoma, Arial, Helvetica">0765
At least in one of the cases it ended in the same line, but with "076 instead of 0765". So I suspect we hit some C string size limit once again.
I don't have a proper backtrace (yet), but I had look at the source code and I'm quite confident that the issue is inside the function print_alert_recipients() starting at lib/loadalerts.c, line 1124.
I suspect it's an overflow of the variable "buf". "l" seems large enough with 4kB.
Kind regards, Axel Beckert-- Axel Beckert <beckert at phys.ethz.ch> support: +41 44 633 26 68 IT Services Group, HPT H 6 voice: +41 44 633 41 89 Departement of Physics, ETH Zurich CH-8093 Zurich, Switzerland http://nic.phys.ethz.ch/
Hi,
On Thu, Dec 10, 2015 at 01:45:49PM +0100, Axel Beckert wrote:
I don't have a proper backtrace (yet), but I had look at the source code and I'm quite confident that the issue is inside the function print_alert_recipients() starting at lib/loadalerts.c, line 1124.
Here's the according backtrace. Doesn't look too helpful to me, though:
gdb /usr/lib/xymon/server/bin/confreport.cgi ./207855-33-33-11-1449754874-silberspitz--usr-lib-xymon-server-bin-confreport.cgi.core
GNU gdb (Debian 7.7.1+dfsg-5) 7.7.1 [...] Reading symbols from /usr/lib/xymon/server/bin/confreport.cgi...Reading symbols from /usr/lib/debug/.build-id/f2/5c37643ea6a391c09535cc250d18cef4aa39cf.debug...done. done. [New LWP 207855] [Thread debugging using libthread_db enabled] Using host libthread_db library "/lib/x86_64-linux-gnu/libthread_db.so.1". Core was generated by `/usr/lib/xymon/server/bin/confreport.cgi --env=/usr/lib/xymon/server/etc/xymons'. Program terminated with signal SIGSEGV, Segmentation fault. #0 strlen () at ../sysdeps/x86_64/strlen.S:106 106 ../sysdeps/x86_64/strlen.S: No such file or directory. (gdb) bt #0 strlen () at ../sysdeps/x86_64/strlen.S:106 #1 0x0000000000403640 in print_host (testcount=18, testnames=0x167dcc0, host=0x166b070) at confreport.c:310 #2 main (argc=<optimized out>, argv=<optimized out>) at confreport.c:876
Line 310 of web/confreport.c looks liek this:
newitem->visualdata = (char *)realloc(newitem->visualdata, strlen(newitem->visualdata) + strlen(visdata) + 5);
So do I read the backtrace correctly that there was no proper value (e.g. a null pointer) was passed to strlen()?
Then again I also don't find any file named strlen.S on the system where I build the packages either. (But it seems to be part of glibc's source code.)
Kind regards, Axel Beckert-- Axel Beckert <beckert at phys.ethz.ch> support: +41 44 633 26 68 IT Services Group, HPT H 6 voice: +41 44 633 41 89 Departement of Physics, ETH Zurich CH-8093 Zurich, Switzerland http://nic.phys.ethz.ch/
On Thu, December 10, 2015 5:59 am, Axel Beckert wrote:
Hi,
On Thu, Dec 10, 2015 at 01:45:49PM +0100, Axel Beckert wrote:
I don't have a proper backtrace (yet), but I had look at the source code and I'm quite confident that the issue is inside the function print_alert_recipients() starting at lib/loadalerts.c, line 1124.
Here's the according backtrace. Doesn't look too helpful to me, though:
gdb /usr/lib/xymon/server/bin/confreport.cgi
./207855-33-33-11-1449754874-silberspitz--usr-lib-xymon-server-bin-confreport.cgi.core GNU gdb (Debian 7.7.1+dfsg-5) 7.7.1 [...] Reading symbols from /usr/lib/xymon/server/bin/confreport.cgi...Reading symbols from /usr/lib/debug/.build-id/f2/5c37643ea6a391c09535cc250d18cef4aa39cf.debug...done. done. [New LWP 207855] [Thread debugging using libthread_db enabled] Using host libthread_db library "/lib/x86_64-linux-gnu/libthread_db.so.1". Core was generated by `/usr/lib/xymon/server/bin/confreport.cgi --env=/usr/lib/xymon/server/etc/xymons'. Program terminated with signal SIGSEGV, Segmentation fault. #0 strlen () at ../sysdeps/x86_64/strlen.S:106 106 ../sysdeps/x86_64/strlen.S: No such file or directory. (gdb) bt #0 strlen () at ../sysdeps/x86_64/strlen.S:106 #1 0x0000000000403640 in print_host (testcount=18, testnames=0x167dcc0, host=0x166b070) at confreport.c:310 #2 main (argc=<optimized out>, argv=<optimized out>) at confreport.c:876
Line 310 of web/confreport.c looks liek this:
newitem->visualdata = (char *)realloc(newitem->visualdata, strlen(newitem->visualdata) + strlen(visdata) + 5);
So do I read the backtrace correctly that there was no proper value (e.g. a null pointer) was passed to strlen()?
Then again I also don't find any file named strlen.S on the system where I build the packages either. (But it seems to be part of glibc's source code.)
Kind regards, Axel Beckert
Hmm,
Well, that's different from loadalerts.c, but there does seem to be a problem there.
Can you see if this patch helps?
-jc
Hi JC, On Thu, Dec 10, 2015 at 08:23:23AM -0800, J.C. Cleaver wrote:
On Thu, December 10, 2015 5:59 am, Axel Beckert wrote:
On Thu, Dec 10, 2015 at 01:45:49PM +0100, Axel Beckert wrote:
I don't have a proper backtrace (yet), but I had look at the source code and I'm quite confident that the issue is inside the function print_alert_recipients() starting at lib/loadalerts.c, line 1124.
Here's the according backtrace. Doesn't look too helpful to me, though: [...] #1 0x0000000000403640 in print_host (testcount=18, testnames=0x167dcc0, host=0x166b070) at confreport.c:310 [...] Well, that's different from loadalerts.c,
Indeed. My first guess was solely based on where the interrupted HTML output comes from.
but there does seem to be a problem there.
Yeah, I figured in the meanwhile that visdata likely was NULL to cause that crash. I was just not sure which places would all need changes to fix that.
Can you see if this patch helps?
Will check. Thanks for the patch!
Index: web/confreport.c =================================================================== --- web/confreport.c (revision 7835) +++ web/confreport.c (working copy) @@ -288,9 +288,10 @@ } else if (is_net_test(itm)) { colname = strdup(itm); + visdata = strdup(""); }
- + if (!visdata) visdata = strdup("");
Those two additionans look a little bit redundant, but that shouldn't cause any harm. (I'd say the first addition shouldn't be necessary if the second one is present.)
- newitem->visualdata = (char *)realloc(newitem->visualdata, strlen(newitem->visualdata) + strlen(visdata) + 5); + newitem->visualdata = newitem->visualdata ? + (char *)realloc(newitem->visualdata, strlen(newitem->visualdata) + strlen(visdata) + 5) : + (char *)malloc(strlen(visdata) + 5);
That's the place where I thought, it might need changes, too, but I had no ideas which exactly. Kind regards, Axel Beckert -- Axel Beckert <beckert at phys.ethz.ch> support: +41 44 633 26 68 IT Services Group, HPT H 6 voice: +41 44 633 41 89 Departement of Physics, ETH Zurich CH-8093 Zurich, Switzerland http://nic.phys.ethz.ch/
Hi JC,
On Thu, Dec 10, 2015 at 08:23:23AM -0800, J.C. Cleaver wrote:
Can you see if this patch helps?
The patched helped, thanks! Can't reproduce the issue anymore.
Kind regards, Axel Beckert-- Axel Beckert <beckert at phys.ethz.ch> support: +41 44 633 26 68 IT Services Group, HPT H 6 voice: +41 44 633 41 89 Departement of Physics, ETH Zurich CH-8093 Zurich, Switzerland http://nic.phys.ethz.ch/
On Thu, December 10, 2015 8:53 am, Axel Beckert wrote:
Hi JC,
On Thu, Dec 10, 2015 at 08:23:23AM -0800, J.C. Cleaver wrote:
Can you see if this patch helps?
The patched helped, thanks! Can't reproduce the issue anymore.
Perfect, thanks! -jc (From earlier)
--- web/confreport.c (revision 7835) +++ web/confreport.c (working copy) @@ -288,9 +288,10 @@ } else if (is_net_test(itm)) { colname = strdup(itm); + visdata = strdup(""); }
- + if (!visdata) visdata = strdup("");
Those two additionans look a little bit redundant, but that shouldn't cause any harm. (I'd say the first addition shouldn't be necessary if the second one is present.)
That's correct. I added that primarily because of the structures above it (which all set visdata), and because I wasn't quite sure of ultimate source of some of this, and what was liable to fall through or not.
participants (2)
-
beckert＠phys.ethz.ch
-
cleaver＠terabithia.org