Re: Segmentation fauls because libmad doesn't check for buffer overflows

On Oct 18, 2006, at 2:57 AM, [email protected] wrote:
> On a couple of occasions I have found that libmad (v0.15.0b)  
> generates segmentation errors because it doesn't really bother  
> checking whether the end of buffer has been reached.

If you have an example case where this problem is occurring, I'd be  
very interested to see it.

> That is a real worry, because one of the first thing one usually  
> does when writing robust code is ensure that wherever a pointer is  
> used it is never allowed to go beyond available data.

In most (hopefully all) cases this is indeed ensured. For example, a  
check is performed to ensure the full contents of a frame are present  
before any attempt is made to decode it (see libmad/frame.c lines  
402-408).

> Look at function mad_layer_II() for example.
>
> Here we have a "struct mad_stream" containing "buffer", "bufend",  
> and "ptr" pointers.
>
> Then just a few lines later, about line 382, we have the following  
> code:
>
>     for (ch = 0; ch < nch; ++ch)
>           allocation[ch][sb] = mad_bit_read(&stream->ptr, nbal);
>
> Unbelievably just the ptr is passed to mad_bit_read().  This means  
> that mad_bit_read() can, and sometimes does, increment the "ptr"  
> beyond "bufend".

Again, I'd be very interested in a case where this actually happens.  
The amount of data read here is fixed and predetermined by the frame  
parameters (bitrate, mode, and sample rate) which also determine the  
length of the frame.

> One solution to above problem would be to created a bufend ptr in  
> struct mad_bitptr (ie. mad_bit_init() should be passed buffer and  
> bufend). Or just rationalize lots of the code by joining buffer/ 
> bufend into a new struct and never using a anything but that struct  
> everywhere.

While certainly this is possible, there is always a tradeoff with  
performance. Having already performed buffer length checks on a macro  
scale, performing them again on a micro scale tends to gain very little.

-- 
Rob Leslie
[email protected]