| Message ID | 096C6A08-1B2C-4025-B96A-A8B2B5CD6C28@tagtraum.com |
|---|---|
| State | Accepted |
| Commit | 647fd4b8292e3bfae30b1086aa842a5ee47ee868 |
| Headers | show |
On Thu, Apr 05, 2018 at 02:38:03PM +0200, Hendrik Schreiber wrote: > Hey there, > > I have recently switched to using FFmpeg for conversions of 24bit stereo WAV to 16bit stereo WAV (with dithering). > > For some very large files, I occasionally encountered a segmentation fault in _sum2_float. Unfortunately, I was not able to reproduce the issue in a small test setting, but only in a quite large environment. > > Debugging showed that the issue was caused in function swr_convert_internal() in swresample.c, specifically in line 681 > > s->mix_2_1_f( > conv_src->ch[ch] + off, // out array > preout->ch[ch] + off, // in1 array > s->dither.noise.ch[ch] + s->dither.noise.bps * s->dither.noise_pos + off + len1, // in2 array > s->native_one, // coefficients > 0, // coefficient index 1 > 0, // coefficient index 2 > out_count - len1 // length > ); > > (https://github.com/FFmpeg/FFmpeg/blob/53688b62ca96ad9a3b0e7d201caca61c79a68648/libswresample/swresample.c#L681) > > where dithering is applied. Here, s->mix_2_1_f() is the same as sum2_float(). The in2 array pointer is too large. > > I was able to log values before one of the crashs and found: > > out_count=682 > len1=672 > (out_count - len1)=10 > off=2688 > s->dither.noise.bps =4 > s->dither.noise_pos =130262 > s->dither.noise.count=131072 > s->dither.noise.bps * s->dither.noise_pos + off + len1 = 524408 // in2 start address offset greater than buffer size!!! > > The buffer count has a total size of s->dither.noise.count * s->dither.noise.bps = 524288 > Therefore the start address for the in2 array (3rd argument for mix_2_1_f(...)) is already outside of the buffer. > > I was not able to find a reason for the “+len1” in “s->dither.noise.bps * s->dither.noise_pos + off + len1”. It looks out of place to me. Without it the buffer overrun does not occur. > > “make fate” worked like a charm. > > -hendrik > > tagtraum industries incorporated > 724 Nash Drive > Raleigh, NC 27608 > USA > +1 (919) 809-7797 > http://www.tagtraum.com/ > http://www.beatunes.com/ > > From ef79e9286c4b8b694715e978f48abe1601956c0e Mon Sep 17 00:00:00 2001 > From: Hendrik Schreiber <hs@tagtraum.com> > Date: Thu, 5 Apr 2018 13:58:37 +0200 > Subject: [PATCH] Fix for seg fault in swr_convert_internal() -> sum2_float > during dithering. Removed +len1 in call to s->mix_2_1_f() as I found no > logical explanation for it. After removal, problem was gone. > > Signed-off-by: Hendrik Schreiber <hs@tagtraum.com> > --- > libswresample/swresample.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) will apply thanks [...]
> On Apr 7, 2018, at 02:07, Michael Niedermayer <michael@niedermayer.cc> wrote: > > On Thu, Apr 05, 2018 at 02:38:03PM +0200, Hendrik Schreiber wrote: > > will apply > > thanks Great. Much appreciated. -hendrik
diff --git a/libswresample/swresample.c b/libswresample/swresample.c index f076b6c8cf..5bd39caac4 100644 --- a/libswresample/swresample.c +++ b/libswresample/swresample.c @@ -678,7 +678,7 @@ static int swr_convert_internal(struct SwrContext *s, AudioData *out, int out_co s->mix_2_1_simd(conv_src->ch[ch], preout->ch[ch], s->dither.noise.ch[ch] + s->dither.noise.bps * s->dither.noise_pos, s->native_simd_one, 0, 0, len1); if(out_count != len1) for(ch=0; ch<preout->ch_count; ch++) - s->mix_2_1_f(conv_src->ch[ch] + off, preout->ch[ch] + off, s->dither.noise.ch[ch] + s->dither.noise.bps * s->dither.noise_pos + off + len1, s->native_one, 0, 0, out_count - len1); + s->mix_2_1_f(conv_src->ch[ch] + off, preout->ch[ch] + off, s->dither.noise.ch[ch] + s->dither.noise.bps * s->dither.noise_pos + off, s->native_one, 0, 0, out_count - len1); } else { for(ch=0; ch<preout->ch_count; ch++) s->mix_2_1_f(conv_src->ch[ch], preout->ch[ch], s->dither.noise.ch[ch] + s->dither.noise.bps * s->dither.noise_pos, s->native_one, 0, 0, out_count);