[FFmpeg-devel] [PATCH] avfilter/formats: Fix heap-buffer overflow when merging channel layouts

[Andreas Rheinhardt]

Nicolas George:
> Andreas Rheinhardt (12020-08-13):
>> The channel layouts accepted by ff_merge_channel_layouts() are of two
>> types: Ordinary channel layouts and generic channel layouts. These are
>> layouts that match all layouts with a certain number of channels.
>> Therefore parsing these channel layouts is not done in one go; instead
>> first the intersection of the ordinary layouts of the first input
>> list of channel layouts with the ordinary layouts of the second list is
>> determined, then the intersection of the ordinary layouts of the first
>> one and the generic layouts of the second one etc. In order to mark the
>> ordinary channel layouts that have already been matched as used they are
>> zeroed. The inner loop that does this is as follows:
>>
>> for (j = 0; j < b->nb_channel_layouts; j++) {
>>     if (a->channel_layouts[i] == b->channel_layouts[j]) {
>>         ret->channel_layouts[ret_nb++] = a->channel_layouts[i];
>>         a->channel_layouts[i] = b->channel_layouts[j] = 0;
>>     }
>> }
>>
>> (Here ret->channel_layouts is the array containing the intersection of
>> the two input arrays.)
>>
>> Yet the problem with this code is that after a match has been found, the
>> loop continues the search with the new value a->channel_layouts[i].
>> The intention of zeroing these elements was to make sure that elements
>> already paired at this stage are ignored later. And while they are indeed
>> ignored when pairing ordinary and generic channel layouts later, it has
>> the exact opposite effect when pairing ordinary channel layouts.
>>
>> To see this consider the channel layouts A B C D E and E D C B A. In the
>> first round, A and A will be paired and added to ret->channel_layouts.
>> In the second round, the input arrays are 0 B C D E and E D C B 0.
>> At first B and B will be matched and zeroed, but after doing so matching
>> continues, but this time it will search for 0, which will match with the
>> last entry of the second array. ret->channel_layouts now contains A B 0.
>> In the third round, C 0 0 will be added to ret->channel_layouts etc.
>> This gives a quadratic amount of elements, yet the amount of elements
>> allocated for said array is only the sum of the sizes of a and b.
>>
>> This issue can e.g. be reproduced by
>> ffmpeg -f lavfi -i anullsrc=cl=7.1 \
>> -af 'aformat=cl=mono|stereo|2.1|3.0|4.0,aformat=cl=4.0|3.0|2.1|stereo|mono' \
>> -f null -
> 
> Oh, good catch. It is a bug indeed.
> 
>> The fix is easy: break out of the inner loop after having found a match.
>>
>> Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt at gmail.com>
>> ---
>> 1. This patch is intended to be applied before
>> https://ffmpeg.org/pipermail/ffmpeg-devel/2020-August/267790.html
>>
> 
>> 2. After this patch the code will stay within the buffer provided that
>> no list contains one and the same generic channel layout multiple times
>> (ordinary channel layouts may appear arbitrary often and there may even
>> be ordinary channel layouts and generic channel layouts of the same
>> channel count in the same list). But is this actually ensured? 
>> (Given that the concept of generic channel layouts is not part of the
>> public API, but just valid in AVFilterChannelLayouts the question can be
>> rephrased as: Is it possible for the (API) user to somehow create
>> generic channel layouts?)
> 
> I thought it was ensured by the rest of the code, where the lists of
> supported formats are very limited. But I forgot what you found: a few
> filters accept them from the application or the user. In particular,
> bufferink can inject generic layouts.
> 
> I will make a patch that checks the validity after calling
> query_formats.
> 
>>
>>  libavfilter/formats.c | 1 +
>>  1 file changed, 1 insertion(+)
>>
>> diff --git a/libavfilter/formats.c b/libavfilter/formats.c
>> index 00d050e439..f39396ed81 100644
>> --- a/libavfilter/formats.c
>> +++ b/libavfilter/formats.c
>> @@ -219,6 +219,7 @@ AVFilterChannelLayouts *ff_merge_channel_layouts(AVFilterChannelLayouts *a,
>>              if (a->channel_layouts[i] == b->channel_layouts[j]) {
>>                  ret->channel_layouts[ret_nb++] = a->channel_layouts[i];
>>                  a->channel_layouts[i] = b->channel_layouts[j] = 0;
>> +                break;
>>              }
>>          }
>>      }
> 
> I think you can add the break to the other two loops too.
> 
Yes. And this even makes sure that we always stay within the buffer
regardless of whether the lists contain duplicate generic elements.

- Andreas