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

[Marton Balint]

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.

Maybe some middle ground can be found defining macros so the user can 
decide if he wants these messages in the build or not, also maybe some
"probability" levels can be added to these errors so the macros can work 
based on them, and finally a lot of similar checks exist around the 
codebase which could be factorized, so the code - even with reported 
messages - would look less cluttered with functionally useless parts.

Regards,
Marton