[FFmpeg-devel] [PATCH 5/9] nistspheredec: prevent overflow during block alignment calculation

Marton Balint cus at passwd.hu
Fri Jan 27 03:56:59 EET 2017


On Thu, 26 Jan 2017, Michael Niedermayer wrote:

> On Thu, Jan 26, 2017 at 03:52:04AM +0100, Marton Balint wrote:
>>
>> On Thu, 26 Jan 2017, Michael Niedermayer wrote:
>>
>>> On Thu, Jan 26, 2017 at 02:58:07AM +0100, Andreas Cadhalpun wrote:
>>>> On 26.01.2017 02:29, Ronald S. Bultje wrote:
>>>>> On Wed, Jan 25, 2017 at 8:12 PM, Andreas Cadhalpun <
>>>>> andreas.cadhalpun at googlemail.com> wrote:
>>>>>
>>>>>> Signed-off-by: Andreas Cadhalpun <Andreas.Cadhalpun at googlemail.com>
>>>>>> ---
>>>>>> libavformat/nistspheredec.c | 11 +++++++++++
>>>>>> 1 file changed, 11 insertions(+)
>>>>>>
>>>>>> diff --git a/libavformat/nistspheredec.c b/libavformat/nistspheredec.c
>>>>>> index 782d1dfbfb..3386497682 100644
>>>>>> --- a/libavformat/nistspheredec.c
>>>>>> +++ b/libavformat/nistspheredec.c
>>>>>> @@ -21,6 +21,7 @@
>>>>>>
>>>>>> #include "libavutil/avstring.h"
>>>>>> #include "libavutil/intreadwrite.h"
>>>>>> +#include "libavcodec/internal.h"
>>>>>> #include "avformat.h"
>>>>>> #include "internal.h"
>>>>>> #include "pcm.h"
>>>>>> @@ -90,6 +91,11 @@ static int nist_read_header(AVFormatContext *s)
>>>>>>             return 0;
>>>>>>         } else if (!memcmp(buffer, "channel_count", 13)) {
>>>>>>             sscanf(buffer, "%*s %*s %"SCNd32, &st->codecpar->channels);
>>>>>> +            if (st->codecpar->channels > FF_SANE_NB_CHANNELS) {
>>>>>> +                av_log(s, AV_LOG_ERROR, "Too many channels %d > %d\n",
>>>>>> +                       st->codecpar->channels, FF_SANE_NB_CHANNELS);
>>>>>> +                return AVERROR(ENOSYS);
>>>>>> +            }
>>>>>
>>>>>
>>>>> I've said this before, but again - please don't add useless log messages.
>>>>
>>>> I disagree that these log messages are useless and I'm not the only one [1].
>>>
>>> +1
>>>
>>> Log messages make debuging the code easier, as a developer i like to
>>> know why something fails having a hard failure but no clear and easy
>>> findable error message is bad
>>>
>>
>> I tend to agree with Ronald here, log messages which are practically
>> impossible to trigger with real-world files have little benefit,
>> also I don't think it is ffmpeg's job to thoroughly explain every
>> different kind of error.
>
> would you want a log message be removed if the
> people working on the code want the error message to be there ?
> You seem to just say that you see little benefit in it.
>

Yeah, because as far as I understand the reason behind this, it is 
only there to let the fuzzer people know why a fuzzed file fails. If we 
don't draw the line somewhere, ffmpeg will be an analyzer and not a 
decoder.

I see 3 problems (wm4 explicitly named them, but I also had them in mind)
- Little benefit, yet
- Makes the code less clean, more cluttered
- Increases binary size

The ideas I proposed (use macros, use common / factorized checks for 
common validatons and errors) might be a good compromise IMHO. Fuzzing 
thereforce can be done with "debug" builds.

Anyway, I am not blocking the patch, just expressing what I would prefer 
in the long run.

Regards,
Marton


More information about the ffmpeg-devel mailing list