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. -Justin _______________________________________________ libav-devel mailing list [email protected] https://lists.libav.org/mailman/listinfo/libav-devel
