[FFmpeg-devel] [PATCH] oggparsevorbis: use av_realloc consistently

[Michael Niedermayer]

On Fri, Jan 11, 2013 at 10:16:04AM -0800, Paweł Hajdan wrote:
> On Fri, Jan 11, 2013 at 5:54 AM, Michael Niedermayer <michaelni at gmx.at>wrote:
> 
> > On Thu, Jan 10, 2013 at 08:17:36PM +0100, Nicolas George wrote:
> > > Le primidi 21 nivôse, an CCXXI, Dale Curtis a écrit :
> > > > It's probably not only there, but it was the only one we detected in
> > the
> > > > codecs used by Chrome.
> > >
> > > Ok, that explains, but if there is really a problem, we will need to fix
> > it
> > > completely. Can you explain exactly how the problem manifests itself.
> >
> 
> Fully agreed. I'm glad that's your approach to fixes (it's the first time
> I'm submitting a patch for ffmpeg, and I subscribed to ffmpeg-devel on the
> same day).
> 
> I'll wait until we have a consensus here about the right course of action,
> and then I'd be happy to do a full pass with debugallocation + FATE over
> the codebase (if there is any other testing procedure I should be using,
> please let me know).
> 
> 
> > > (Single Unix does not tell anything about interactions between
> > > posix_memalign and realloc; nor does the GNU man page; the NetBSD man
> > page
> > > states that they are compatible.)
> >
> > Iam curious about this as well.
> > Is there an actual platform relevant to ffmpeg on which
> > posix_memalign() + realloc() fails ?
> >
> 
> Do you mean a system-default memory allocator that would not work properly
> with posix_memalign + realloc? I'm not aware of any.

I meant anything that a user would want to use ffmpeg with.


> 
> However, debugallocation which is part of popular google-perftools (
> http://code.google.com/p/gperftools/), and which is also used in Chrome,
> does not support it. The failure mode in current version is buffer contents
> corruption, and I have a patch that makes it fail with a diagnostic message
> instead. I also have a patch that makes it support posix_memalign +
> realloc, but so far maintainers of the code went with the diagnostic
> message + failure route instead, since it seems implementations may
> legitimately refuse to support it

> (and strict debug allocators
> *should*only implement the smallest subset that is required to work,
> to ensure
> client code doesn't rely on behavior of particular implementation).

This is maybe drifting off topic but i dont fully agree,
A tool to find memory allocation anomalies first has to work with the
application it analyzes.
If it fails on specific constructs then any application that uses
such constructs will not be debugable with said tool.



> 
> 
> > Also the "calloc, malloc, or realloc" Text has been removed from ISO C
> > C99:
> > Otherwise, if ptr does not match a pointer earlier returned by the
> > calloc, malloc, or realloc function, or if the space has been deallocated
> > by a call
> > to the free or realloc function, the behavior is undefined.
> >
> 
> Oh, this is interesting. :) Note however that even though the wording is
> different (the one I quoted comes from a Linux man page), I think the
> meaning is the same. Do you agree?

They sound similar yes


> 
> 
> > C11:
> > Otherwise, if ptr does not match a pointer earlier returned by a memory
> > management function, or if the space has been deallocated by a call to the
> > free or
> > realloc function, the behavior is undefined.
> >
> 
> Interesting. :) posix_memalign sounds like a memory management function
> (does the standard specify a list of what is considered a memory management
> function? they should be precise about that to avoid any ambiguity), so the
> wording above would make posix_memalign + realloc legal.

The C11 draft i have does not seem to define the term
"memory management function" or i fail to find it


> 
> Now the question is about C99-conformant implementations that do not
> support posix_memalign + realloc (debugallocation comes to mind)?

yes, we need to do something about this, is there some way by which
debugallocation can be detected ?

It would also be interresting to know how many cases of
posix_memalign + realloc() exist in the code.

ATM i see as solutions
A. detect debugallocation and do a malloc+memcpy+free for av_realloc
B. change the code to never mix av_malloc* and av_realloc
C. ignore it and declare debugallocation unsupported
D. mark memory so the used allocator can be detected 

B is only practical if there are very few such cases

D would interfere with memory debuging using valgrind and asan
  and would add overhead so it does not seem like a good idea

C sucks as debugallocation could then not be used


[...]

-- 
Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

It is dangerous to be right in matters on which the established authorities
are wrong. -- Voltaire
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 198 bytes
Desc: Digital signature
URL: <http://ffmpeg.org/pipermail/ffmpeg-devel/attachments/20130111/8baf19a2/attachment.asc>