| Message ID | 20200813034317.6782-1-andreas.rheinhardt@gmail.com |
|---|---|
| State | Accepted |
| Commit | 4147f63d63358e5c1969bfe431ee08ca54f8434d |
| Headers | show |
| Series | [FFmpeg-devel] avfilter/formats: Fix heap-buffer overflow when merging channel layouts | expand |
| Context | Check | Description |
|---|---|---|
| andriy/default | pending | |
| andriy/make | success | Make finished |
| andriy/make_fate | success | Make fate finished |
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@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. Regards,
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@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
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; } } }
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 - The fix is easy: break out of the inner loop after having found a match. Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@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?) libavfilter/formats.c | 1 + 1 file changed, 1 insertion(+)