Hi,
Michael Niedermayer wrote:
On Thu, Jun 26, 2008 at 07:26:20PM +0100, Ramiro Polla wrote:
Hello,
ramiro wrote:
Author: ramiro
Date: Thu Jun 26 19:33:28 2008
New Revision: 2578
Log:
More checks to see if there is enough data.
Modified:
mlp/mlpdec.c
Modified: mlp/mlpdec.c
==============================================================================
--- mlp/mlpdec.c (original)
+++ mlp/mlpdec.c Thu Jun 26 19:33:28 2008
@@ -1009,6 +1009,9 @@ static int read_access_unit(AVCodecConte
for (substr = 0; substr < m->num_substreams; substr++) {
int extraword_present, checkdata_present, end;
+ if (bytes_left < 2)
+ return -1;
+
extraword_present = get_bits1(&gb);
skip_bits1(&gb);
checkdata_present = get_bits1(&gb);
@@ -1022,6 +1025,8 @@ static int read_access_unit(AVCodecConte
bytes_left -= 2;
if (extraword_present) {
+ if (bytes_left < 2)
+ return -1;
skip_bits(&gb, 16);
parity_bits ^= *buf++;
parity_bits ^= *buf++;
This seems like a long and painful road... While we're still reading
headers with easily calculated size, this isn't too hard, and there
won't be much overhead in the checks. But on more complicated headers
and specially the VLC values it's not easy to see if we still have
enough data.
Lots of other codecs I've looked at seem to just trust init_get_bits()
with the remaining bytes.
What's the best practice here? Should the header checksum && coded
lengths be enough to validate the input size?
All _writes_ must be checked, the reads are not critical.
also dont forget FF_INPUT_BUFFER_PADDING_SIZE
Ok. So is it ok to apply these patches?
revert_1 just revert the last commit of extra checks.
revert_2 removes keeping track of bytes left. the value is never used
later on the code anyways. coded values which pass the checksum are trusted.
about other codecs
mpeg & h26* are designed so 23 zero bits can never occur in a valid bitstream
that way the 64 zero bit at the end are guranteed to trigger some kind of
error.
Oh, so they're guaranteed to be zeroes? I'll have to take another look
at the code with this new information in mind. MLP doesn't seem to be so
well designed anyways.
I definitly do not want the code to be salted with input buffer checks in
every second line!
Too much salt is bad for our health =)
Ramiro Polla
Index: mlpdec.c
===================================================================
--- mlpdec.c (revision 2578)
+++ mlpdec.c (working copy)
@@ -1009,9 +1009,6 @@
for (substr = 0; substr < m->num_substreams; substr++) {
int extraword_present, checkdata_present, end;
- if (bytes_left < 2)
- return -1;
-
extraword_present = get_bits1(&gb);
skip_bits1(&gb);
checkdata_present = get_bits1(&gb);
@@ -1025,8 +1022,6 @@
bytes_left -= 2;
if (extraword_present) {
- if (bytes_left < 2)
- return -1;
skip_bits(&gb, 16);
parity_bits ^= *buf++;
parity_bits ^= *buf++;
--- mlpdec.c.orig 2008-06-27 21:59:34.000000000 +0100
+++ mlpdec.c 2008-06-27 22:19:16.000000000 +0100
@@ -967,7 +967,7 @@
{
MLPDecodeContext *m = avctx->priv_data;
GetBitContext gb;
- unsigned int length, substr, bytes_left;
+ unsigned int length, substr;
unsigned int substream_start;
unsigned int header_size = 4;
uint8_t substream_parity_present[MAX_SUBSTREAMS];
@@ -978,23 +978,21 @@
if (buf_size < 4)
return 0;
- bytes_left = length = (AV_RB16(buf) & 0xfff) * 2;
+ length = (AV_RB16(buf) & 0xfff) * 2;
if (length > buf_size)
return -1;
for(i = 0; i < 4; i++)
parity_bits ^= *buf++;
- bytes_left -= 4;
- init_get_bits(&gb, buf, bytes_left * 8);
+ init_get_bits(&gb, buf, (length - 4) * 8);
if (show_bits_long(&gb, 31) == (0xf8726fba >> 1)) {
dprintf(m->avctx, "Found major sync\n");
if (read_major_sync(m, &gb) < 0)
goto error;
header_size += 28;
- bytes_left -= 28;
buf += 28;
}
@@ -1019,14 +1017,12 @@
parity_bits ^= *buf++;
parity_bits ^= *buf++;
header_size += 2;
- bytes_left -= 2;
if (extraword_present) {
skip_bits(&gb, 16);
parity_bits ^= *buf++;
parity_bits ^= *buf++;
header_size += 2;
- bytes_left -= 2;
}
if (end + header_size > length) {
@@ -1129,7 +1125,6 @@
}
buf += substream_data_len[substr];
- bytes_left -= substream_data_len[substr];
}
rematrix_channels(m, substr - 1);
_______________________________________________
FFmpeg-soc mailing list
[email protected]
https://lists.mplayerhq.hu/mailman/listinfo/ffmpeg-soc
