Opened 4 months ago

Closed 4 months ago

Last modified 4 months ago

#6403 closed defect (wontfix)

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

Reported by: jrummell Owned by:
Priority: normal Component: undetermined
Version: git-master Keywords:
Cc: Blocked By:
Blocking: Reproduced by developer: no
Analyzed by developer: no

Description

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

Change History (9)

comment:1 Changed 4 months ago by 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?

comment:2 follow-up: Changed 4 months ago by cehoyos

  • Component changed from avformat to undetermined
  • Version changed from unspecified to git-master

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.

comment:3 in reply to: ↑ 2 Changed 4 months ago by 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.

comment:4 Changed 4 months ago by heleppkes

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

comment:5 Changed 4 months ago by Cigaes

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

comment:6 Changed 4 months ago by 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.

comment:7 Changed 4 months ago by cehoyos

  • Resolution set to wontfix
  • Status changed from new to closed

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

comment:8 Changed 4 months ago by heleppkes

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.

comment:9 Changed 4 months ago by 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.

Note: See TracTickets for help on using tickets.