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