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

[Paweł Hajdan]

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.

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


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


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

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


> mac osx seems to say:
>      Memory that is allocated via posix_memalign() can be used as an
> argument in subsequent calls to
>      realloc(3), reallocf(3), and free(3).  (Note however, that the
> allocation returned by realloc(3) or
>      reallocf(3) is not guaranteed to preserve the original alignment).
>
> dragonfly bsd seems to say:
>     Memory that is allocated via posix_memalign() can be used as an
> argument
>     in subsequent calls to realloc(3), reallocf(3), and free(3).
>

Alright, and on Linux/glibc it also works, even though it's not guaranteed
from reading the man page. Looks like the platforms (and standards) move in
the direction of allowing posix_memalign + realloc, which also makes sense
to me, since that makes the interface of memory allocator more "complete"
in some sense.

I'm going to see what google-perftools maintainers think about this. I
didn't know about C11 changes before, and I have a patch for that scenario
ready just in case.

Paweł