GNOME Bugzilla – Bug 349180
gst_parse_launch is not reentrant (or recursively callable)
Last modified: 2007-05-03 16:12:02 UTC
Steps to reproduce: By calling gst_parse_launch recursively Stack trace: This is an excerpt of valgrind analysis ==22502== Invalid write of size 1 ==22502== at 0x40BD49E: _gst_parse_yylex (lex._gst_parse_yy.c:1059) ==22502== by 0x40C4374: _gst_parse__yylex (grammar.y:797) ==22502== by 0x40C0F6E: _gst_parse__yyparse (grammar.tab.c:1629) ==22502== by 0x40C4478: _gst_parse_launch (grammar.y:838) ==22502== by 0x40B6D50: gst_parse_launch (gstparse.c:159) ==22502== by 0x40B6C7C: gst_parse_launchv (gstparse.c:121) ==22502== by 0x804B1F1: main (gst-launch.c:603) ==22502== Address 0x48AAC40 is 16 bytes inside a block of size 18 free'd ==22502== at 0x401CFCF: free (vg_replace_malloc.c:235) ==22502== by 0x40BF380: _gst_parse_yyfree (lex._gst_parse_yy.c:2138) ==22502== by 0x40BEB02: _gst_parse_yy_delete_buffer (lex._gst_parse_yy.c:1726) ==22502== by 0x40C453A: _gst_parse_launch (grammar.y:845) ==22502== by 0x40B6D50: gst_parse_launch (gstparse.c:159) ==22502== by 0x40AD840: gst_parse_bin_from_description (gstutils.c:3151) ==22502== by 0x463C67E: gsmart_audio_sink_set_property (gsmart-audiosink.c:369) ==22502== by 0x423267A: g_object_set_property (in /usr/lib/libgobject-2.0.so.0.1000.3) ==22502== by 0x40BF5DF: gst_parse_element_set (grammar.y:289) ==22502== by 0x40C1579: _gst_parse__yyparse (grammar.y:581) ==22502== by 0x40C4478: _gst_parse_launch (grammar.y:838) ==22502== by 0x40B6D50: gst_parse_launch (gstparse.c:159) Other information: it seems that 2 functions should be used to push/pop the state of the lexer. _gst_parse_yypush_buffer_state /pop I will provide a patch ASAP if it's the right way to correct this behavior.
Created attachment 69889 [details] [review] This patch solve the problem for me (and changes some other things cause of bison/flex mess)
the patch seems to miss something: cc1: warnings being treated as errors grammar.tab.c: In function '_gst_parse__yyparse': grammar.tab.c:1638: warning: implicit declaration of function '_gst_parse__yylex' make: *** [libgstparse_la-grammar.tab.lo] Fehler 1
Created attachment 69892 [details] [review] The missing Makefile.am update in the previous patch Indeed, the previous patch was lacking of the Makefile.am modification. I don't know why, but I was forced to remove one underscore. I hope it won't be terrible..
it works now! Althogh I have to escape spaces in "" args: gsmartaudiosink sink-description="alsasink\ device=plughw:1,0" IMHO this fix can be committed, waiting for another +1 (thaytan?)
Looks fine to me. Marc-Andre: The makefile modification is needed because you removed the yylex() wrapper that was calling the lexer method. It wasn't doing anything else useful, so I think it's cleaner to change the makefile the way you have. Stefan: You must be doing something wrong when you pass the stuff into gst-parse-launch, because quoted stuff with spaces should continue to work the same way. c.f: gst-launch fakesrc name="test 1 fakesrc" ! fakesink works the same both with and without the patch Lastly, there's a fixme in yyerror about placing the error into a GError, which can be done now that the graph is being passed to yyerror. I can fix that after you commit though if you're not sure what I mean.
sorry, escaping the space is needed in gstreamer-properties (gconf) for some odd reason.
2006-07-30 Stefan Kost <ensonic@users.sf.net> Patch by: Marc-Andre Lureau <marcandre.lureau@gmail.com> * gst/parse/Makefile.am: * gst/parse/grammar.y: * gst/parse/parse.l: push & pop the state of the lexer for reentrant use case Fixes #349180
reverted patch as it required to bump the flex dependency to 2.5.31, where fc4/5 seem to ship only the ancient 2.5.4a :( February 21, 2006 * flex version 2.5.33 March 3, 2003 * flex version 2.5.31 June 27, 1997 * flex version 2.5.4a
What do you think of shipping the generated source code? I can propose a patch that will use it if the required version of flex is higher than the one available on the system.
Is this related to the problem I am seeing with gaim: Program received signal SIGSEGV, Segmentation fault. [Switching to Thread -1231995984 (LWP 10092)] 0x418b6dee in _gst_parse_yylex (lvalp=0xb69122d8) at lex._gst_parse_yy.c:1224 (see http://bugs.debian.org/cgi-bin/bugreport.cgi?bug=403343 )?
I tried this again disabling the gaim icr-helper plugin, but in the 2 subsequent startup attempts I received: Program received signal SIGSEGV, Segmentation fault. [Switching to Thread -1222599760 (LWP 3589)] 0x46db2bde in _gst_parse_yylex (lvalp=0xb72082d8) at lex._gst_parse_yy.c:1224 1224 lex._gst_parse_yy.c: No such file or directory. in lex._gst_parse_yy.c
Created attachment 84824 [details] [review] new patch, fix the parse-param error with bison 2.3 (scanner/graph bug)
Works perfect... Thomas, shall I commit this patch + a pre-generated version of the sources which is used when a too old bison/flex is installed (checked with a configure check)?
(In reply to comment #13) Sebastien, I think it is the best solution we can reach, and quite common (I proposed this in #9, but nobody replied, and no urgency has been discussed on this patch). Last week, someone sent a gconfaudiosink patch on gst-dev. My guess is that it will fail quite miserably if the parser is not reentrant :)
It sounds acceptable as long as it is preferring to build the source on its own.
Ok, thanks. Using bison/flex to generate the source would always be preffered unless there is a too old bison/flex installed. I'll care for this later...
Created attachment 85273 [details] [review] gst-parse.diff Ok, here's a patch that does the required magic... The HAVE_MT_SAVE_FLEX define is obsolete now as the parser sources that are used are always generated by a new enough flex, thus the #ifdefs are removed. The pre-generated sources are always updated if the flex/bison sources are modified, thus every developer who intents to change the parser needs a new enough flex and bison installed. The pre-generated sources should always be committed after a change, otherwise the build bot will complain. Can someone please review this patch?
Created attachment 85275 [details] [review] gst-parse.diff Fix build of the docs...
I've tested it under ubuntu 6.10 and opensuse 10.2 and it works there.
This is fixed now in CVS... 2007-04-18 Sebastian Dröge <slomo@circular-chaos.org> * m4/gst-parser.m4: Check for flex >= 2.5.31 and set GENERATE_PARSER if we have at least that version. Otherwise use pre-generated parser sources as we can't raise the required flex version. HAVE_MT_SAVE_FLEX is obsolete now as we use a new enough flex version anyway. First part of #349180 2007-04-18 Sebastian Dröge <slomo@circular-chaos.org> Patch by: Marc-Andre Lureau <marcandre dot lureau at gmail dot com> * gst/parse/Makefile.am: * gst/parse/grammar.y: * gst/parse/parse.l: Make the parser reentrant and recursively callable. This requires flex >= 2.5.31, for older versions pregenerated sources are used as we can't bump the build dependency. Finally fixes #349180. * gst/gstparse.c: (gst_parse_launch): Drop the HAVE_MT_SAVE_FLEX #ifdefs as we always use a new enough flex now anyway. * docs/gst/Makefile.am: * docs/gst/Makefile.am: * gst/parse/grammar.tab.pre.c: (__gst_parse_strdup), (__gst_parse_strfree), (__gst_parse_link_new), (__gst_parse_link_free), (__gst_parse_chain_new), (__gst_parse_chain_free), (SET_ERROR), (YYPRINTF), (gst_parse_element_set), (gst_parse_free_link), (gst_parse_found_pad), (gst_parse_perform_delayed_link), (gst_parse_perform_link), (yytnamerr), (yysyntax_error), (yyerror), (_gst_parse_launch): * gst/parse/grammar.tab.pre.h: * gst/parse/lex._gst_parse_yy.pre.c: (PRINT), (yy_get_next_buffer), (yy_get_previous_state), (yy_try_NUL_trans), (input), (_gst_parse_yyrestart), (_gst_parse_yy_switch_to_buffer), (_gst_parse_yy_load_buffer_state), (_gst_parse_yy_create_buffer), (_gst_parse_yy_delete_buffer), (_gst_parse_yy_init_buffer), (_gst_parse_yy_flush_buffer), (_gst_parse_yypush_buffer_state), (_gst_parse_yypop_buffer_state), (_gst_parse_yyensure_buffer_stack), (_gst_parse_yy_scan_buffer), (_gst_parse_yy_scan_string), (_gst_parse_yy_scan_bytes), (yy_fatal_error), (_gst_parse_yyget_extra), (_gst_parse_yyget_lineno), (_gst_parse_yyget_column), (_gst_parse_yyget_in), (_gst_parse_yyget_out), (_gst_parse_yyget_leng), (_gst_parse_yyget_text), (_gst_parse_yyset_extra), (_gst_parse_yyset_lineno), (_gst_parse_yyset_column), (_gst_parse_yyset_in), (_gst_parse_yyset_out), (_gst_parse_yyget_debug), (_gst_parse_yyset_debug), (_gst_parse_yyget_lval), (_gst_parse_yyset_lval), (_gst_parse_yylex_init), (yy_init_globals), (_gst_parse_yylex_destroy), (yy_flex_strncpy), (yy_flex_strlen), (_gst_parse_yyalloc), (_gst_parse_yyrealloc), (_gst_parse_yyfree): If the installed flex version is too old use pre-generated parser sources. These pre-generated parser sources are always updated when the actual flex/bison sources change but require everybody who wants to change something in the parser to have flex >= 2.5.31 installed.