Bug/Array index out of bounds

[Tilo Renz]

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