| Message ID | 20171116001431.8588-1-jamrial@gmail.com |
|---|---|
| State | Accepted |
| Commit | f399172d6e842fbdd05c599cdbbb1668c8c354be |
| Headers | show |
2017-11-16 1:14 GMT+01:00 James Almer <jamrial@gmail.com>: > Should fix ticket #6838 > Untested as i can't reproduce. What did you try? Thank you! Carl Eugen
On 11/15/2017 9:21 PM, Carl Eugen Hoyos wrote: > 2017-11-16 1:14 GMT+01:00 James Almer <jamrial@gmail.com>: >> Should fix ticket #6838 > >> Untested as i can't reproduce. > > What did you try? GCC 7.2.0 mingw-w64 x86_64. The buffers were sufficiently aligned every time i decoded the file without this patch. > > Thank you! > > Carl Eugen Pushed.
2017-11-16 1:29 GMT+01:00 James Almer <jamrial@gmail.com>: > On 11/15/2017 9:21 PM, Carl Eugen Hoyos wrote: >> 2017-11-16 1:14 GMT+01:00 James Almer <jamrial@gmail.com>: >>> Should fix ticket #6838 >> >>> Untested as i can't reproduce. >> >> What did you try? > > GCC 7.2.0 mingw-w64 x86_64. The buffers were sufficiently > aligned every time i decoded the file without this patch. It crashes here for many gcc binaries since gcc-4. What exact configure line did you test? Carl Eugen
On 11/15/2017 9:33 PM, Carl Eugen Hoyos wrote: > 2017-11-16 1:29 GMT+01:00 James Almer <jamrial@gmail.com>: >> On 11/15/2017 9:21 PM, Carl Eugen Hoyos wrote: >>> 2017-11-16 1:14 GMT+01:00 James Almer <jamrial@gmail.com>: >>>> Should fix ticket #6838 >>> >>>> Untested as i can't reproduce. >>> >>> What did you try? >> >> GCC 7.2.0 mingw-w64 x86_64. The buffers were sufficiently >> aligned every time i decoded the file without this patch. > > It crashes here for many gcc binaries since gcc-4. > What exact configure line did you test? --enable-gpl --extra-cflags='-D_WIN32_WINNT=0x0602' --prefix=/mingw64 Using the MSYS2 packages (both gcc and mingw-w64), so pretty much a vanilla build. > > Carl Eugen > _______________________________________________ > ffmpeg-devel mailing list > ffmpeg-devel@ffmpeg.org > http://ffmpeg.org/mailman/listinfo/ffmpeg-devel >
2017-11-16 1:49 GMT+01:00 James Almer <jamrial@gmail.com>: > On 11/15/2017 9:33 PM, Carl Eugen Hoyos wrote: >> 2017-11-16 1:29 GMT+01:00 James Almer <jamrial@gmail.com>: >>> On 11/15/2017 9:21 PM, Carl Eugen Hoyos wrote: >>>> 2017-11-16 1:14 GMT+01:00 James Almer <jamrial@gmail.com>: >>>>> Should fix ticket #6838 >>>> >>>>> Untested as i can't reproduce. >>>> >>>> What did you try? >>> >>> GCC 7.2.0 mingw-w64 x86_64. The buffers were sufficiently >>> aligned every time i decoded the file without this patch. >> >> It crashes here for many gcc binaries since gcc-4. >> What exact configure line did you test? > > --enable-gpl --extra-cflags='-D_WIN32_WINNT=0x0602' --prefix=/mingw64 So you did not try the compilation flags used by me or the OP? Carl Eugen
On 11/15/2017 9:50 PM, Carl Eugen Hoyos wrote: > 2017-11-16 1:49 GMT+01:00 James Almer <jamrial@gmail.com>: >> On 11/15/2017 9:33 PM, Carl Eugen Hoyos wrote: >>> 2017-11-16 1:29 GMT+01:00 James Almer <jamrial@gmail.com>: >>>> On 11/15/2017 9:21 PM, Carl Eugen Hoyos wrote: >>>>> 2017-11-16 1:14 GMT+01:00 James Almer <jamrial@gmail.com>: >>>>>> Should fix ticket #6838 >>>>> >>>>>> Untested as i can't reproduce. >>>>> >>>>> What did you try? >>>> >>>> GCC 7.2.0 mingw-w64 x86_64. The buffers were sufficiently >>>> aligned every time i decoded the file without this patch. >>> >>> It crashes here for many gcc binaries since gcc-4. >>> What exact configure line did you test? >> >> --enable-gpl --extra-cflags='-D_WIN32_WINNT=0x0602' --prefix=/mingw64 > > So you did not try the compilation flags used by me or the OP? > > Carl Eugen I think it's clear i didn't. The OP configure line is a massive dump of pointless "--disable" options typical from Gentoo builds, so i didn't even bother looking at it for specific things. And now that i look at yours they are completely different as well, so perhaps it doesn't depend on configure options but something else, like target.
2017-11-16 2:29 GMT+01:00 James Almer <jamrial@gmail.com>: > The OP configure line is a massive dump of pointless > "--disable" options typical from Gentoo builds, Good to know we agree on something. > so i didn't even bother looking at it for specific things. > And now that i look at yours they are completely different > as well And I thought I spent several hours today only to allow you to reproduce a crash to ease testing by providing the necessary configure switches. They are of course identical to what the op provided, do you really suggest I added those stupidities for fun? > so perhaps it doesn't depend on configure options but > something else, like target. Yes, the issue is only reproducible on x86-64. Carl Eugen
On 11/15/2017 10:35 PM, Carl Eugen Hoyos wrote: > 2017-11-16 2:29 GMT+01:00 James Almer <jamrial@gmail.com>: > >> The OP configure line is a massive dump of pointless >> "--disable" options typical from Gentoo builds, > > Good to know we agree on something. > >> so i didn't even bother looking at it for specific things. > >> And now that i look at yours they are completely different >> as well > > And I thought I spent several hours today only to allow you > to reproduce a crash to ease testing by providing the > necessary configure switches. > They are of course identical to what the op provided, do > you really suggest I added those stupidities for fun? Yours was "--enable-small --toolchain=hardened --disable-avx", and only the latter is in the OP configure line. They didn't use neither small or hardened. At no point i tried to offend you in any way. I did not care about the configure line from OP as it was a mess, and did not even get to look at yours since after checking what you reported as the first bad commit (Making proresdec2 use the other decoder's DSP) it was clear to me that said commit did not bother making sure the buffers were aligned for simd optimizations, even if i couldn't reproduce it. Sorry for any misunderstanding. > >> so perhaps it doesn't depend on configure options but >> something else, like target. > > Yes, the issue is only reproducible on x86-64. > > Carl Eugen > _______________________________________________ > ffmpeg-devel mailing list > ffmpeg-devel@ffmpeg.org > http://ffmpeg.org/mailman/listinfo/ffmpeg-devel >
2017-11-16 2:52 GMT+01:00 James Almer <jamrial@gmail.com>: > On 11/15/2017 10:35 PM, Carl Eugen Hoyos wrote: >> 2017-11-16 2:29 GMT+01:00 James Almer <jamrial@gmail.com>: >> >>> The OP configure line is a massive dump of pointless >>> "--disable" options typical from Gentoo builds, >> >> Good to know we agree on something. >> >>> so i didn't even bother looking at it for specific things. >> >>> And now that i look at yours they are completely different >>> as well >> >> And I thought I spent several hours today only to allow you >> to reproduce a crash to ease testing by providing the >> necessary configure switches. >> They are of course identical to what the op provided, do >> you really suggest I added those stupidities for fun? > > Yours was "--enable-small --toolchain=hardened --disable-avx", > and only the latter is in the OP configure line. They didn't use > neither small or hardened. So you are still saying that I added the options for fun? (Thank you, as said it did take significantly more than an hour to find them.) The op used an equivalent for both small and hardened in his configure line that imo is not supported by us, I found a supported configure line that has the same effect on config.mak and the resulting binary. I knew that you were able to solve the riddle without the configure line but I still believe it makes sense for you to also being able to test a fix. I do not understand why you wrote you cannot reproduce when you did not even try to. Since the issue is not easily reproducible with clang, I still wonder if it is reproducible on Windows. Carl Eugen
On 11/15/2017 11:01 PM, Carl Eugen Hoyos wrote: > 2017-11-16 2:52 GMT+01:00 James Almer <jamrial@gmail.com>: >> On 11/15/2017 10:35 PM, Carl Eugen Hoyos wrote: >>> 2017-11-16 2:29 GMT+01:00 James Almer <jamrial@gmail.com>: >>> >>>> The OP configure line is a massive dump of pointless >>>> "--disable" options typical from Gentoo builds, >>> >>> Good to know we agree on something. >>> >>>> so i didn't even bother looking at it for specific things. >>> >>>> And now that i look at yours they are completely different >>>> as well >>> >>> And I thought I spent several hours today only to allow you >>> to reproduce a crash to ease testing by providing the >>> necessary configure switches. >>> They are of course identical to what the op provided, do >>> you really suggest I added those stupidities for fun? >> >> Yours was "--enable-small --toolchain=hardened --disable-avx", >> and only the latter is in the OP configure line. They didn't use >> neither small or hardened. > > So you are still saying that I added the options for fun? No, i didn't say or even imply that. What i said is what's written in that email: Since the two command lines seemed pretty different, then perhaps said options were not related to the crash and it's a target dependent issue. Again, i didn't intend to offend you in any way whatsoever. > (Thank you, as said it did take significantly more than > an hour to find them.) > > The op used an equivalent for both small and hardened in > his configure line that imo is not supported by us, I found > a supported configure line that has the same effect on > config.mak and the resulting binary. > > I knew that you were able to solve the riddle without > the configure line but I still believe it makes sense > for you to also being able to test a fix. > > I do not understand why you wrote you cannot reproduce > when you did not even try to. Because at no point i assumed the configure line you or the OP used could be needed to reproduce a crash like this. A wrong assumption in retrospect, as STRIDE and av_malloc alignment can be affected by configure options, and stack alignment (as it was the issue here) by compiler options. So since i was testing with a different OS than you two, i figured it was pretty much target dependent and decided to send the patch stating it was untested. > Since the issue is not easily reproducible with clang, I > still wonder if it is reproducible on Windows. gcc -U_FORTIFY_SOURCE -D_FORTIFY_SOURCE=2 -fno-strict-overflow -fstack-protector-all -fPIE -c -o /tmp/ffconf.OTilhXct/test.o /tmp/ffconf.OTilhXct/test.c gcc -Wl,-z,relro -Wl,-z,now -fPIE -pie -o /tmp/ffconf.OTilhXct/test.exe /tmp/ffconf.OTilhXct/test.o F:/msys/mingw64/bin/../lib/gcc/x86_64-w64-mingw32/7.2.0/../../../../x86_64-w64-mingw32/bin/ld.exe: unrecognized option '-z' F:/msys/mingw64/bin/../lib/gcc/x86_64-w64-mingw32/7.2.0/../../../../x86_64-w64-mingw32/bin/ld.exe: use the --help option for usage information collect2.exe: error: ld returned 1 exit status C compiler test failed. configure can't even succeed with --toolchain=hardened on mingw-w64, so looks like i wouldn't have been able to reproduce it to begin with.
2017-11-16 3:51 GMT+01:00 James Almer <jamrial@gmail.com>: > configure can't even succeed with --toolchain=hardened on mingw-w64, so > looks like i wouldn't have been able to reproduce it to begin with. Try replacing toolchain with --extra-cflags=-fstack-protector-all: $ configure --extra-cflags=-fstack-protector-all --enable-small --disable-avx Thank you for the useful info, I didn't know that hardened doesn't work on Windows. Carl Eugen
On 11/15/2017 11:55 PM, Carl Eugen Hoyos wrote: > 2017-11-16 3:51 GMT+01:00 James Almer <jamrial@gmail.com>: > >> configure can't even succeed with --toolchain=hardened on mingw-w64, so >> looks like i wouldn't have been able to reproduce it to begin with. > > Try replacing toolchain with --extra-cflags=-fstack-protector-all: > $ configure --extra-cflags=-fstack-protector-all --enable-small --disable-avx BEGIN /tmp/ffconf.pipZnMhH/test.c 1 int main(void){ return 0; } END /tmp/ffconf.pipZnMhH/test.c gcc -fstack-protector-all -c -o /tmp/ffconf.pipZnMhH/test.o /tmp/ffconf.pipZnMhH/test.c gcc -o /tmp/ffconf.pipZnMhH/test.exe /tmp/ffconf.pipZnMhH/test.o F:/msys/tmp/ffconf.pipZnMhH/test.o:test.c:(.text+0x33): undefined reference to `__stack_chk_fail' F:/msys/tmp/ffconf.pipZnMhH/test.o:test.c:(.rdata$.refptr.__stack_chk_guard[.refptr.__stack_chk_guard]+0x0): undefined reference to `__stack_chk_guard' collect2.exe: error: ld returned 1 exit status > > Thank you for the useful info, I didn't know that hardened doesn't > work on Windows. > > Carl Eugen > _______________________________________________ > ffmpeg-devel mailing list > ffmpeg-devel@ffmpeg.org > http://ffmpeg.org/mailman/listinfo/ffmpeg-devel >
On 11/16/2017 12:04 AM, James Almer wrote: > On 11/15/2017 11:55 PM, Carl Eugen Hoyos wrote: >> 2017-11-16 3:51 GMT+01:00 James Almer <jamrial@gmail.com>: >> >>> configure can't even succeed with --toolchain=hardened on mingw-w64, so >>> looks like i wouldn't have been able to reproduce it to begin with. >> >> Try replacing toolchain with --extra-cflags=-fstack-protector-all: >> $ configure --extra-cflags=-fstack-protector-all --enable-small --disable-avx > > BEGIN /tmp/ffconf.pipZnMhH/test.c > 1 int main(void){ return 0; } > END /tmp/ffconf.pipZnMhH/test.c > gcc -fstack-protector-all -c -o /tmp/ffconf.pipZnMhH/test.o > /tmp/ffconf.pipZnMhH/test.c > gcc -o /tmp/ffconf.pipZnMhH/test.exe /tmp/ffconf.pipZnMhH/test.o > F:/msys/tmp/ffconf.pipZnMhH/test.o:test.c:(.text+0x33): undefined > reference to `__stack_chk_fail' > F:/msys/tmp/ffconf.pipZnMhH/test.o:test.c:(.rdata$.refptr.__stack_chk_guard[.refptr.__stack_chk_guard]+0x0): > undefined reference to `__stack_chk_guard' > collect2.exe: error: ld returned 1 exit status Looks like adding --enable-ldflags=-fstack-protector-all to the above command line makes it work. > >> >> Thank you for the useful info, I didn't know that hardened doesn't >> work on Windows. >> >> Carl Eugen >> _______________________________________________ >> ffmpeg-devel mailing list >> ffmpeg-devel@ffmpeg.org >> http://ffmpeg.org/mailman/listinfo/ffmpeg-devel >> >
diff --git a/libavcodec/proresdec2.c b/libavcodec/proresdec2.c index e9e0153ee9..d97e264e44 100644 --- a/libavcodec/proresdec2.c +++ b/libavcodec/proresdec2.c @@ -517,8 +517,8 @@ static int decode_slice_thread(AVCodecContext *avctx, void *arg, int jobnr, int int luma_stride, chroma_stride; int y_data_size, u_data_size, v_data_size, a_data_size; uint8_t *dest_y, *dest_u, *dest_v, *dest_a; - int16_t qmat_luma_scaled[64]; - int16_t qmat_chroma_scaled[64]; + LOCAL_ALIGNED_16(int16_t, qmat_luma_scaled, [64]); + LOCAL_ALIGNED_16(int16_t, qmat_chroma_scaled,[64]); int mb_x_shift; int ret;
Should fix ticket #6838 Signed-off-by: James Almer <jamrial@gmail.com> --- Untested as i can't reproduce. libavcodec/proresdec2.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)