[FFmpeg-devel] [PATCH] pixdesc: Explicitly handle invalid arguments to av_find_best_pix_fmt_of_2()

[Michael Niedermayer]

On Thu, Jul 20, 2017 at 09:54:05PM +0100, Mark Thompson wrote:
> ---
> On 20/07/17 01:33, Michael Niedermayer wrote:
> > Hi
> > 
> > On Tue, Jul 18, 2017 at 11:01:01PM +0000, Mark Thompson wrote:
> >> ffmpeg | branch: master | Mark Thompson <sw at jkqxz.net> | Thu Jul  6 22:50:35 2017 +0100| [8a442d7a8a687a469ca502a18a0c68f5302b15e0] | committer: Mark Thompson
> >>
> >> pixdesc: Improve scoring for opaque/unknown pixel formats
> >>
> >> Hardware pixel formats do not tell you anything about their actual
> >> contents, but should still score higher than formats with completely
> >> unknown properties, which in turn should score higher than invalid
> >> formats.
> >>
> >> Do not return an AVERROR code as a score.
> >>
> >> Fixes a hang in libavfilter where format negotiation gets stuck in a
> >> loop because AV_PIX_FMT_NONE scores more highly than all other
> >> possibilities.
> >>
> >>> http://git.videolan.org/gitweb.cgi/ffmpeg.git/?a=commit;h=8a442d7a8a687a469ca502a18a0c68f5302b15e0
> >> ---
> >>
> >>  libavutil/pixdesc.c | 16 ++++++++++++----
> >>  1 file changed, 12 insertions(+), 4 deletions(-)
> > 
> > This still breaks
> > valgrind ./ffmpeg_g -i ~/videos/matrixbench_mpeg2.mpg -y  -t 4 -acodec libmp3lame http://127.0.0.1:8192/feed1.ffm
> > [mpeg @ 0x24823f20] start time for stream 0 is not set in estimate_timings_from_pts
> > Input #0, mpeg, from '/home/michael/videos/matrixbench_mpeg2.mpg':
> >   Duration: 00:03:07.66, start: 0.220000, bitrate: 5633 kb/s
> >     Stream #0:0[0x1bf]: Data: dvd_nav_packet
> >     Stream #0:1[0x1e0]: Video: mpeg2video (Main), yuv420p(tv, bt470bg/bt470m/bt470m, bottom first), 720x576 [SAR 16:15 DAR 4:3], 25 fps, 25 tbr, 90k tbn, 50 tbc
> >     Stream #0:2[0x1c0]: Audio: mp2, 48000 Hz, stereo, s16p, 384 kb/s
> > ==17852== Invalid read of size 1
> > ==17852==    at 0x104871B: av_find_best_pix_fmt_of_2 (in ffmpeg/ffmpeg_g)
> > ==17852==    by 0x4BD23A: choose_pixel_fmt (in ffmpeg/ffmpeg_g)
> > ==17852==    by 0x4BBB2C: open_output_file (in ffmpeg/ffmpeg_g)
> > ==17852==    by 0x4BC696: ffmpeg_parse_options (in ffmpeg/ffmpeg_g)
> > ==17852==    by 0x4A8A7D: main (in ffmpeg/ffmpeg_g)
> > ==17852==  Address 0x9 is not stack'd, malloc'd or (recently) free'd
> > ==17852==
> > ==17852==
> > ==17852== Process terminating with default action of signal 11 (SIGSEGV)
> > ==17852==  Access not within mapped region at address 0x9
> > ==17852==    at 0x104871B: av_find_best_pix_fmt_of_2 (in ffmpeg/ffmpeg_g)
> > ==17852==    by 0x4BD23A: choose_pixel_fmt (in ffmpeg/ffmpeg_g)
> > ==17852==    by 0x4BBB2C: open_output_file (in ffmpeg/ffmpeg_g)
> > ==17852==    by 0x4BC696: ffmpeg_parse_options (in ffmpeg/ffmpeg_g)
> > ==17852==    by 0x4A8A7D: main (in ffmpeg/ffmpeg_g)
> > ==17852==  If you believe this happened as a result of a stack
> > ==17852==  overflow in your program's main thread (unlikely but
> > ==17852==  possible), you can try to increase the size of the
> > ==17852==  main thread stack using the --main-stacksize= flag.
> > ==17852==  The main thread stack size used in this run was 8388608.
> > 
> > 
> > the receiver side of the connection can be setup with
> > valgrind ./ffserver_g -f ~/videos/ffserver.conf
> > 
> > ffserver.conf attached
> > 
> > [...]
> > 
> 
> Right - so someone does call find_best() with an invalid source format, which will give the same score to all possibilities and then barf if one of them is invalid.
> 
> This change makes the invalid format handling more explicit, and fixes your case.
> 
> Thanks,
> 
> - Mark
> 
> 
>  libavutil/pixdesc.c | 32 +++++++++++++++++++-------------
>  1 file changed, 19 insertions(+), 13 deletions(-)

LGTM

thx

PS: heres the patch without whitespace changes (much more readable)

commit a0f2f597f0922a37486732a961c94af0507af234
Author: Mark Thompson <sw at jkqxz.net>
Date:   Thu Jul 20 21:54:05 2017 +0100

    pixdesc: Explicitly handle invalid arguments to av_find_best_pix_fmt_of_2()

    Signed-off-by: Michael Niedermayer <michael at niedermayer.cc>

diff --git a/libavutil/pixdesc.c b/libavutil/pixdesc.c
index 1983ce9ef5..a9dd11a498 100644
--- a/libavutil/pixdesc.c
+++ b/libavutil/pixdesc.c
@@ -2633,6 +2633,11 @@ enum AVPixelFormat av_find_best_pix_fmt_of_2(enum AVPixelFormat dst_pix_fmt1, en
     const AVPixFmtDescriptor *desc2 = av_pix_fmt_desc_get(dst_pix_fmt2);
     int score1, score2;

+    if (!desc1)
+        dst_pix_fmt = dst_pix_fmt2;
+    else if (!desc2)
+        dst_pix_fmt = dst_pix_fmt1;
+    else {
         loss_mask= loss_ptr?~*loss_ptr:~0; /* use loss mask if provided */
         if(!has_alpha)
             loss_mask &= ~FF_LOSS_ALPHA;
@@ -2649,6 +2654,7 @@ enum AVPixelFormat av_find_best_pix_fmt_of_2(enum AVPixelFormat dst_pix_fmt1, en
         } else {
             dst_pix_fmt = score1 < score2 ? dst_pix_fmt2 : dst_pix_fmt1;
         }
+    }

     if (loss_ptr)
         *loss_ptr = av_get_pix_fmt_loss(dst_pix_fmt, src_pix_fmt, has_alpha)

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

Elect your leaders based on what they did after the last election, not
based on what they say before an election.

-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 181 bytes
Desc: Digital signature
URL: <http://ffmpeg.org/pipermail/ffmpeg-devel/attachments/20170721/90a51eac/attachment.sig>