Re: [FFmpeg-devel] [PATCH] avformat: remove request_probe assert from ff_read_packet
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
On Wed, Oct 19, 2016 at 07:27:59PM +0200, Andreas Cadhalpun wrote: > 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 > > utils.c |2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > 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 thx [...] -- Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB No great genius has ever existed without some touch of madness. -- Aristotle 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
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
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
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
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
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
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