Bug/Array index out of bounds

Tilo Renz trenz at tagwork-one.de
Thu Jun 4 14:05:38 CEST 2009


I think the zero-padding is necessary.

> > 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,
> 
> Right. It shouldn't touch the buffer at all before or after 
> writing the
> read data into it.

Line 4488 isn't useless. The buffer should be zero-padded, I'll explain why.

> > but when reading, it tries to fill the buffer
> > up to the end, whithout remaining space for the null-byte.
> 
> What makes you think it should save space for the nul byte 
> (not null, that's
> something else)? Since the size of the received data is 
> returned as well,
> we can safely access all the data we just read. We're reading 
> a bytestream,
> not a string. If the caller expects the bytestream to be 
> nul-terminated,
> that's a bug in the caller, not in the routine.

Yes, it's the callers fault, my_recvall is OK.

> 
> man 2 read
> 
> It works the same, and it's the only sensible way it *can* 
> work. If it all
> of a sudden starts adding nul bytes to the buffer you'll all 
> of a sudden
> end up unable to read binary byte sequences from files and 
> sockets alike.

Yes, all true, but then we must not use the buffer as a string.
Here zero-padding is neccessary.
* Weak argument: the debug-printf in line 4495 indicates developers 
zero-padding intention.
* Hard argument: In line 3504 the buffer is used to call 
get_next_string_from_buf(recv_buf,&buf_index,sizeof(recv_buf)).
While we know the number of valid bytes in the buffer, no one cares.
And even if we change this call to 
get_next_string_from_buf(recv_buf,&buf_index,recv_len), zero-padding 
is still necessary. In get_next_string_from_buf, line 3020, the buffer 
is given to strcspn(buffer, "\n"). Since we can't guarantee 
Newline-Occurence, buffer must be null-terminated or strcspn will 
slurp beyond buffers end.


> 
> > Things like this are the place [..]
> Please refrain from cluttering the internet with such useless 
> flame-inciting
> statements. Especially in conjunction with such erroneous 
> statements as
> those above.

You're right.

> [..]


Regards, Joey5337

------------------------------------------------------------------------------
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




More information about the Developers mailing list