Re: future proofing GConf for backend vtable changes

[Mark McLoughlin <mark skynet ie>]

Hi Joerg,
	Thanks for taking a look ..

On Wed, 2004-03-24 at 08:58, Joerg Barfurth wrote:

> Even with this patch, extending the vtable needs to be coordinated very 
> carefully. Things get dangerous, if a backend supporting one extension 
> meets a gconfd supporting another. Probably their extension function 
> slots will collide and with your patch the backend will merrily call 
> functions that don't do what the backend expects. The size check may 
> convey a false sense of security against such incompatibilities.

	We're not talking about "extensions" here but "additions" - i.e. the
*only* thing I'm trying to protect against here is if we add a few new
members to the vtable that running a new gconfd with an old backend (or
an old gconfd with a new backend) won't work.

	i.e. I'm not trying to support:

  1. Hacker A adds "do_A1", "do_A2", "do_A3"
  2. Hacker B adds "do_B1", "do_B1", "do_B1"
  3. Somehow gconfdA gets run with backendB or vice versa

	Basically, if people deploy their own modifications to the vtable
layout, they do so at their own risk. Its not something that's
worthwhile working around ..


> > 	There's some other stuff in the patch too - this part is the only
> > really relevant bit:
> > 
> >   retval = g_new0(GConfBackendVTable, 1);
> > 
> >   retval = memcpy(retval, vtable, MIN(vtable->vtable_size, sizeof(GConfBackendVTable)));
> > 
> 
> To maintain the proper state, you'd need to add
> 
> + retval->vtable_size = sizeof *retval;
> 
> after the memcpy.

	You mean, retval->vtable_size = sizeof (GConfBackendVTable) ?

	Right, I'll do that for clarity ... but the vtable_size in the copy
will never be used.

	Attaching an updated patch. Includes your change and another minor
change to not introduce an extra unnecessary malloc().

Cheers,
Mark.

Index: gconf-backend.c
===================================================================
RCS file: /cvs/gnome/gconf/gconf/gconf-backend.c,v
retrieving revision 1.22
diff -u -p -r1.22 gconf-backend.c
--- gconf-backend.c	27 Mar 2003 01:41:14 -0000	1.22
+++ gconf-backend.c	24 Mar 2004 09:26:10 -0000
@@ -178,6 +178,62 @@ gconf_backend_file(const gchar* address)
 
 static GHashTable* loaded_backends = NULL;
 
+static gboolean
+gconf_backend_verify_vtable (GConfBackendVTable  *vtable,
+			     GConfBackendVTable  *vtable_copy,
+			     const char          *backend_name,			    
+			     GError             **err)
+{
+  int i;
+  struct
+  {
+    char  *name;
+    gsize  offset;
+  } required_vtable_functions[] = {
+    { "shutdown",        G_STRUCT_OFFSET(GConfBackendVTable, shutdown)        },
+    { "resolve_address", G_STRUCT_OFFSET(GConfBackendVTable, resolve_address) },
+    { "query_value",     G_STRUCT_OFFSET(GConfBackendVTable, query_value)     },
+    { "query_metainfo",  G_STRUCT_OFFSET(GConfBackendVTable, query_metainfo)  },
+    { "set_value",       G_STRUCT_OFFSET(GConfBackendVTable, set_value)       },
+    { "all_entries",     G_STRUCT_OFFSET(GConfBackendVTable, all_entries)     },
+    { "all_subdirs",     G_STRUCT_OFFSET(GConfBackendVTable, all_subdirs)     },
+    { "unset_value",     G_STRUCT_OFFSET(GConfBackendVTable, unset_value)     },
+    { "dir_exists",      G_STRUCT_OFFSET(GConfBackendVTable, dir_exists)      },
+    { "remove_dir",      G_STRUCT_OFFSET(GConfBackendVTable, remove_dir)      },
+    { "set_schema",      G_STRUCT_OFFSET(GConfBackendVTable, set_schema)      },
+    { "sync_all",        G_STRUCT_OFFSET(GConfBackendVTable, sync_all)        },
+    { "destroy_source",  G_STRUCT_OFFSET(GConfBackendVTable, destroy_source)  },
+    { "blow_away_locks", G_STRUCT_OFFSET(GConfBackendVTable, blow_away_locks) }
+  };
+
+  if (!vtable)
+    {
+      gconf_set_error(err,
+		      GCONF_ERROR_FAILED, _("Backend `%s' failed return a vtable\n"),
+		      backend_name);
+      return FALSE;
+    }
+
+  /* Create a copy in case vtable size doesn't match */
+  memcpy(vtable_copy, vtable, MIN(vtable->vtable_size, sizeof(GConfBackendVTable)));
+
+  vtable_copy->vtable_size = sizeof(GConfBackendVTable);
+
+  for (i = 0; i < G_N_ELEMENTS(required_vtable_functions); i++)
+    {
+      if (G_STRUCT_MEMBER_P(vtable_copy, required_vtable_functions[i].offset) == NULL)
+	{
+	  gconf_set_error(err,
+			  GCONF_ERROR_FAILED, _("Backend `%s' missing required vtable member `%s'\n"),
+			  backend_name,
+			  required_vtable_functions[i].name);
+	  return FALSE;
+	}
+    }
+
+  return FALSE;
+}
+
 GConfBackend* 
 gconf_get_backend(const gchar* address, GError** err)
 {
@@ -225,10 +281,9 @@ gconf_get_backend(const gchar* address, 
           
           if (module == NULL)
             {
-              const gchar* error = g_module_error();
               gconf_set_error(err,
                               GCONF_ERROR_FAILED, _("Error opening module `%s': %s\n"),
-                              name, error);
+                              name, g_module_error());
               g_free(name);
               return NULL;
             }
@@ -237,6 +292,10 @@ gconf_get_backend(const gchar* address, 
                                "gconf_backend_get_vtable", 
                                (gpointer*)&get_vtable))
             {
+              gconf_set_error(err,
+                              GCONF_ERROR_FAILED, _("Error initializing module `%s': %s\n"),
+                              name, g_module_error());
+              g_module_close(module);
               g_free(name);
               return NULL;
             }
