Hi, On Sat, Mar 31, 2012 at 10:40 AM, Justin Ruggles <[email protected]> wrote: > On 03/31/2012 01:25 PM, Måns Rullgård wrote: > >> Justin Ruggles <[email protected]> writes: >> >>> On 03/31/2012 01:07 PM, Måns Rullgård wrote: >>> >>>> "Ronald S. Bultje" <[email protected]> writes: >>>> >>>>> Hi, >>>>> >>>>> On Sat, Mar 31, 2012 at 9:18 AM, Justin Ruggles >>>>> <[email protected]> wrote: >>>>>> On 03/29/2012 03:44 PM, Ronald S. Bultje wrote: >>>>>> >>>>>>> From: "Ronald S. Bultje" <[email protected]> >>>>>>> >>>>>>> This prevents sample_rate/data_length from going negative, which >>>>>>> caused various crashes and undefined behaviour further down. >>>>>>> >>>>>>> Found-by: Mateusz "j00ru" Jurczyk and Gynvael Coldwind >>>>>>> CC: [email protected] >>>>>>> --- >>>>>>> libavcodec/tta.c | 8 +++++--- >>>>>>> 1 file changed, 5 insertions(+), 3 deletions(-) >>>>>>> >>>>>>> diff --git a/libavcodec/tta.c b/libavcodec/tta.c >>>>>>> index ad80246..808de04 100644 >>>>>>> --- a/libavcodec/tta.c >>>>>>> +++ b/libavcodec/tta.c >>>>>>> @@ -61,7 +61,8 @@ typedef struct TTAContext { >>>>>>> GetBitContext gb; >>>>>>> const AVCRC *crc_table; >>>>>>> >>>>>>> - int format, channels, bps, data_length; >>>>>>> + int format, channels, bps; >>>>>>> + unsigned data_length; >>>>>>> int frame_length, last_frame_length, total_frames; >>>>>>> >>>>>>> int32_t *decode_buffer; >>>>>>> @@ -253,7 +254,7 @@ static av_cold int tta_decode_init(AVCodecContext * >>>>>>> avctx) >>>>>>> } >>>>>>> >>>>>>> // prevent overflow >>>>>>> - if (avctx->sample_rate > 0x7FFFFF) { >>>>>>> + if (avctx->sample_rate > 0x7FFFFFu) { >>>>>> >>>>>> >>>>>> this change doesn't make sense to me >>>>> >>>>> Samplerate is a 32bit thing in the bitstream header (read in the >>>>> codec, not the demuxer), placed in a signed integer. Therefore, >>>>> 0x80000000 and up are read as negative. Comparing to an unsigned int >>>>> yields an unsigned comparison which errors out on negatives. >>>> >>>> Does it make sense at all for AVCodecContext.sample_rate to be signed? >>> >>> Practically speaking, no. But wouldn't making it unsigned possibly lead >>> to some unexpected implicit casts? >> >> Having it signed is currently leading to unexpected problems like the >> one fixed above. We know this for certain. > > I'm ok with changing it. I just think we need to look through the code > first to see if there is anything that will obviously break. > >>> Also there might be cases where a maximum of INT_MAX is assumed and >>> checked against for overflow prevention. >> >> Does it matter? > > > no, I suppose not.
So is anyone doing this? This is a recurring story where we detect a vulnerability, a fix is submitted, we bikeshed for a while and then nothing actually happens. That's not right. Ronald _______________________________________________ libav-devel mailing list [email protected] https://lists.libav.org/mailman/listinfo/libav-devel
