Re: core segfaults (Yahoo?)

On Wed, 10 Mar 2004, A. Craig West wrote:

> ==27377== Invalid read of size 1
> ==27377==    at 0x40020A0F: strcmp (in /usr/lib/valgrind/vgskin_memcheck.so)
> ==27377==    by 0x8053984: plugin_by_name (plugin.c:117)
> ==27377==    by 0x8053A86: load_plugin (plugin.c:161)
> ==27377==    by 0x8053926: load_plugins (plugin.c:103)
> ==27377==    Address 0x41459D98 is 0 bytes inside a block of size 31 free'd
> ==27377==    at 0x40027E67: free (in /usr/lib/valgrind/vgskin_memcheck.so)
> ==27377==    by 0x8053B41: load_plugin (plugin.c:188)
> ==27377==    by 0x8053926: load_plugins (plugin.c:103)
> ==27377==    by 0x8051A1D: main (main.c:230)

I think I like this valgrind thing :-) For some reason (possibly even an
intentional one, but I can't think of it) we only add loaded plugins to the
list of loaded plugins in case of an error, and when we do this, we copy the
pointer to the name of the plugin instead of the name, and then free the
name. This patch deals with all of that, in a relatively sane way, I hope.

---------------------- Patch Begins ---------------------
Index: core/src/plugin.c
===================================================================
--- core/src/plugin.c	(revision 306)
+++ core/src/plugin.c	(working copy)
@@ -121,6 +121,18 @@
 }
 
 #if HAVE_LIBLTDL  // MOTHBALLED
+static eb_plugin * create_plugin(char * fnam)
+{
+  eb_plugin * plugin = (eb_plugin *)malloc(sizeof(eb_plugin));
+  plugin->filename=(char *)strdup(fnam);
+  plugin->info=NULL;
+  plugin->status=PLUGIN_NOT_LOADED;
+  plugin->errormsg=NULL;
+  plugins=e_list_append(plugins, plugin);
+
+  return plugin;
+}
+
 static void add_error_entry(char * fnam, char * errormsg)
 {
   eb_plugin * plugin;
@@ -128,20 +140,23 @@
 
   if((plugin=plugin_by_name(fnam))==NULL)
   {
-    plugin=(eb_plugin *)malloc(sizeof(eb_plugin));
+    plugin=create_plugin(fnam);
     isnew=1;
   }
 
-  plugin->filename=fnam;
+  free(plugin->info);
+  free(plugin->errormsg);
+
   plugin->info=NULL;
   plugin->status=PLUGIN_CANNOT_LOAD;
   plugin->errormsg=(char *)strdup(errormsg);
 
   if(isnew)
   {
-    plugins=e_list_append(plugins, plugin);
     new_plugin_notify(plugin);
-  } else {
+  }
+  else
+  {
     plugin_update_notify(plugin);
   }
 }
@@ -154,6 +169,7 @@
   dlhandle dat;
   eb_plugin_info * info;
   eb_plugin * plugin;
+  int isnew=0;
 
   full_path=(char *)malloc(strlen(path)+strlen(fnam)+2);
   sprintf(full_path, "%s/%s", path, fnam);
@@ -192,12 +208,27 @@
   // Now, we dispatch based on plugin type. Are we all sitting comfortably?
 
   if(plugin==NULL)
-  { plugin=(eb_plugin *)malloc(sizeof(eb_plugin)); }
+  {
+    plugin=create_plugin(full_path);
+    isnew=1;
+  }
 
+  free(plugin->info);
+  free(plugin->errormsg);
+
   plugin->status=PLUGIN_LOADED;
   plugin->errormsg=NULL;
   plugin->info=info;
 
+  if(isnew)
+  {
+    new_plugin_notify(plugin);
+  }
+  else
+  {
+    plugin_update_notify(plugin);
+  }
+
   free(full_path);
 
   info->init();
---------------------- Patch Ends---------------------

-- 
Craig West         Ph: (416) 666-1645	|  It's not a bug,
[email protected]              	|  It's a feature...