GNOME Bugzilla – Bug 167609
patch to fix esound/gstreamer ESPEAKER interaction bug
Last modified: 2007-01-05 09:46:36 UTC
esdlib changes the value of host in esd_open_sound, which causes some problems further up the stack with gstreamer when trying to make it use ESPEAKER. without this patch, gstreamer-based applications won't work correctly for common thin client setups.
Created attachment 37543 [details] [review] patch to stop changing host variable
Comment on attachment 37543 [details] [review] patch to stop changing host variable thanks for all irc dudes : it is leaking host and might crash if passed NULL
Created attachment 37544 [details] [review] fixed up maybe
Comment on attachment 37544 [details] [review] fixed up maybe >- if ( !host ) host = getenv("ESPEAKER"); >+ if ( rhost ) { >+ host = strdup(rhost); >+ } else { >+ host = strdup(getenv("ESPEAKER")); >+ } This looks like it will crash if ESPEAKER is not set and rhost == NULL (since getenv will return NULL). >+ free (host); >+ It looks like there is a case where this free() will cause a crash too: char display_host[ 256 ]; ... len = min( len, 256 ); strncpy( display_host, display, len ); display_host[ len ] = '\0'; host = display_host; In this case, host will point at a stack allocated character array, and freeing something on the stack will cause heap corruption.
Created attachment 37556 [details] [review] more anal retentiveness
Comment on attachment 37556 [details] [review] more anal retentiveness >- if ( !host ) host = getenv("ESPEAKER"); >+ if ( rhost ) { >+ host = strdup(rhost); >+ } else if ( getenv("ESPEAKER") ) { >+ host = strdup(getenv("ESPEAKER")); >+ } How about writing this bit like below? if (!host) host = getenv("ESPEAKER"); if (host) host = strdup(host); (don't worry about renaming the function parameter to rhost -- that change shouldn't make any difference) >- host = display_host; >+ host = strdup(display_host); That should fix the problem I mentioned earlier However, this iteration of the patch seems to be missing the free(host) bit ...
Created attachment 37557 [details] [review] this time for sure!
Created attachment 37559 [details] [review] are we there yet?
Created attachment 37560 [details] [review] cleanliness c/o pbor
*** Bug 156806 has been marked as a duplicate of this bug. ***
Created attachment 37582 [details] [review] Alternate fix Attaching an alternate patch that fixes the problem inside esd_connect_tcpip where it lives.
*** Bug 169627 has been marked as a duplicate of this bug. ***
Good grief. Doesn't anyone bother to change the algorithm so it doesn't modify the string in-place? Modifying passed strings is so... unnatural. It's like you people _don't even care_ about code quality of esound. OK, I didn't bother to do it either. :) Applied Jan's patch, additionally making sure the correct string is freed after ++host.
> Applied Jan's patch, additionally making sure the correct string is freed after > ++host. So can this bug be closed then?
Don't see why not.