On 28/10/15 16:19, Hendrik Leppkes wrote: > On Wed, Oct 28, 2015 at 3:50 PM, Luca Barbato <[email protected]> wrote: >> On 28/10/15 15:16, Hendrik Leppkes wrote: >>> On Wed, Oct 28, 2015 at 2:35 PM, Luca Barbato <[email protected]> wrote: >>>> On 27/10/15 01:30, Luca Barbato wrote: >>>>> On 27/10/15 00:09, Kieran Kunhya wrote: >>>>>> On 26 October 2015 at 22:48, Hendrik Leppkes <[email protected]> wrote: >>>>>>> On Mon, Oct 26, 2015 at 11:29 PM, Kieran Kunhya <[email protected]> wrote: >>>>>>>> From a1314d5c9774d555718bbc0a8612144c890bbc59 Mon Sep 17 00:00:00 2001 >>>>>>>> From: Kieran Kunhya <[email protected]> >>>>>>>> Date: Mon, 26 Oct 2015 22:26:35 +0000 >>>>>>>> Subject: [PATCH] opusdec: Don't run vector_fmul_scalar on zero length >>>>>>>> arrays >>>>>>>> >>>>>>>> Fixes crashes on fuzzed files >>>>>>>> >>>>>>>> --- >>>>>>>> libavcodec/opusdec.c | 2 +- >>>>>>>> 1 file changed, 1 insertion(+), 1 deletion(-) >>>>>>>> >>>>>>>> diff --git a/libavcodec/opusdec.c b/libavcodec/opusdec.c >>>>>>>> index acae6e1..03dd872 100644 >>>>>>>> --- a/libavcodec/opusdec.c >>>>>>>> +++ b/libavcodec/opusdec.c >>>>>>>> @@ -587,7 +587,7 @@ static int opus_decode_packet(AVCodecContext >>>>>>>> *avctx, void *data, >>>>>>>> memset(frame->extended_data[i], 0, frame->linesize[0]); >>>>>>>> } >>>>>>>> >>>>>>>> - if (c->gain_i) { >>>>>>>> + if (c->gain_i && decoded_samples >= 8) { >>>>>>>> >>>>>>>> c->fdsp.vector_fmul_scalar((float*)frame->extended_data[i], >>>>>>>> >>>>>>>> (float*)frame->extended_data[i], >>>>>>>> c->gain, >>>>>>>> FFALIGN(decoded_samples, 8)); >>>>>>> >>>>>>>> 0 might be less arbitrary. >>>>>> >>>>>> New version: >>>>>> >>>>>> From 25edf86e35773d419b4bcc7aeeb7b96d0f1ef958 Mon Sep 17 00:00:00 2001 >>>>>> From: Kieran Kunhya <[email protected]> >>>>>> Date: Mon, 26 Oct 2015 23:08:31 +0000 >>>>>> Subject: [PATCH] opusdec: Don't run vector_fmul_scalar on zero length >>>>>> arrays >>>>>> >>>>>> Fixes crashes on fuzzed files >>>>>> --- >>>>>> libavcodec/opusdec.c | 2 +- >>>>>> 1 file changed, 1 insertion(+), 1 deletion(-) >>>>>> >>>>>> diff --git a/libavcodec/opusdec.c b/libavcodec/opusdec.c >>>>>> index acae6e1..93c72c3 100644 >>>>>> --- a/libavcodec/opusdec.c >>>>>> +++ b/libavcodec/opusdec.c >>>>>> @@ -587,7 +587,7 @@ static int opus_decode_packet(AVCodecContext >>>>>> *avctx, void *data, >>>>>> memset(frame->extended_data[i], 0, frame->linesize[0]); >>>>>> } >>>>>> >>>>>> - if (c->gain_i) { >>>>>> + if (c->gain_i && decoded_samples > 0) { >>>>>> c->fdsp.vector_fmul_scalar((float*)frame->extended_data[i], >>>>>> (float*)frame->extended_data[i], >>>>>> c->gain, >>>>>> FFALIGN(decoded_samples, 8)); >>>>> >>>>> Which is the range of valid values here? >>>>> >>>> >>>> The documentation states "multiple of 4", all the other implementation >>>> of that function behave, Kostya suggests to fix the faulty >>>> implementation and I'm not really fond to triplecheck that the other >>>> uses and the future use of this function would have the same issue... >>> >>> I don't get this point at all. You already have to check any future >>> use if its a multiple of 4, whats the harm in just checking that its >>> not 0 in the same thought? >> >> That 0 is a multiple of 4 so at least the documented constraint should >> be updated. >> >>> Instead, you want to add error checks into the ASM? C does error >>> checks, ASM does optimized algorithms, no checking. >> >> No, I want to make sure that the functions to behave as documented, it >> can works either by making sure that the ASM implementation doesn't do >> anything unexpected or making sure that the documentation and the usage >> fit the new constraints. >> >> We mandate to have proper padding to let those functions run once IIRC. >> >> Either solution is fine as long we do not have dangerous leftovers, >> making sure that the x86 assembly behaves is a single change so might be >> safer. >> > > I can only repeat the point I made before - ASM is not about safety, > its about speed.
This specific case is not really meaningful, but the other arches do not have the problem so you are effectively slowing them down because whoever wrote the assembly did not consider 0 a multiple of 4... > If you can re-write it to not be a single cycle slower and not have > this particular behavior, great. I doubt I'll do that today, I have a quite limited knowledge of the x86 assembly. > In the meantime, can we apply this patch and get the crash done with? That's the consensus I guess and the 24h are passed so I'll merge it tonight if nobody beats me at it. http://plaid.libav.org/patch/1895/ will be pushed as well, assuming an implicit Ok from you and me. > FWIW, all other callers of vector_fmul_scalar don't seem to have a > chance of the value being zero. Thanks for checking. lu _______________________________________________ libav-devel mailing list [email protected] https://lists.libav.org/mailman/listinfo/libav-devel
