Re: [FFmpeg-devel] [PATCH] avformat: remove request_probe assert from ff_read_packet

[Andreas Cadhalpun]

On 19.10.2016 22:45, Michael Niedermayer wrote:
> On Wed, Oct 19, 2016 at 07:27:59PM +0200, Andreas Cadhalpun wrote:
>> f1f2b8b10efdab27848eeb2e2bac646eda08a175  
>> 0001-avformat-prevent-triggering-request_probe-assert-in-.patch
>> From 7912c6f200a37130844221a73941a7971afa6455 Mon Sep 17 00:00:00 2001
>> From: Andreas Cadhalpun 
>> Date: Wed, 19 Oct 2016 19:23:49 +0200
>> Subject: [PATCH] avformat: prevent triggering request_probe assert in
>>  ff_read_packet
>>
>> If probe_codec is called with pkt == NULL, it sets probe_packets to 0
>> and request_probe to -1.
>> However, request_probe can change when calling s->iformat->read_packet
>> and thus a probe_packets value of 0 doesn't guarantee a request_probe
>> value of -1.
>> In that case calling probe_codec again is necessary to prevent
>> triggering the assert.
>>
>> Signed-off-by: Andreas Cadhalpun 
>> ---
>>  libavformat/utils.c | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> LGTM

Pushed.

Best regards,
Andreas

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

Re: [FFmpeg-devel] [PATCH] avformat: remove request_probe assert from ff_read_packet

[Andreas Cadhalpun]

