mbox series

[FFmpeg-devel,0/3] avcodec/aacdec_template: improvements to 22.2 layout logic

Message ID 20200818192538.35023-1-jeebjp@gmail.com
Headers show
Series avcodec/aacdec_template: improvements to 22.2 layout logic
Related show

Message

Jan Ekström Aug. 18, 2020, 7:25 p.m. UTC
The first two commits fix both of the fuzzing samples I have on hand.
One being from #8845, and another provided privately by Michael. Changes have
been tested both with clang 10's ASAN as well as standard valgrind.

The latter 22.2 check could be simplified to just the layout (since if the
stream passes all checks to that point, it probably is 22.2), but I
decided to post the first version this way to show that I haven't just
been sitting on my backside.

The last commit is general debug logging that I found helpful during
adding support of 22.2 by logging the element order before/after reorder.
This one, if found non-interesting, can just be ignored. I decided to
post it as it was part of my verification.

Jan Ekström (3):
  avcodec/aacdec_template: keep tabs on layout in sniff_channel_order
  avcodec/aacdec_template: add more checks to make sure only 22.2 gets
    to 22.2
  avcodec/aacdec_template: log the element order before/after reordering

 libavcodec/aacdec_template.c | 119 ++++++++++++++++++++++++++++-------
 1 file changed, 96 insertions(+), 23 deletions(-)

Comments

Jan Ekström Aug. 19, 2020, 9:51 p.m. UTC | #1
On Tue, Aug 18, 2020 at 10:25 PM Jan Ekström <jeebjp@gmail.com> wrote:
>
> The first two commits fix both of the fuzzing samples I have on hand.
> One being from #8845, and another provided privately by Michael. Changes have
> been tested both with clang 10's ASAN as well as standard valgrind.
>

For the record I have a feeling that the actual reason for the issues
is an underlying issue where a ChannelElement in a list gets
allocated, but then also freed (yet not actually set to nullptr?), and
my not strict enough validation based on valid samples just happened
to bring it to the surface.

Since I got publicly hurried and called out to "Please fix it or
revert ASAP!", here is the thing that anyone sane enough will attempt
to do to get people off their backs to get more breathing room: Here's
more stringent checks so that 22.2 will only be probed if the
configuration aligns exactly as it does for valid streams, and it
seems to remove the symptoms with regards to all of the provided
fuzzed samples.

I hope y'all have much more fun time than I have.

Jan
James Almer Aug. 19, 2020, 11:32 p.m. UTC | #2
On 8/19/2020 6:51 PM, Jan Ekström wrote:
> On Tue, Aug 18, 2020 at 10:25 PM Jan Ekström <jeebjp@gmail.com> wrote:
>>
>> The first two commits fix both of the fuzzing samples I have on hand.
>> One being from #8845, and another provided privately by Michael. Changes have
>> been tested both with clang 10's ASAN as well as standard valgrind.
>>
> 
> For the record I have a feeling that the actual reason for the issues
> is an underlying issue where a ChannelElement in a list gets
> allocated, but then also freed (yet not actually set to nullptr?), and
> my not strict enough validation based on valid samples just happened
> to bring it to the surface.
> 
> Since I got publicly hurried and called out to "Please fix it or
> revert ASAP!", here is the thing that anyone sane enough will attempt
> to do to get people off their backs to get more breathing room: Here's
> more stringent checks so that 22.2 will only be probed if the
> configuration aligns exactly as it does for valid streams, and it
> seems to remove the symptoms with regards to all of the provided
> fuzzed samples.
> 
> I hope y'all have much more fun time than I have.

I think you're reading too much into what Paul said. The change
introduced issues with fuzzed samples, which of course needs to be
fixed. He could have simply asked for a fix before suggesting a revert
(last resort when the author refuses to fix something, which is not the
case here), but by no means you should feed stressed about the request
or the way he made it.

> 
> Jan
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel@ffmpeg.org
> https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
> 
> To unsubscribe, visit link above, or email
> ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".
>