GNOME Bugzilla – Bug 573369
[gstfaad] Memory corruption and segfault
Last modified: 2009-03-29 22:08:27 UTC
When using the latest faad2 release, using the gstfaad plugin segfaults. It seems to be some kind of memory corruption, since it happens just after the return from gst_faad_setcaps. Platform: amd64 Distribution: gentoo ~amd64 (up to date) GStreamer: git of everything
+ Trace 212951
This breaks swfdec (just try playing any youtube video :) )
I've been digging a bit deeper at this (both faad2-2.7 and gstfaad compiled with -O0): The corruption seems to happen during the call to faacDecInit2, since the stack is corrupted when you return from that function call (all local variables are busted). I'll try to dig in deeper. P.S. It would be awesome if I could figure out faad's bugtracker... or mailing list :(
Created attachment 129783 [details] [review] Avoid stack screwup The problem is that in faad2-2.7, somebody changed the *INTERNAL* prototype of the DecInit2 method without changing the public '/usr/include' header. previously: int8_t NEAACDECAPI NeAACDecInit2(NeAACDecHandle hDecoder, uint8_t *pBuffer, uint32_t SizeOfDecoderSpecificInfo, uint32_t *samplerate, uint8_t *channels) faad-2.7: char NEAACDECAPI NeAACDecInit2(NeAACDecHandle hpDecoder, unsigned char *pBuffer, unsigned long SizeOfDecoderSpecificInfo, unsigned long *samplerate, unsigned char *channels) While this looks very innocent, the problem is that we *WERE* passing it previously a pointer to a guint32 value, which was coherent with the previous (internal) prototype. But on 64bit ... unsigned long is 64bit wide. So we're passing it a pointer to a guint32 value, and it's assuming it's a 64bit value, ergo writing 8 bytes instead of 4 at *samplerate... and writing over the stack.
and it gets worse... This was all due to our 'redefinition' of the init prototypes at the top of gstfaad.c extern long faacDecInit (faacDecHandle, guint8 *, guint32, guint32 *, guint8 *); extern int8_t faacDecInit2 (faacDecHandle, guint8 *, guint32, guint32 *, guint8 *); We force the samplerate argument to be a (guint32 *), whereas they've now fixed their issues, and the public headers are now in sync with the internal ones. What do we do ? pushing this up to blocker.
Created attachment 129790 [details] [review] A more extensive fixup.
Ugh, so they changed the ABI (on 64 bit systems at least). Great. We need to detect which version we're using, and then put some #ifdef HAVE_FAAD_2_7 around our override-prototypes, etc.
Created attachment 130031 [details] [review] Proper patch with detection in configure This updated patch fixes the issue the proper way. I added a detection of faad2-2.7 in configure.ac which will define HAVE_FAAD_27 in config.gh if present. Then all the "let's redefine the headers" cruft in gstfaad.c is dropped if HAVE_FAAD_27 is defined.
No other way to detect the version (e.g. newly-added exported symbol or so)? AC_TRY_RUN() is always a bit problematic because it fails when cross-compiling.
I welcome a better fix. My autocrack skills are not as good as one might expect. But since the freeze is in 2 days... that better fix had better arrive quickly. Otherwise, as MikeS suggested, since they fixed their headers, I'll replace 'gulong' by 'unsigned long'.
Maybe this helps (and works for you as well): commit 60080ee20bb900221ff2cfb8ca44db8cb3010542 Author: Tim-Philipp Müller <tim.muller@collabora.co.uk> Date: Wed Mar 4 20:37:51 2009 +0000 configure: detect faad's minor version and define FAAD2_MINOR_VERSION in config.h
neat, I'll adjust my patch and commit it!
Created attachment 130099 [details] [review] gstfaad.c only patch using latest commits This new patch uses the latest additions to config.h by Tim, and uses 'unsigned long' for the samplerate argument if we're using a 'fixed' faad (let's just hope they don't break it again in future versions...)
commit bdc9c5618a6d8f49c1efa3c05319d84087c0f455 Author: Edward Hervey <bilboed@bilboed.com> Date: Fri Mar 6 12:42:50 2009 +0100 faad: Use the public headers if faad2 >= 2.7. Fixes #573369 Since faad2-2.7, the public function prototypes are in sync with the actual function prototypes used internally in libfaad.
*** Bug 574407 has been marked as a duplicate of this bug. ***
*** Bug 574877 has been marked as a duplicate of this bug. ***
*** Bug 575400 has been marked as a duplicate of this bug. ***
*** Bug 575840 has been marked as a duplicate of this bug. ***
*** Bug 577230 has been marked as a duplicate of this bug. ***