On 19.10.2016 05:29, Michael Niedermayer wrote:
> hmm, i guess the patch is then ok
> alternatively you could use i think:
> @@ -803,7 +803,7 @@ int ff_read_packet(AVFormatContext *s, AVPacket *pkt)
>  return ret;
>  for (i = 0; i < s->nb_streams; i++) {
>  st = s->streams[i];
> -if (st->probe_packets)
> +if (st->probe_packets || st->request_probe > 0)
>  if ((err = probe_codec(s, st, NULL)) < 0)
>  return err;
>  av_assert0(st->request_probe <= 0);

Yes, this works fine and should guarantee that the assert can't be triggered.
Patch doing it that way is attached.

Best regards,
Andreas

>From 7912c6f200a37130844221a73941a7971afa6455 Mon Sep 17 00:00:00 2001
From: Andreas Cadhalpun 
Date: Wed, 19 Oct 2016 19:23:49 +0200
Subject: [PATCH] avformat: prevent triggering request_probe assert in
 ff_read_packet

If probe_codec is called with pkt == NULL, it sets probe_packets to 0
and request_probe to -1.
However, request_probe can change when calling s->iformat->read_packet
and thus a probe_packets value of 0 doesn't guarantee a request_probe
value of -1.
In that case calling probe_codec again is necessary to prevent
triggering the assert.

Signed-off-by: Andreas Cadhalpun 
---
 libavformat/utils.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/libavformat/utils.c b/libavformat/utils.c
index 8a51aea..70dbfa1 100644
--- a/libavformat/utils.c
+++ b/libavformat/utils.c
@@ -803,7 +803,7 @@ int ff_read_packet(AVFormatContext *s, AVPacket *pkt)
 return ret;
 for (i = 0; i < s->nb_streams; i++) {
 st = s->streams[i];
-if (st->probe_packets)
+if (st->probe_packets || st->request_probe > 0)
 if ((err = probe_codec(s, st, NULL)) < 0)
 return err;
 av_assert0(st->request_probe <= 0);
-- 
2.9.3

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

Re: [FFmpeg-devel] [PATCH] avformat: remove request_probe assert from ff_read_packet

[Michael Niedermayer]

On Tue, Oct 18, 2016 at 11:26:24PM +0200, Andreas Cadhalpun wrote:
> On 18.10.2016 22:56, Michael Niedermayer wrote:
> > On Tue, Oct 18, 2016 at 10:31:37PM +0200, Andreas Cadhalpun wrote:
> >> Nothing guarantees to set request_probe to -1, so this assert can be
> >> triggered, e.g. if st->probe_packets is 0.
> > 
> > probe_codec() called with NULL should cause
> > st->probe_packets = 0
> > st->request_probe = -1;
> 
> Yes, but request_probe can be change to a different value later on,
> e.g. in ff_parse_mpeg2_descriptor:
> 
> int ff_read_packet(AVFormatContext *s, AVPacket *pkt)
> {
> [...]
> if (s->internal->raw_packet_buffer_remaining_size <= 0)
> if ((err = probe_codec(s, st, NULL)) < 0) // probe_packets = 
> 0, request_probe = -1
> return err;
> [...]
> ret = s->iformat->read_packet(s, pkt);
> ~~~
> ff_parse_mpeg2_descriptor([...])
> {
> [...]
> switch (desc_tag) {
> [...]
> case 0x05: /* registration descriptor */
> [...]
> st->request_probe = 50;
> [...]
> }
> ~~~
> [...]
> if (st->probe_packets) // still 0
> if ((err = probe_codec(s, st, NULL)) < 0)
> return err;
> av_assert0(st->request_probe <= 0); // now 50
> SIGABRT

hmm, i guess the patch is then ok
alternatively you could use i think:
@@ -803,7 +803,7 @@ int ff_read_packet(AVFormatContext *s, AVPacket *pkt)
 return ret;
 for (i = 0; i < s->nb_streams; i++) {
 st = s->streams[i];
-if (st->probe_packets)
+if (st->probe_packets || st->request_probe > 0)
 if ((err = probe_codec(s, st, NULL)) < 0)
 return err;
 av_assert0(st->request_probe <= 0);


[...]

-- 
Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

Never trust a computer, one day, it may think you are the virus. -- Compn


signature.asc
Description: Digital signature
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

Re: [FFmpeg-devel] [PATCH] avformat: remove request_probe assert from ff_read_packet

[Andreas Cadhalpun]

On 18.10.2016 23:46, Hendrik Leppkes wrote:
> On Tue, Oct 18, 2016 at 11:26 PM, Andreas Cadhalpun
>  wrote:
>> On 18.10.2016 22:56, Michael Niedermayer wrote:
>>> On Tue, Oct 18, 2016 at 10:31:37PM +0200, Andreas Cadhalpun wrote:
 Nothing guarantees to set request_probe to -1, so this assert can be
 triggered, e.g. if st->probe_packets is 0.
>>>
>>> probe_codec() called with NULL should cause
>>> st->probe_packets = 0
>>> st->request_probe = -1;
>>
>> Yes, but request_probe can be change to a different value later on,
>> e.g. in ff_parse_mpeg2_descriptor:
>>
>> int ff_read_packet(AVFormatContext *s, AVPacket *pkt)
>> {
>> [...]
>> if (s->internal->raw_packet_buffer_remaining_size <= 0)
>> if ((err = probe_codec(s, st, NULL)) < 0) // probe_packets = 
>> 0, request_probe = -1
>> return err;
>> [...]
>> ret = s->iformat->read_packet(s, pkt);
>> ~~~
>> ff_parse_mpeg2_descriptor([...])
>> {
>> [...]
>> switch (desc_tag) {
>> [...]
>> case 0x05: /* registration descriptor */
>> [...]
>> st->request_probe = 50;
>> [...]
>> }
>> ~~~
>> [...]
>> if (st->probe_packets) // still 0
>> if ((err = probe_codec(s, st, NULL)) < 0)
>> return err;
>> av_assert0(st->request_probe <= 0); // now 50
>> SIGABRT
>>
> 
> Can you actually make that happen, or is that just speculation?

Yes, at least in ffmpeg 3.1.4 and master with commit 04fa20d reverted.
(I do fuzz-testing, not speculating.)

Best regards,
Andreas

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

Re: [FFmpeg-devel] [PATCH] avformat: remove request_probe assert from ff_read_packet

[Hendrik Leppkes]

On Tue, Oct 18, 2016 at 11:26 PM, Andreas Cadhalpun
 wrote:
> On 18.10.2016 22:56, Michael Niedermayer wrote:
>> On Tue, Oct 18, 2016 at 10:31:37PM +0200, Andreas Cadhalpun wrote:
>>> Nothing guarantees to set request_probe to -1, so this assert can be
>>> triggered, e.g. if st->probe_packets is 0.
>>
>> probe_codec() called with NULL should cause
>> st->probe_packets = 0
>> st->request_probe = -1;
>
> Yes, but request_probe can be change to a different value later on,
> e.g. in ff_parse_mpeg2_descriptor:
>
> int ff_read_packet(AVFormatContext *s, AVPacket *pkt)
> {
> [...]
> if (s->internal->raw_packet_buffer_remaining_size <= 0)
> if ((err = probe_codec(s, st, NULL)) < 0) // probe_packets = 
> 0, request_probe = -1
> return err;
> [...]
> ret = s->iformat->read_packet(s, pkt);
> ~~~
> ff_parse_mpeg2_descriptor([...])
> {
> [...]
> switch (desc_tag) {
> [...]
> case 0x05: /* registration descriptor */
> [...]
> st->request_probe = 50;
> [...]
> }
> ~~~
> [...]
> if (st->probe_packets) // still 0
> if ((err = probe_codec(s, st, NULL)) < 0)
> return err;
> av_assert0(st->request_probe <= 0); // now 50
> SIGABRT
>

Can you actually make that happen, or is that just speculation?

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

Re: [FFmpeg-devel] [PATCH] avformat: remove request_probe assert from ff_read_packet

[Andreas Cadhalpun]

On 18.10.2016 22:56, Michael Niedermayer wrote:
> On Tue, Oct 18, 2016 at 10:31:37PM +0200, Andreas Cadhalpun wrote:
>> Nothing guarantees to set request_probe to -1, so this assert can be
>> triggered, e.g. if st->probe_packets is 0.
> 
> probe_codec() called with NULL should cause
> st->probe_packets = 0
> st->request_probe = -1;

Yes, but request_probe can be change to a different value later on,
e.g. in ff_parse_mpeg2_descriptor:

int ff_read_packet(AVFormatContext *s, AVPacket *pkt)
{
[...]
if (s->internal->raw_packet_buffer_remaining_size <= 0)
if ((err = probe_codec(s, st, NULL)) < 0) // probe_packets = 0, 
request_probe = -1
return err;
[...]
ret = s->iformat->read_packet(s, pkt);
~~~
ff_parse_mpeg2_descriptor([...])
{
[...]
switch (desc_tag) {
[...]
case 0x05: /* registration descriptor */
[...]
st->request_probe = 50;
[...]
}
~~~
[...]
if (st->probe_packets) // still 0
if ((err = probe_codec(s, st, NULL)) < 0)
return err;
av_assert0(st->request_probe <= 0); // now 50
SIGABRT

Best regards,
Andreas

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

Re: [FFmpeg-devel] [PATCH] avformat: remove request_probe assert from ff_read_packet

[Michael Niedermayer]

On Tue, Oct 18, 2016 at 10:31:37PM +0200, Andreas Cadhalpun wrote:
> Nothing guarantees to set request_probe to -1, so this assert can be
> triggered, e.g. if st->probe_packets is 0.

probe_codec() called with NULL should cause
st->probe_packets = 0
st->request_probe = -1;

[...]
-- 
Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

Never trust a computer, one day, it may think you are the virus. -- Compn


signature.asc
Description: Digital signature
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

[FFmpeg-devel] [PATCH] avformat: remove request_probe assert from ff_read_packet

[Andreas Cadhalpun]

Nothing guarantees to set request_probe to -1, so this assert can be
triggered, e.g. if st->probe_packets is 0.

Signed-off-by: Andreas Cadhalpun 
---

I think the reason why this assert isn't triggered way more often is
that the probing code usually works quite well.

---
 libavformat/utils.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/libavformat/utils.c b/libavformat/utils.c
index 8a51aea..a62a073 100644
--- a/libavformat/utils.c
+++ b/libavformat/utils.c
@@ -806,7 +806,6 @@ int ff_read_packet(AVFormatContext *s, AVPacket *pkt)
 if (st->probe_packets)
 if ((err = probe_codec(s, st, NULL)) < 0)
 return err;
-av_assert0(st->request_probe <= 0);
 }
 continue;
 }
-- 
2.9.3
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel