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. > Also there might be cases where a maximum of INT_MAX is assumed and > checked against for overflow prevention. Does it matter? -- Måns Rullgård [email protected] _______________________________________________ libav-devel mailing list [email protected] https://lists.libav.org/mailman/listinfo/libav-devel
