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.
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.
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".
This is really lame code, if you'll excuse the pun.
Somebody should run some tests that pass randomly corrupted packets to libmad and catch all of the segmentation faults that libmad will generate.
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.
MAD.
On Oct 18, 2006, at 2:57 AM, mad@zacglen.com 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.
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.
All it needs is corrupted frame parameters! Anything that is driven by data content in determining lengths should be more stringently checked.
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.
The checks on a macro-scale are not adequate. The segmentation errors do occur - that is proof enough.
The required checks are quite slight, and like this:
if (bitptr->byte < bitptr->bufend) bitptr->byte++;
The cost is close to zero.
Or, if you were genuinely concerned about pointers going beyond the end of buffer you could put some more assertions in your code.
The only downside to this type of check is that it inhibits segmentation errors but doesn't return error to caller. Instead it replicates the final byte.
Also, as I said, if you want examples you would be better off taking a good packet and randomly corrupting it and feeding it to your test engine (random can be repeatable by choosing fixed seed).
If you have one, please send me a test case that causes a segfault.
I have patched libmad so that it checks for buffer overflow.
I cannot produce the segfault any more. And I have also deleted the file (it was an 11GB video file and the segfault was at about the 10GB mark).
You really should take a more pro-active approach to testing. One way is to read your own code and make changes that guarantee that buffer overflows cannot possibly happen. You shouldn't wait for users to report segmentation errors and fix them on a case-by-case basis.
The other way is to generate randcom bad frames and see how your code behaves. What is so difficult about that?
On Wed, Oct 18, 2006 at 03:39:09PM -0700, Rob Leslie wrote:
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.
Please see http://bugs.debian.org/287519
Kurt
-
Kurt Roeckx -
mad＠zacglen.com -
Rob Leslie