@@ -245,7 +304,13 @@ gconf_get_backend(const gchar* address, 
 
           backend->module = module;
 
-          backend->vtable = (*get_vtable)();
+	  if (!gconf_backend_verify_vtable((*get_vtable)(), &backend->vtable, name, err))
+	    {
+	      g_module_close(module);
+	      g_free(name);
+	      g_free(backend);
+	      return NULL;
+	    }
               
           backend->name = name;
 
@@ -287,7 +352,7 @@ gconf_backend_unref(GConfBackend* backen
     {
       GError* error = NULL;
       
-      (*backend->vtable->shutdown)(&error);
+      (*backend->vtable.shutdown)(&error);
 
       if (error != NULL)
         {
@@ -319,7 +384,7 @@ gconf_backend_resolve_address (GConfBack
   gchar** iter;
   GConfSource* retval;
 
-  retval = (*backend->vtable->resolve_address)(address, err);
+  retval = (*backend->vtable.resolve_address)(address, err);
 
   if (retval == NULL)
     return NULL;
@@ -362,6 +427,6 @@ gconf_blow_away_locks (const gchar* addr
 
   if (backend != NULL)
     {
-      (*backend->vtable->blow_away_locks) (address);
+      (*backend->vtable.blow_away_locks) (address);
     }
 }
Index: gconf-backend.h
===================================================================
RCS file: /cvs/gnome/gconf/gconf/gconf-backend.h,v
retrieving revision 1.30
diff -u -p -r1.30 gconf-backend.h
--- gconf-backend.h	27 Mar 2003 01:41:14 -0000	1.30
+++ gconf-backend.h	24 Mar 2004 09:26:10 -0000
@@ -33,6 +33,9 @@
 typedef struct _GConfBackendVTable GConfBackendVTable;
 
 struct _GConfBackendVTable {
+  /* Set to sizeof (GConfBackendVTable) - used for future proofing */
+  gsize                  vtable_size;
+
   void                (* shutdown)        (GError** err);
 
   GConfSource*        (* resolve_address) (const gchar* address,
@@ -135,7 +138,7 @@ struct _GConfBackendVTable {
 struct _GConfBackend {
   const gchar* name;
   guint refcount;
-  GConfBackendVTable* vtable;
+  GConfBackendVTable vtable;
   GModule* module;
 };
 
Index: gconf-sources.c
===================================================================
RCS file: /cvs/gnome/gconf/gconf/gconf-sources.c,v
retrieving revision 1.43
diff -u -p -r1.43 gconf-sources.c
--- gconf-sources.c	17 Jun 2002 02:42:54 -0000	1.43
+++ gconf-sources.c	24 Mar 2004 09:26:11 -0000
@@ -78,7 +78,7 @@ gconf_source_free (GConfSource* source)
 
   backend = source->backend;
   
-  (*source->backend->vtable->destroy_source)(source);
+  (*source->backend->vtable.destroy_source)(source);
   
   /* Remove ref held by the source. */
   gconf_backend_unref(backend);
@@ -86,8 +86,8 @@ gconf_source_free (GConfSource* source)
 
 #define SOURCE_READABLE(source, key, err)                  \
      ( ((source)->flags & GCONF_SOURCE_ALL_READABLE) ||    \
-       ((source)->backend->vtable->readable != NULL &&     \
-        (*(source)->backend->vtable->readable)((source), (key), (err))) )
+       ((source)->backend->vtable.readable != NULL &&     \
+        (*(source)->backend->vtable.readable)((source), (key), (err))) )
 
 static gboolean
 source_is_writable(GConfSource* source, const gchar* key, GError** err)
@@ -96,8 +96,8 @@ source_is_writable(GConfSource* source, 
     return FALSE;
   else if ((source->flags & GCONF_SOURCE_ALL_WRITEABLE) != 0)
     return TRUE;
-  else if ((source->backend->vtable->writable != NULL) &&
-           (*source->backend->vtable->writable)(source, key, err))
+  else if ((source->backend->vtable.writable != NULL) &&
+           (*source->backend->vtable.writable)(source, key, err))
     return TRUE;
   else
     return FALSE;
@@ -119,7 +119,7 @@ gconf_source_query_value      (GConfSour
   if ( SOURCE_READABLE(source, key, err) )
     {
       g_return_val_if_fail(err == NULL || *err == NULL, NULL);
-      return (*source->backend->vtable->query_value)(source, key, locales, schema_name, err);
+      return (*source->backend->vtable.query_value)(source, key, locales, schema_name, err);
     }
   else
     return NULL;
@@ -139,7 +139,7 @@ gconf_source_query_metainfo      (GConfS
   if ( SOURCE_READABLE(source, key, err) )
     {
       g_return_val_if_fail(err == NULL || *err == NULL, NULL);
-      return (*source->backend->vtable->query_metainfo)(source, key, err);
+      return (*source->backend->vtable.query_metainfo)(source, key, err);
     }
   else
     return NULL;
@@ -163,7 +163,7 @@ gconf_source_set_value        (GConfSour
   if ( source_is_writable(source, key, err) )
     {
       g_return_val_if_fail(err == NULL || *err == NULL, FALSE);
-      (*source->backend->vtable->set_value)(source, key, value, err);
+      (*source->backend->vtable.set_value)(source, key, value, err);
       return TRUE;
     }
   else
@@ -184,7 +184,7 @@ gconf_source_unset_value      (GConfSour
     {
       g_return_val_if_fail(err == NULL || *err == NULL, FALSE);
 
-      (*source->backend->vtable->unset_value)(source, key, locale, err);
+      (*source->backend->vtable.unset_value)(source, key, locale, err);
       return TRUE;
     }
   else
@@ -204,7 +204,7 @@ gconf_source_all_entries         (GConfS
   if ( SOURCE_READABLE(source, dir, err) )
     {
       g_return_val_if_fail(err == NULL || *err == NULL, NULL);
-      return (*source->backend->vtable->all_entries)(source, dir, locales, err);
+      return (*source->backend->vtable.all_entries)(source, dir, locales, err);
     }
   else
     return NULL;
@@ -222,7 +222,7 @@ gconf_source_all_dirs          (GConfSou
   if ( SOURCE_READABLE(source, dir, err) )
     {
       g_return_val_if_fail(err == NULL || *err == NULL, NULL);
-      return (*source->backend->vtable->all_subdirs)(source, dir, err);
+      return (*source->backend->vtable.all_subdirs)(source, dir, err);
     }
   else
     return NULL;
@@ -240,7 +240,7 @@ gconf_source_dir_exists        (GConfSou
   if ( SOURCE_READABLE(source, dir, err) )
     {
       g_return_val_if_fail(err == NULL || *err == NULL, FALSE);
-      return (*source->backend->vtable->dir_exists)(source, dir, err);
+      return (*source->backend->vtable.dir_exists)(source, dir, err);
     }
   else
     return FALSE;
@@ -258,7 +258,7 @@ gconf_source_remove_dir        (GConfSou
   if ( source_is_writable(source, dir, err) )
     {
       g_return_if_fail(err == NULL || *err == NULL);
-      (*source->backend->vtable->remove_dir)(source, dir, err);
+      (*source->backend->vtable.remove_dir)(source, dir, err);
     }
 }
 
@@ -275,7 +275,7 @@ gconf_source_set_schema        (GConfSou
   if ( source_is_writable(source, key, err) )
     {
       g_return_val_if_fail(err == NULL || *err == NULL, FALSE);
-      (*source->backend->vtable->set_schema)(source, key, schema_key, err);
+      (*source->backend->vtable.set_schema)(source, key, schema_key, err);
       return TRUE;
     }
   else
@@ -285,7 +285,7 @@ gconf_source_set_schema        (GConfSou
 static gboolean
 gconf_source_sync_all         (GConfSource* source, GError** err)
 {
-  return (*source->backend->vtable->sync_all)(source, err);
+  return (*source->backend->vtable.sync_all)(source, err);
 }
 
 /*
@@ -413,8 +413,8 @@ gconf_sources_clear_cache        (GConfS
     {
       GConfSource* source = tmp->data;
 
-      if (source->backend->vtable->clear_cache)
-        (*source->backend->vtable->clear_cache)(source);
+      if (source->backend->vtable.clear_cache)
+        (*source->backend->vtable.clear_cache)(source);
       
       tmp = g_list_next(tmp);
     }