[MPlayer-dev-eng] CONF_TYPE_STRING issue

[Reimar Döffinger]

On Wed, May 04, 2011 at 10:49:44PM +0200, Reimar Döffinger wrote:
> On Wed, May 04, 2011 at 02:51:28PM +0200, Ingo Brückl wrote:
> > Reimar Döffinger wrote on Mon, 2 May 2011 19:16:04 +0200:
> > > The slot data is there to allow "pushing" and "poping" configuration.
> > 
> > And the program-accessible variable is "set" from such slot data (strdup'ed
> > in case of char *variable) which leads to the leak.
> > 
> > When the slot data and the slot itself (one or multiple slots) have been
> > freed, the allocated data for the program-accessible variable (from the last
> > "set slot to variable") is still present and won't get freed. See attached
> > patch proposal (in which I've added a few more preceding lines for better
> > clarification).
> 
> I don't think that's correct.
> In MPlayer code you might have
> char *option = "value";
> to set some default, and your code would end up
> calling free on that "value" pointer if the option
> was never set, which is not correct.
> Though I admit I am not sure if this works correctly
> even currently.
> If you can show and actual memleak (e.g. something that
> valgrind puts in the category "definitely lost") it
> should be easier to figure out where to put a free.

Below seems to work for me, though I need to check it
when I'm properly awake, I currently don't fully
understand why the equality condition is necessary
(though it sure is a good idea anyway as any C++ book will tell :-) ).
Index: m_option.c
===================================================================
--- m_option.c  (revision 33362)
+++ m_option.c  (working copy)
@@ -413,7 +413,7 @@
 }
 
 static void copy_str(const m_option_t* opt,void* dst, const void* src) {
-  if(dst && src) {
+  if(dst && src && VAL(dst) != VAL(src)) {
 #ifndef NO_FREE
     free(VAL(dst)); //FIXME!!!
 #endif
@@ -421,6 +421,12 @@
   }
 }
 
+static void set_str(const m_option_t* opt,void* dst, const void* src) {
+  if(dst && src) {
+    VAL(dst) = VAL(src);
+  }
+}
+
 static void free_str(void* src) {
   if(src && VAL(src)){
 #ifndef NO_FREE
@@ -438,8 +444,8 @@
   parse_str,
   print_str,
   copy_str,
+  set_str,
   copy_str,
-  copy_str,
   free_str
 };