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

[Hendrik Leppkes]

On Sun, Dec 27, 2015 at 9:29 PM, Andreas Cadhalpun
 wrote:
> On 27.12.2015 21:13, Hendrik Leppkes wrote:
>> On Sun, Dec 27, 2015 at 9:03 PM, Andreas Cadhalpun
>>  wrote:
>>> On 27.12.2015 20:43, Hendrik Leppkes wrote:
 On Sun, Dec 27, 2015 at 8:29 PM, Andreas Cadhalpun
  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 = _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, 
>> _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.
>
> Then please fix this bug also in teletext_decode_frame.
>

Pushed with the second fix added and slightly reworded message.

- Hendrik
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

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

[Andreas Cadhalpun]

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 = _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, 
> _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?

Best regards,
Andreas
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

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

[Andreas Cadhalpun]

On 27.12.2015 20:43, Hendrik Leppkes wrote:
> On Sun, Dec 27, 2015 at 8:29 PM, Andreas Cadhalpun
>  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 = _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, 
>>> _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.

> 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@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

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

[Hendrik Leppkes]

On Sun, Dec 27, 2015 at 8:29 PM, Andreas Cadhalpun
 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 = _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, 
>> _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. All timebase checks are for num only.
Its an assert2 for a reason (ie. a developer tool), its checked in an
error condition right after and returns INT64_MIN.
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

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

[Hendrik Leppkes]

Invalid timebases have a zero numerator, not denominator.
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 = _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, _recoded);
-- 
2.6.2.windows.1

___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

Re: [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
 wrote:
> On 27.12.2015 20:43, Hendrik Leppkes wrote:
>> On Sun, Dec 27, 2015 at 8:29 PM, Andreas Cadhalpun
>>  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 = _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, 
 _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@ffmpeg.org
> http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

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

[Andreas Cadhalpun]

On 27.12.2015 21:13, Hendrik Leppkes wrote:
> On Sun, Dec 27, 2015 at 9:03 PM, Andreas Cadhalpun
>  wrote:
>> On 27.12.2015 20:43, Hendrik Leppkes wrote:
>>> On Sun, Dec 27, 2015 at 8:29 PM, Andreas Cadhalpun
>>>  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 = _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, 
> _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.

Then please fix this bug also in teletext_decode_frame.

> The same timebase is checked 5 lines down for num already.

Indeed.

Best regards,
Andreas

___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel