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

wm4 nfxjfg at googlemail.com
Fri Jan 27 14:03:00 EET 2017


On Thu, 26 Jan 2017 17:02:37 +0100
Michael Niedermayer <michaelni at gmx.at> wrote:

> 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"

In my opinion, there's no choice but to debug such cases manually
anyway. Whether you think that log messages help you better than
breakpoints in a source debugger is a different question.

> > 
> > 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

We could probably return a struct that contains the __FILE__ etc. value
of the error site, but this would probably be too over-engineered and
intrusive for a real solution.

Back to this patch and the core issue: I think there's no ideal
solution. So balance is required. Don't add av_logs to every error, but
add them to cases that are interesting and would be otherwise hard to
find.

I think fine grained error logging for read_header in a really obscure
and tiny demuxer isn't an interesting/useful case.


More information about the ffmpeg-devel mailing list