[FFmpeg-devel] [PATCH] avcodec/utils: fix check for invalid timebase when decoding subtitles

[Hendrik Leppkes]

On Sun, Dec 27, 2015 at 9:03 PM, Andreas Cadhalpun
<andreas.cadhalpun at googlemail.com> wrote:
> On 27.12.2015 20:43, Hendrik Leppkes wrote:
>> On Sun, Dec 27, 2015 at 8:29 PM, Andreas Cadhalpun
>> <andreas.cadhalpun at googlemail.com> wrote:
>>> On 27.12.2015 20:10, Hendrik Leppkes wrote:
>>>> Invalid timebases have a zero numerator, not denominator.
>>>
>>> A timebase with zero numerator is probably invalid, but a timebase
>>> with zero denominator is not even well defined.
>>> So this comment doesn't seem quite right.
>>>
>>>> Fixes a integer divison by zero.
>>>> ---
>>>>  libavcodec/utils.c | 2 +-
>>>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>>>
>>>> diff --git a/libavcodec/utils.c b/libavcodec/utils.c
>>>> index 19f3f0a..33295ed 100644
>>>> --- a/libavcodec/utils.c
>>>> +++ b/libavcodec/utils.c
>>>> @@ -2435,7 +2435,7 @@ int avcodec_decode_subtitle2(AVCodecContext *avctx, AVSubtitle *sub,
>>>>          } else {
>>>>              avctx->internal->pkt = &pkt_recoded;
>>>>
>>>> -            if (avctx->pkt_timebase.den && avpkt->pts != AV_NOPTS_VALUE)
>>>> +            if (avctx->pkt_timebase.num && avpkt->pts != AV_NOPTS_VALUE)
>>>>                  sub->pts = av_rescale_q(avpkt->pts,
>>>>                                          avctx->pkt_timebase, AV_TIME_BASE_Q);
>>>>              ret = avctx->codec->decode(avctx, sub, got_sub_ptr, &pkt_recoded);
>>>>
>>>
>>> If avctx->pkt_timebase.den is 0, calling av_rescale_q here will trigger the
>>> av_assert2(c > 0) in av_rescale_rnd, so removing this check isn't good.
>>>
>>> Why not check for both num and den?
>>>
>>
>> We never check both, invalid timebase is {0, 1}, anything else needs
>> to have values in both.
>
> I'd call this unknown timebase, as {0, 1} is the initialization value.
> A {1, 0} timebase is certainly not valid.
>
>> All timebase checks are for num only.
>
> The check here was for den and there is a similar check in teletext_decode_frame.

And thats a bug since that can actually lead to div by 0 and crash.
The same timebase is checked 5 lines down for num already.

>
>> Its an assert2 for a reason (ie. a developer tool), its checked in an
>> error condition right after and returns INT64_MIN.
>
> It's an assert, because it should never happen.
>
> If the check for den here was intended to prevent triggering such an assert,
> then it would still be needed.
> If on the other hand this check was meant to check for an uninitialized
> pkt_timebase, then removing it would be fine.
>
> So you can remove this check, if you're sure it's the second case.
> But please clarify this in the commit message.
>
> Best regards,
> Andreas
>
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel at ffmpeg.org
> http://ffmpeg.org/mailman/listinfo/ffmpeg-devel