Bug/Array index out of bounds
Tilo Renz
trenz at tagwork-one.de
Wed Jun 3 17:38:58 CEST 2009
Hi all,
Concerning the array index out of bounds in nagios-3.1.0/base/utils.c:4488 I investigated a bit further.
Original code is:
4485: /* get response */
4486: recv_len=sizeof(recv_buf);
4487: result=my_recvall(sd,recv_buf,&recv_len,2);
4488: recv_buf[sizeof(recv_buf)]='\x0';
Not only in 4488 a Nullbyte is written beyond the limit of recv_buf, its also zero-padding at the wrong place. After calling my_recvall recv_len contains the number of received bytes. Zero-Padding at the buffer-end would leave undefined content beginning at recv_buf[recv_len].
A fix should look something like this:
4485: /* get response */
4486: recv_len=sizeof(recv_buf)-1;
4487: result=my_recvall(sd,recv_buf,&recv_len,2);
4488: recv_buf[recv_len]='\x0';
By the way: before reading anything at all my_recvall needlessly zeroes out the complete recv_buf, but when reading, it tries to fill the buffer up to the end, whithout remaining space for the null-byte.
Things like this are the place where sniffy C-programmers gamble away the negligible, but always so upholded performance advantage of their error prone low level language.
Code smell, but positive after all: my_recvall is a not used anywhere else.
During static code analysis of the nagios code I found two additional bugs in nagios:
* memory leak in nagios-3.1.0/cgi/cgiutils.c:1146
If the function is left in line 1146, the Memory associated with new_mmapfile, allocated in line 1141 is lost.
One can argue memory leaks in cgi arent that harmful, because the program is short-running and the OS will do the garbage Collection for us. Also, if opening the file fails, results may be fubar anyway.
On the other hand leaking memory always causes bad karma.
* Same issue in nagios-3.1.0/cgi/statuswrl.c:932
Memory associated with vrml_safe_hostname, allocated in line 921 will be lost.
Analyzing the code from ndoutils-1.4b7 I found another error.
In ndoutils-1.4b7/src/ndo2db.c:625 _one_ childprocess-status is cleared.
But before the signalhandler is executed another child may have finished its job.
There won't be a second signal for it, as a SIGCHLD is already pending.
One of the two child processes will remain an uncleared zombie until somebody terminates the ndo2db-daemon.
Possible Fix:
current code:
623 /* cleanup children that exit, so we don't have zombies */
624 if(sig==SIGCHLD){
625 waitpid(-1,NULL,WNOHANG);
626 return;
627 }
should become something like:
623 /* cleanup children that exit, so we don't have zombies */
624 if(sig==SIGCHLD){
625 while( waitpid(-1,NULL,WNOHANG)>0 ) ;
626 return;
627 }
Nagios itself does not contain this flaw. Most times waitpid is called with an explicit pid and without WNOHANG. In events.c:988 waitpid(-1,NULL,WNOHANG) is called, but protected with the suggested while-loop.)
Regards, Joey5337
--
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _
E-Mail: trenz at tagwork-one.de | Fon +49(731)28064320
tagwork<one> Technology Services GmbH
Lise-Meitner-Str. 13 | Science-Park-II | D-89081 Ulm
Fon +49(731)28064320 | Fax +49(731)28064329
Website www.tagwork-one.de | E-Mail: info at tagwork-one.de
Registergericht: Amtsgericht Ulm
Registernummer: HRB 720166 | Sitz Ulm
Geschäftsführer: Gerhard Strehle
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _
------------------------------------------------------------------------------
OpenSolaris 2009.06 is a cutting edge operating system for enterprises
looking to deploy the next generation of Solaris that includes the latest
innovations from Sun and the OpenSource community. Download a copy and
enjoy capabilities such as Networking, Storage and Virtualization.
Go to: http://p.sf.net/sfu/opensolaris-get
_______________________________________________
Nagios-devel mailing list
Nagios-devel at lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/nagios-devel
More information about the Developers
mailing list