Use of both posix_memalign() and realloc() on same memory block not supported

[jrummell]

Attempting to update Chromium to use the latest FFmpeg, I get the following when running some of the tests:

realloc/memalign mismatch at 0x63cf52e67c0: non-NULL pointers passed to realloc must be obtained from malloc, calloc, or realloc

This is processing an H264 file (repro case for ​https://crbug.com/444539).

Chromium uses posix_memalign() for av_malloc().

It turns out that the code in libavformat/mov.c uses both av_malloc() and av_realloc() on AVCodecParameters.extradata. This appears to be incompatible if
HAVE_POSIX_MEMALIGN is defined.

Allocations of extradata in ​https://github.com/FFmpeg/FFmpeg/blob/master/libavformat/mov.c:
line 1428 in mov_realloc_extradata() uses av_reallocp().
line 1650 in mov_read_wave() uses av_mallocz().
line 2073 in mov_rewrite_dvd_sub_extradata() uses av_mallocz().
line 2333 in ff_mov_read_stsd_entries() uses av_malloc().

I also noticed that ff_alloc_extradata() in utils.c uses av_malloc().

So it looks like mov_realloc_extradata() should have a special case for HAVE_POSIX_MEMALIGN that implements realloc locally.

(And possibly cleanup the other calls in mov.c to use ff_alloc_extradata()).

[jkqxz]

ffmpeg uses posix_memalign() for all allocation on platforms where it is supported, and requires that realloc() be usable on the returned pointers. While POSIX does not require this, it is supported by all implementations we are aware of. (And indeed I would expect it to be supported by all non-DS9K implementations, since storing additional metadata to distinguish between pointers returned by posix_memalign() and malloc() in order to be able to release nasal demons when the former are passed to realloc() would be a pointless overhead.)

Do you have a platform on which this is not usable?

[Carl Eugen Hoyos]

Would a configure option that disables posix_memalign() help with your particular "use-case"?
If not, this is wont-fix unless a platform exists where this is a real issue.

[Cigaes]

Replying to cehoyos:

Would a configure option that disables posix_memalign() help with your particular "use-case"?

No, it will not. posix_memalign() is the preferred function for that task, and rightly so since it is the more standard.

If not, this is wont-fix unless a platform exists where this is a real issue.

Please do not do that. Not mixing av_malloc() and av_realloc() is a documented constraint of our API, mov.c seems to be violating that.

[Hendrik]

Actually, since 21f70940ae106bfffa07e73057cdb4b5e81a767a it is no longer a documented constraint, since no platforms are known where this is actually required.

[Cigaes]

My bad. But traces of that constraint still appear in the doxy.

[jrummell]

Since I'm not sure what Chromium / Chrome does, I asked ​others.

It appears that Chromium / Chrome uses ​tcmalloc. I was pointed to several internal bugs from 2013 that discussed this issue, and the resolution back then was that since the documentation for realloc() does not indicate that it definitely maintains memory alignment, it should not be allowed (and thus the warning in debug builds). I checked realloc() documentation, and did not find anything that says that alignment is preserved (although it would if the memory could be realloc'd in place). So I'm unlikely to make a strong case that tcmalloc needs changing.

Since there is data in FFmpeg that needs to be aligned, having Chromium move from using posix_memalign() in general seems like it would result in slower performance. Unfortunately there's no av_aligned_alloc() to help distinguish between the places where aligned memory is required, and where it's optional.

[Carl Eugen Hoyos]

I believe this is wont-fix until a real-world test-case exists.

[Hendrik]

realloc not actually preserving alignment is known, and av_realloc should not be used anywhere where alignment is required.

However, the key point is that using it on aligned memory should not break on any known systems, even if the reallocated memory is not aligned anymore afterwards.

[gjdfgh]

Indeed. Is it a problem that this mov extradata block is not aligned? (It was pointed out as only example.)

Personally I might still feel better about not mixing memalign/realloc.

[Pierre Bourdon]

I just hit another instance of this bug today on my system and ended up finding this bug sadly closed as wontfix. Mixing memalign and realloc hits some of the hardening measures implemented in Scudo (​https://llvm.org/docs/ScudoHardenedAllocator.html).

Scudo ERROR: allocation type mismatch when reallocating address 0x7c23c940d440
#0 0x00007e13cc19cbe0 in raise ()

from /nix/store/bjmg1g133m9xwxa0iy5inp2snvnfg151-glibc-2.27/lib/libc.so.6

#1 0x00007e13cc19ddc1 in abort ()

from /nix/store/bjmg1g133m9xwxa0iy5inp2snvnfg151-glibc-2.27/lib/libc.so.6

#2 0x00007e13cf793d9b in sanitizer::Abort() ()

from /nix/store/d3h5ip3azss68fak4fq2gk4d8vikh8k7-malloc-provider-scudo/lib/libclang_rt.scudo-x86_64.so

#3 0x00007e13cf781339 in sanitizer::Die() ()

from /nix/store/d3h5ip3azss68fak4fq2gk4d8vikh8k7-malloc-provider-scudo/lib/libclang_rt.scudo-x86_64.so

#4 0x00007e13cf7815c6 in scudo::dieWithMessage(char const*, ...) ()

from /nix/store/d3h5ip3azss68fak4fq2gk4d8vikh8k7-malloc-provider-scudo/lib/libclang_rt.scudo-x86_64.so

#5 0x00007e13cf77cc51 in scudo::scudoRealloc(void*, unsigned long) ()

from /nix/store/d3h5ip3azss68fak4fq2gk4d8vikh8k7-malloc-provider-scudo/lib/libclang_rt.scudo-x86_64.so

#6 0x00007e13cc97c62f in av_realloc_f ()

from /nix/store/1kxpdivgdaw0znrfwmknj6pgzhh3dc5y-ffmpeg-4.1.3/lib/libavutil.so.56

#7 0x00007e13cc97c685 in av_reallocp_array ()

from /nix/store/1kxpdivgdaw0znrfwmknj6pgzhh3dc5y-ffmpeg-4.1.3/lib/libavutil.so.56

#8 0x00007e13cd26d188 in ff_h2645_extract_rbsp ()

from /nix/store/1kxpdivgdaw0znrfwmknj6pgzhh3dc5y-ffmpeg-4.1.3/lib/libavcodec.so.58

...

ff_h2645_extract_rbsp calls av_reallocp_array (realloc internally) on nal->skipped_bytes_pos which is allocated via av_malloc_array (posix_memalign internally).

Does this qualify as a platform that exists and where this is a real issue?

[Carl Eugen Hoyos]

I don't think so.

[Cigaes]

Did you report the issue to Scudo? Most likely it is just something they neglected.