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

[Michael Niedermayer]

On Thu, Jan 26, 2017 at 06:43:45AM +0100, wm4 wrote:
> On Thu, 26 Jan 2017 03:20:02 +0100
> Michael Niedermayer <michaelni at gmx.at> 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
> > 
> > 
> > [...]
> 
> -1
> 
> This kind of things bloat the code with rare corner cases. One point
> would be that this increases binary size (why do we even still have the
> NULL_IF_CONFIG_SMALL macro? I'm not saying we should use it here, but
> the current use of the macro will barely even make a dent into the
> number of bytes "wasted" for optional strings, yet we bother).
> 
> Another point is that code becomes unreadable if obscure corner cases
> take up most of the code. I think that's w worrisome direction we're
> taking here.

While it doesnt apply to this patch here but,
Error messages in obscure checks that describe the error condition
in the source would make at least some checks easier to understand
than just a generic
"return AVERROR_INVALIDDATA"



> 
> When I debug FFmpeg API use, the thing that annoys me most is that I
> can't know where an error happens. I have to trace the origin manually.
> Error codes are almost always completely useless. This patchset adds a
> lot of NOSYS, which is used 254 times in FFmpeg and which is about 99%
> meaningless and resolves to an even worse error message when using
> av_err2str(). Adding an av_log to error error point would "work" (at
> least you could use grep), but isn't a good solution.

I think the idea about something more informative than a int32 error
code did come up previously.
I certainly would be in favor of having an error value that could be
used to pinpoint the error location(s) and or function(s) it passed
through, this would be usefull for debguging in general

[...]
-- 
Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

It is what and why we do it that matters, not just one of them.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 181 bytes
Desc: Digital signature
URL: <http://ffmpeg.org/pipermail/ffmpeg-devel/attachments/20170126/5d8147de/attachment.sig>