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

[Mark Thompson]

---
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(-)

diff --git a/libavutil/pixdesc.c b/libavutil/pixdesc.c
index 1983ce9ef5..a9dd11a498 100644
--- a/libavutil/pixdesc.c
+++ b/libavutil/pixdesc.c
@@ -2633,21 +2633,27 @@ 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;
 
-    loss_mask= loss_ptr?~*loss_ptr:~0; /* use loss mask if provided */
-    if(!has_alpha)
-        loss_mask &= ~FF_LOSS_ALPHA;
-
-    score1 = get_pix_fmt_score(dst_pix_fmt1, src_pix_fmt, &loss1, loss_mask);
-    score2 = get_pix_fmt_score(dst_pix_fmt2, src_pix_fmt, &loss2, loss_mask);
-
-    if (score1 == score2) {
-        if(av_get_padded_bits_per_pixel(desc2) != av_get_padded_bits_per_pixel(desc1)) {
-            dst_pix_fmt = av_get_padded_bits_per_pixel(desc2) < av_get_padded_bits_per_pixel(desc1) ? dst_pix_fmt2 : dst_pix_fmt1;
+    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;
+
+        score1 = get_pix_fmt_score(dst_pix_fmt1, src_pix_fmt, &loss1, loss_mask);
+        score2 = get_pix_fmt_score(dst_pix_fmt2, src_pix_fmt, &loss2, loss_mask);
+
+        if (score1 == score2) {
+            if(av_get_padded_bits_per_pixel(desc2) != av_get_padded_bits_per_pixel(desc1)) {
+                dst_pix_fmt = av_get_padded_bits_per_pixel(desc2) < av_get_padded_bits_per_pixel(desc1) ? dst_pix_fmt2 : dst_pix_fmt1;
+            } else {
+                dst_pix_fmt = desc2->nb_components < desc1->nb_components ? dst_pix_fmt2 : dst_pix_fmt1;
+            }
         } else {
-            dst_pix_fmt = desc2->nb_components < desc1->nb_components ? dst_pix_fmt2 : dst_pix_fmt1;
+            dst_pix_fmt = score1 < score2 ? dst_pix_fmt2 : dst_pix_fmt1;
         }
-    } else {
-        dst_pix_fmt = score1 < score2 ? dst_pix_fmt2 : dst_pix_fmt1;
     }
 
     if (loss_ptr)
-- 
2.11.0