| Message ID | c54fb7f0-aa76-4f79-cdf8-c668672bb847@gmail.com |
|---|---|
| State | Accepted |
| Headers | show |
On Wed, Dec 21, 2016 at 09:45:39PM -0300, James Almer wrote: > On 12/20/2016 11:36 PM, Michael Niedermayer wrote: > > On Tue, Dec 20, 2016 at 04:53:51PM -0800, Thomas Turner wrote: > >> Signed-off-by: Thomas Turner <thomastdt@googlemail.com> > >> --- > >> libavutil/Makefile | 1 + > >> libavutil/tests/audio_fifo.c | 196 +++++++++++++++++++++++++++++++++++++ > >> tests/fate/libavutil.mak | 4 + > >> tests/ref/fate/audio_fifo | 228 +++++++++++++++++++++++++++++++++++++++++++ > >> 4 files changed, 429 insertions(+) > >> create mode 100644 libavutil/tests/audio_fifo.c > >> create mode 100644 tests/ref/fate/audio_fifo > > > > applied > > > > thx > > This is crashing on some fate clients. > > I noticed it's using malloc and free instead of the av_malloc family, > so maybe it's related to that? > Patch attached in any case, it's proper even if not the reason behind > the crashes. i saw the malloc/free before applying. It seemed to make sense to use libc functions in a test of our public API why is it crashing ? [...]
On 12/21/2016 10:49 PM, Michael Niedermayer wrote: > On Wed, Dec 21, 2016 at 09:45:39PM -0300, James Almer wrote: >> On 12/20/2016 11:36 PM, Michael Niedermayer wrote: >>> On Tue, Dec 20, 2016 at 04:53:51PM -0800, Thomas Turner wrote: >>>> Signed-off-by: Thomas Turner <thomastdt@googlemail.com> >>>> --- >>>> libavutil/Makefile | 1 + >>>> libavutil/tests/audio_fifo.c | 196 +++++++++++++++++++++++++++++++++++++ >>>> tests/fate/libavutil.mak | 4 + >>>> tests/ref/fate/audio_fifo | 228 +++++++++++++++++++++++++++++++++++++++++++ >>>> 4 files changed, 429 insertions(+) >>>> create mode 100644 libavutil/tests/audio_fifo.c >>>> create mode 100644 tests/ref/fate/audio_fifo >>> >>> applied >>> >>> thx >> >> This is crashing on some fate clients. >> >> I noticed it's using malloc and free instead of the av_malloc family, >> so maybe it's related to that? >> Patch attached in any case, it's proper even if not the reason behind >> the crashes. > > i saw the malloc/free before applying. It seemed to make sense to > use libc functions in a test of our public API > > why is it crashing ? I don't know, i can't reproduce it. Seems to be OpenBSD FATE clients only as far as i could see.
On Wed, Dec 21, 2016 at 10:53:36PM -0300, James Almer wrote: > On 12/21/2016 10:49 PM, Michael Niedermayer wrote: > > On Wed, Dec 21, 2016 at 09:45:39PM -0300, James Almer wrote: > >> On 12/20/2016 11:36 PM, Michael Niedermayer wrote: > >>> On Tue, Dec 20, 2016 at 04:53:51PM -0800, Thomas Turner wrote: > >>>> Signed-off-by: Thomas Turner <thomastdt@googlemail.com> > >>>> --- > >>>> libavutil/Makefile | 1 + > >>>> libavutil/tests/audio_fifo.c | 196 +++++++++++++++++++++++++++++++++++++ > >>>> tests/fate/libavutil.mak | 4 + > >>>> tests/ref/fate/audio_fifo | 228 +++++++++++++++++++++++++++++++++++++++++++ > >>>> 4 files changed, 429 insertions(+) > >>>> create mode 100644 libavutil/tests/audio_fifo.c > >>>> create mode 100644 tests/ref/fate/audio_fifo > >>> > >>> applied > >>> > >>> thx > >> > >> This is crashing on some fate clients. > >> > >> I noticed it's using malloc and free instead of the av_malloc family, > >> so maybe it's related to that? > >> Patch attached in any case, it's proper even if not the reason behind > >> the crashes. > > > > i saw the malloc/free before applying. It seemed to make sense to > > use libc functions in a test of our public API > > > > why is it crashing ? > > I don't know, i can't reproduce it. Seems to be OpenBSD FATE clients only > as far as i could see. no openbsd needed, valgrind shows these errors Thomas, can you look at this ? TEST: 1 written: 12 written: 12 remaining samples in audio_fifo: 24 ==32011== Invalid write of size 8 ==32011== at 0x4016B2: read_samples_from_audio_fifo (audio_fifo.c:92) ==32011== by 0x4018C6: test_function (audio_fifo.c:141) ==32011== by 0x401AE2: main (audio_fifo.c:193) ==32011== Address 0x540f040 is 0 bytes after a block of size 0 alloc'd ==32011== at 0x4C2C66F: malloc (vg_replace_malloc.c:270) ==32011== by 0x4014E5: allocate_memory (audio_fifo.c:55) ==32011== by 0x401674: read_samples_from_audio_fifo (audio_fifo.c:88) ==32011== by 0x4018C6: test_function (audio_fifo.c:141) ==32011== by 0x401AE2: main (audio_fifo.c:193) ==32011== ==32011== Invalid read of size 8 ==32011== at 0x401320: av_audio_fifo_read (audio_fifo.c:193) ==32011== by 0x4016DD: read_samples_from_audio_fifo (audio_fifo.c:95) ==32011== by 0x4018C6: test_function (audio_fifo.c:141) ==32011== by 0x401AE2: main (audio_fifo.c:193) ==32011== Address 0x540f040 is 0 bytes after a block of size 0 alloc'd ==32011== at 0x4C2C66F: malloc (vg_replace_malloc.c:270) ==32011== by 0x4014E5: allocate_memory (audio_fifo.c:55) ==32011== by 0x401674: read_samples_from_audio_fifo (audio_fifo.c:88) ==32011== by 0x4018C6: test_function (audio_fifo.c:141) ==32011== by 0x401AE2: main (audio_fifo.c:193) ==32011== read: 12 ==32011== Invalid read of size 8 ==32011== at 0x4015A8: print_audio_bytes (audio_fifo.c:74) ==32011== by 0x401900: test_function (audio_fifo.c:146) ==32011== by 0x401AE2: main (audio_fifo.c:193) ==32011== Address 0x540f040 is 0 bytes after a block of size 0 alloc'd ==32011== at 0x4C2C66F: malloc (vg_replace_malloc.c:270) ==32011== by 0x4014E5: allocate_memory (audio_fifo.c:55) ==32011== by 0x401674: read_samples_from_audio_fifo (audio_fifo.c:88) ==32011== by 0x4018C6: test_function (audio_fifo.c:141) ==32011== by 0x401AE2: main (audio_fifo.c:193) ==32011== 00 01 02 03 04 05 06 07 08 09 0a 0b remaining samples in audio_fifo: 12 ==32011== Invalid read of size 8 ==32011== at 0x401164: av_audio_fifo_peek (audio_fifo.c:150) ==32011== by 0x401937: test_function (audio_fifo.c:150) ==32011== by 0x401AE2: main (audio_fifo.c:193) ==32011== Address 0x540f040 is 0 bytes after a block of size 0 alloc'd ==32011== at 0x4C2C66F: malloc (vg_replace_malloc.c:270) ==32011== by 0x4014E5: allocate_memory (audio_fifo.c:55) ==32011== by 0x401674: read_samples_from_audio_fifo (audio_fifo.c:88) ==32011== by 0x4018C6: test_function (audio_fifo.c:141) ==32011== by 0x401AE2: main (audio_fifo.c:193) ==32011== peek: ==32011== Invalid read of size 8 ==32011== at 0x4015A8: print_audio_bytes (audio_fifo.c:74) ==32011== by 0x401967: test_function (audio_fifo.c:155) ==32011== by 0x401AE2: main (audio_fifo.c:193) ==32011== Address 0x540f040 is 0 bytes after a block of size 0 alloc'd ==32011== at 0x4C2C66F: malloc (vg_replace_malloc.c:270) ==32011== by 0x4014E5: allocate_memory (audio_fifo.c:55) ==32011== by 0x401674: read_samples_from_audio_fifo (audio_fifo.c:88) ==32011== by 0x4018C6: test_function (audio_fifo.c:141) ==32011== by 0x401AE2: main (audio_fifo.c:193) ==32011== 00 01 02 03 04 05 06 07 08 09 0a 0b peek_at: ==32011== Invalid read of size 8 ==32011== at 0x401264: av_audio_fifo_peek_at (audio_fifo.c:174) ==32011== by 0x40199E: test_function (audio_fifo.c:161) ==32011== by 0x401AE2: main (audio_fifo.c:193) ==32011== Address 0x540f040 is 0 bytes after a block of size 0 alloc'd ==32011== at 0x4C2C66F: malloc (vg_replace_malloc.c:270) ==32011== by 0x4014E5: allocate_memory (audio_fifo.c:55) ==32011== by 0x401674: read_samples_from_audio_fifo (audio_fifo.c:88) ==32011== by 0x4018C6: test_function (audio_fifo.c:141) ==32011== by 0x401AE2: main (audio_fifo.c:193) ==32011== 0: ==32011== Invalid read of size 8 ==32011== at 0x4015A8: print_audio_bytes (audio_fifo.c:74) ==32011== by 0x4019D8: test_function (audio_fifo.c:166) ==32011== by 0x401AE2: main (audio_fifo.c:193) ==32011== Address 0x540f040 is 0 bytes after a block of size 0 alloc'd ==32011== at 0x4C2C66F: malloc (vg_replace_malloc.c:270) ==32011== by 0x4014E5: allocate_memory (audio_fifo.c:55) ==32011== by 0x401674: read_samples_from_audio_fifo (audio_fifo.c:88) ==32011== by 0x4018C6: test_function (audio_fifo.c:141) ==32011== by 0x401AE2: main (audio_fifo.c:193) ==32011== 00 1: 01 2: 02 3: 03 4: 04 5: 05 6: 06 7: 07 8: 08 9: 09 10: 0a 11: 0b ==32011== Invalid read of size 8 ==32011== at 0x401A4A: test_function (audio_fifo.c:181) ==32011== by 0x401AE2: main (audio_fifo.c:193) ==32011== Address 0x540f040 is 0 bytes after a block of size 0 alloc'd ==32011== at 0x4C2C66F: malloc (vg_replace_malloc.c:270) ==32011== by 0x4014E5: allocate_memory (audio_fifo.c:55) ==32011== by 0x401674: read_samples_from_audio_fifo (audio_fifo.c:88) ==32011== by 0x4018C6: test_function (audio_fifo.c:141) ==32011== by 0x401AE2: main (audio_fifo.c:193) ==32011==
yeah, currently taking a look. On 12/21/2016 06:08 PM, Michael Niedermayer wrote: > On Wed, Dec 21, 2016 at 10:53:36PM -0300, James Almer wrote: >> On 12/21/2016 10:49 PM, Michael Niedermayer wrote: >>> On Wed, Dec 21, 2016 at 09:45:39PM -0300, James Almer wrote: >>>> On 12/20/2016 11:36 PM, Michael Niedermayer wrote: >>>>> On Tue, Dec 20, 2016 at 04:53:51PM -0800, Thomas Turner wrote: >>>>>> Signed-off-by: Thomas Turner <thomastdt@googlemail.com> >>>>>> --- >>>>>> libavutil/Makefile | 1 + >>>>>> libavutil/tests/audio_fifo.c | 196 +++++++++++++++++++++++++++++++++++++ >>>>>> tests/fate/libavutil.mak | 4 + >>>>>> tests/ref/fate/audio_fifo | 228 +++++++++++++++++++++++++++++++++++++++++++ >>>>>> 4 files changed, 429 insertions(+) >>>>>> create mode 100644 libavutil/tests/audio_fifo.c >>>>>> create mode 100644 tests/ref/fate/audio_fifo >>>>> applied >>>>> >>>>> thx >>>> This is crashing on some fate clients. >>>> >>>> I noticed it's using malloc and free instead of the av_malloc family, >>>> so maybe it's related to that? >>>> Patch attached in any case, it's proper even if not the reason behind >>>> the crashes. >>> i saw the malloc/free before applying. It seemed to make sense to >>> use libc functions in a test of our public API >>> >>> why is it crashing ? >> I don't know, i can't reproduce it. Seems to be OpenBSD FATE clients only >> as far as i could see. > no openbsd needed, valgrind shows these errors > Thomas, can you look at this ? > > TEST: 1 > > written: 12 > written: 12 > remaining samples in audio_fifo: 24 > > ==32011== Invalid write of size 8 > ==32011== at 0x4016B2: read_samples_from_audio_fifo (audio_fifo.c:92) > ==32011== by 0x4018C6: test_function (audio_fifo.c:141) > ==32011== by 0x401AE2: main (audio_fifo.c:193) > ==32011== Address 0x540f040 is 0 bytes after a block of size 0 alloc'd > ==32011== at 0x4C2C66F: malloc (vg_replace_malloc.c:270) > ==32011== by 0x4014E5: allocate_memory (audio_fifo.c:55) > ==32011== by 0x401674: read_samples_from_audio_fifo (audio_fifo.c:88) > ==32011== by 0x4018C6: test_function (audio_fifo.c:141) > ==32011== by 0x401AE2: main (audio_fifo.c:193) > ==32011== > ==32011== Invalid read of size 8 > ==32011== at 0x401320: av_audio_fifo_read (audio_fifo.c:193) > ==32011== by 0x4016DD: read_samples_from_audio_fifo (audio_fifo.c:95) > ==32011== by 0x4018C6: test_function (audio_fifo.c:141) > ==32011== by 0x401AE2: main (audio_fifo.c:193) > ==32011== Address 0x540f040 is 0 bytes after a block of size 0 alloc'd > ==32011== at 0x4C2C66F: malloc (vg_replace_malloc.c:270) > ==32011== by 0x4014E5: allocate_memory (audio_fifo.c:55) > ==32011== by 0x401674: read_samples_from_audio_fifo (audio_fifo.c:88) > ==32011== by 0x4018C6: test_function (audio_fifo.c:141) > ==32011== by 0x401AE2: main (audio_fifo.c:193) > ==32011== > read: 12 > ==32011== Invalid read of size 8 > ==32011== at 0x4015A8: print_audio_bytes (audio_fifo.c:74) > ==32011== by 0x401900: test_function (audio_fifo.c:146) > ==32011== by 0x401AE2: main (audio_fifo.c:193) > ==32011== Address 0x540f040 is 0 bytes after a block of size 0 alloc'd > ==32011== at 0x4C2C66F: malloc (vg_replace_malloc.c:270) > ==32011== by 0x4014E5: allocate_memory (audio_fifo.c:55) > ==32011== by 0x401674: read_samples_from_audio_fifo (audio_fifo.c:88) > ==32011== by 0x4018C6: test_function (audio_fifo.c:141) > ==32011== by 0x401AE2: main (audio_fifo.c:193) > ==32011== > 00 01 02 03 04 05 06 07 08 09 0a 0b > remaining samples in audio_fifo: 12 > > ==32011== Invalid read of size 8 > ==32011== at 0x401164: av_audio_fifo_peek (audio_fifo.c:150) > ==32011== by 0x401937: test_function (audio_fifo.c:150) > ==32011== by 0x401AE2: main (audio_fifo.c:193) > ==32011== Address 0x540f040 is 0 bytes after a block of size 0 alloc'd > ==32011== at 0x4C2C66F: malloc (vg_replace_malloc.c:270) > ==32011== by 0x4014E5: allocate_memory (audio_fifo.c:55) > ==32011== by 0x401674: read_samples_from_audio_fifo (audio_fifo.c:88) > ==32011== by 0x4018C6: test_function (audio_fifo.c:141) > ==32011== by 0x401AE2: main (audio_fifo.c:193) > ==32011== > peek: > ==32011== Invalid read of size 8 > ==32011== at 0x4015A8: print_audio_bytes (audio_fifo.c:74) > ==32011== by 0x401967: test_function (audio_fifo.c:155) > ==32011== by 0x401AE2: main (audio_fifo.c:193) > ==32011== Address 0x540f040 is 0 bytes after a block of size 0 alloc'd > ==32011== at 0x4C2C66F: malloc (vg_replace_malloc.c:270) > ==32011== by 0x4014E5: allocate_memory (audio_fifo.c:55) > ==32011== by 0x401674: read_samples_from_audio_fifo (audio_fifo.c:88) > ==32011== by 0x4018C6: test_function (audio_fifo.c:141) > ==32011== by 0x401AE2: main (audio_fifo.c:193) > ==32011== > 00 01 02 03 04 05 06 07 08 09 0a 0b > > peek_at: > ==32011== Invalid read of size 8 > ==32011== at 0x401264: av_audio_fifo_peek_at (audio_fifo.c:174) > ==32011== by 0x40199E: test_function (audio_fifo.c:161) > ==32011== by 0x401AE2: main (audio_fifo.c:193) > ==32011== Address 0x540f040 is 0 bytes after a block of size 0 alloc'd > ==32011== at 0x4C2C66F: malloc (vg_replace_malloc.c:270) > ==32011== by 0x4014E5: allocate_memory (audio_fifo.c:55) > ==32011== by 0x401674: read_samples_from_audio_fifo (audio_fifo.c:88) > ==32011== by 0x4018C6: test_function (audio_fifo.c:141) > ==32011== by 0x401AE2: main (audio_fifo.c:193) > ==32011== > 0: > ==32011== Invalid read of size 8 > ==32011== at 0x4015A8: print_audio_bytes (audio_fifo.c:74) > ==32011== by 0x4019D8: test_function (audio_fifo.c:166) > ==32011== by 0x401AE2: main (audio_fifo.c:193) > ==32011== Address 0x540f040 is 0 bytes after a block of size 0 alloc'd > ==32011== at 0x4C2C66F: malloc (vg_replace_malloc.c:270) > ==32011== by 0x4014E5: allocate_memory (audio_fifo.c:55) > ==32011== by 0x401674: read_samples_from_audio_fifo (audio_fifo.c:88) > ==32011== by 0x4018C6: test_function (audio_fifo.c:141) > ==32011== by 0x401AE2: main (audio_fifo.c:193) > ==32011== > 00 > 1: > 01 > 2: > 02 > 3: > 03 > 4: > 04 > 5: > 05 > 6: > 06 > 7: > 07 > 8: > 08 > 9: > 09 > 10: > 0a > 11: > 0b > > ==32011== Invalid read of size 8 > ==32011== at 0x401A4A: test_function (audio_fifo.c:181) > ==32011== by 0x401AE2: main (audio_fifo.c:193) > ==32011== Address 0x540f040 is 0 bytes after a block of size 0 alloc'd > ==32011== at 0x4C2C66F: malloc (vg_replace_malloc.c:270) > ==32011== by 0x4014E5: allocate_memory (audio_fifo.c:55) > ==32011== by 0x401674: read_samples_from_audio_fifo (audio_fifo.c:88) > ==32011== by 0x4018C6: test_function (audio_fifo.c:141) > ==32011== by 0x401AE2: main (audio_fifo.c:193) > ==32011== > > > > _______________________________________________ > ffmpeg-devel mailing list > ffmpeg-devel@ffmpeg.org > http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
On 12/21/2016 11:22 PM, Thomas Turner wrote:
> yeah, currently taking a look.
int tot_elements = !(planes = av_sample_fmt_is_planar(afifo->sample_fmt))
? samples : afifo->channels * samples;
void **data_planes = allocate_memory(sizeof(void*) * planes);
planes is zero when the sample_fmt is not planar, so you end up
calling malloc(0).
It should be channel count if planar, 1 otherwise. I think you
can just call malloc with afifo->nb_buffers * sizeof(void*) as
size.
This is also a good reason to use av_malloc_array() instead of
a plain malloc().
Yes, you're correct. I'll look over the test and make sure there isn't
anymore bugs before sending in the patch. Thanks
On Dec 21, 2016 6:28 PM, "James Almer" <jamrial@gmail.com> wrote:
On 12/21/2016 11:22 PM, Thomas Turner wrote:
> yeah, currently taking a look.
int tot_elements = !(planes = av_sample_fmt_is_planar(afifo->sample_fmt))
? samples : afifo->channels * samples;
void **data_planes = allocate_memory(sizeof(void*) * planes);
planes is zero when the sample_fmt is not planar, so you end up
calling malloc(0).
It should be channel count if planar, 1 otherwise. I think you
can just call malloc with afifo->nb_buffers * sizeof(void*) as
size.
This is also a good reason to use av_malloc_array() instead of
a plain malloc().
From 61898e2580a67250cb3e1fb25a4e170fd00feada Mon Sep 17 00:00:00 2001 From: James Almer <jamrial@gmail.com> Date: Wed, 21 Dec 2016 21:41:20 -0300 Subject: [PATCH] test/audio_fifo: use av_malloc() family of functions Signed-off-by: James Almer <jamrial@gmail.com> --- libavutil/tests/audio_fifo.c | 22 +++++++++------------- 1 file changed, 9 insertions(+), 13 deletions(-) diff --git a/libavutil/tests/audio_fifo.c b/libavutil/tests/audio_fifo.c index dbadded..e9b1470 100644 --- a/libavutil/tests/audio_fifo.c +++ b/libavutil/tests/audio_fifo.c @@ -19,6 +19,7 @@ #include <stdlib.h> #include <stdio.h> #include <inttypes.h> +#include "libavutil/mem.h" #include "libavutil/audio_fifo.c" #define MAX_CHANNELS 32 @@ -50,15 +51,6 @@ static void ERROR(const char *str) exit(1); } -static void* allocate_memory(size_t size) -{ - void *ptr = malloc(size); - if (ptr == NULL){ - ERROR("failed to allocate memory!"); - } - return ptr; -} - static void print_audio_bytes(const TestStruct *test_sample, void **data_planes, int nb_samples) { int p, b, f; @@ -85,11 +77,15 @@ static int read_samples_from_audio_fifo(AVAudioFifo* afifo, void ***output, int int samples = FFMIN(nb_samples, afifo->nb_samples); int tot_elements = !(planes = av_sample_fmt_is_planar(afifo->sample_fmt)) ? samples : afifo->channels * samples; - void **data_planes = allocate_memory(sizeof(void*) * planes); + void **data_planes = av_malloc_array(planes, sizeof(void*)); + if (!data_planes) + ERROR("failed to allocate memory!"); *output = data_planes; for (i = 0; i < afifo->nb_buffers; ++i){ - data_planes[i] = allocate_memory(afifo->sample_size * tot_elements); + data_planes[i] = av_malloc_array(tot_elements, afifo->sample_size); + if (!data_planes[i]) + ERROR("failed to allocate memory!"); } return av_audio_fifo_read(afifo, *output, nb_samples); @@ -178,9 +174,9 @@ static void test_function(const TestStruct test_sample) /* deallocate */ for (i = 0; i < afifo->nb_buffers; ++i){ - free(output_data[i]); + av_freep(&output_data[i]); } - free(output_data); + av_freep(&output_data); av_audio_fifo_free(afifo); } -- 2.10.2