From 7e47ce8951b132e1fd2ebf5fb35dc165c2b9dbfd Mon Sep 17 00:00:00 2001
From: Simon McVittie <smcv@collabora.com>
Date: Wed, 8 Nov 2017 13:25:35 +0000
Subject: [PATCH] Maintain a list of capsule structures, not a list of
 capsule_metadata

This enforces the following invariants:

* Capsules and capsule_metadata are related 1:1
* All valid capsules are present in the list
* Only valid capsules are present in the list

Signed-off-by: Simon McVittie <smcv@collabora.com>
---
 capsule/capsule-dlmopen.c  |  24 +++----
 capsule/capsule-init.c     | 143 +++++++++++++++++++------------------
 capsule/capsule-private.h  |   2 +-
 capsule/capsule-wrappers.c |  23 +++---
 4 files changed, 96 insertions(+), 96 deletions(-)

diff --git a/capsule/capsule-dlmopen.c b/capsule/capsule-dlmopen.c
index e60dcbfe0..d6fd6208c 100644
--- a/capsule/capsule-dlmopen.c
+++ b/capsule/capsule-dlmopen.c
@@ -252,16 +252,16 @@ capsule_load (const capsule cap,
     {
         // first fetch the subcapsule metadata for this specific
         // target DSO if it exists:
-        for( size_t n = 0; n < _capsule_metadata_list->next; n++ )
+        for( size_t n = 0; n < _capsule_list->next; n++ )
         {
-            capsule_metadata *m = ptr_list_nth_ptr( _capsule_metadata_list, n );
+            capsule other = ptr_list_nth_ptr( _capsule_list, n );
             DEBUG( DEBUG_CAPSULE, "checking metadata %d for %p, %s",
-                   (int) n, m, m ? m->soname : "---" );
+                   (int) n, other, other ? other->meta->soname : "---" );
 
-            if( !m || strcmp( dso, m->soname ) )
+            if( !other || strcmp( dso, other->meta->soname ) )
                 continue;
 
-            cm = m;
+            cm = other->meta;
             break;
         }
 
@@ -271,22 +271,22 @@ capsule_load (const capsule cap,
         // next, if we have subcapsule metadata, see if there's
         // a Lmid_t value alrady allocated for it:
         if( cm && cm->namespace == LM_ID_NEWLM )
-            for( size_t n = 0; n < _capsule_metadata_list->next; n++ )
+            for( size_t n = 0; n < _capsule_list->next; n++ )
             {
-                capsule_metadata *m = ptr_list_nth_ptr( _capsule_metadata_list, n );
+                capsule other = ptr_list_nth_ptr( _capsule_list, n );
 
                 // this subcapsule is for a different prefix. skip it:
                 //DEBUG( DEBUG_CAPSULE, "checking prefix match: %p vs %p",
-                //       cap->prefix, m->active_prefix );
-                if( strcmp( cap->prefix, m->active_prefix ) )
+                //       cap->prefix, other->meta->active_prefix );
+                if( strcmp( cap->prefix, other->prefix ) )
                     continue;
 
                 // we found a pre-allocated Lmid_t. Use it:
-                if( m->namespace != LM_ID_NEWLM )
+                if( other->meta->namespace != LM_ID_NEWLM )
                 {
                     DEBUG( DEBUG_CAPSULE, "re-using LM: %ld from %s",
-                           m->namespace, m->soname );
-                    *namespace = cm->namespace = m->namespace;
+                           other->meta->namespace, other->meta->soname );
+                    *namespace = cm->namespace = other->meta->namespace;
                     break;
                 }
             }
diff --git a/capsule/capsule-init.c b/capsule/capsule-init.c
index dd4274085..e4f5cdda2 100644
--- a/capsule/capsule-init.c
+++ b/capsule/capsule-init.c
@@ -1,6 +1,8 @@
 #include <capsule/capsule.h>
 #include "capsule/capsule-private.h"
 #include "utils/utils.h"
+
+#include <assert.h>
 #include <stdlib.h>
 #include <stdio.h>
 #include <dlfcn.h>
@@ -52,15 +54,10 @@ typedef struct _capsule_namespace
 
 static ptr_list *namespaces = NULL;
 
-ptr_list *_capsule_metadata_list  = NULL;
+ptr_list *_capsule_list  = NULL;
 dlsymfunc capsule_dl_symbol = NULL;
 dlopnfunc capsule_dl_open   = NULL;
 
-static int ptr_equal (const void *a, const void *b)
-{
-    return (a == b) ? 1 : 0;
-}
-
 static int str_equal (const void *a, const void *b)
 {
     if( a == b )
@@ -180,9 +177,9 @@ get_capsule_metadata (struct link_map *map, const char *only)
             cap->seen.all  = ptr_list_alloc( 32 );
             cap->seen.some = ptr_list_alloc( 32 );
             meta->handle = cap;
+            ptr_list_push_ptr( _capsule_list, cap );
         }
 
-        ptr_list_add_ptr( _capsule_metadata_list, meta, ptr_equal );
         DEBUG( DEBUG_CAPSULE,
                "found metadata for %s … %s at %p (capsule: %p)",
                meta->active_prefix, meta->soname, meta, cap );
@@ -268,44 +265,44 @@ update_metadata (const char *match)
     // ie all excludes for /host should be in one exclude list,
     // all nowrap entries for /host should bein another list, all
     // excludes for /badgerbadger should be in another, etc etc:
-    for( size_t i = 0; i < _capsule_metadata_list->next; i++ )
+    for( size_t i = 0; i < _capsule_list->next; i++ )
     {
-        capsule_metadata *cm = ptr_list_nth_ptr( _capsule_metadata_list, i );
+        capsule cap = ptr_list_nth_ptr( _capsule_list, i );
 
-        if( !cm || cm->closed )
+        if( !cap )
             continue;
 
-        capsule_namespace *ns = get_namespace( cm->active_prefix );
+        capsule_namespace *ns = get_namespace( cap->prefix );
 
-        add_new_strings_to_ptrlist( ns->exclusions, cm->exclude );
-        add_new_strings_to_ptrlist( ns->exports,    cm->export  );
-        add_new_strings_to_ptrlist( ns->nowrap,     cm->nowrap  );
+        add_new_strings_to_ptrlist( ns->exclusions, cap->meta->exclude );
+        add_new_strings_to_ptrlist( ns->exports,    cap->meta->export  );
+        add_new_strings_to_ptrlist( ns->nowrap,     cap->meta->nowrap  );
     }
 
     // now squash the meta data ptr_lists into char** that
     // the underlying infrastructure actually uses:
-    for( size_t i = 0; i < _capsule_metadata_list->next; i++ )
+    for( size_t i = 0; i < _capsule_list->next; i++ )
     {
-        capsule_metadata *cm = ptr_list_nth_ptr( _capsule_metadata_list, i );
+        capsule cap = ptr_list_nth_ptr( _capsule_list, i );
 
-        if( !cm || cm->closed )
+        if( !cap )
             continue;
 
-        capsule_namespace *ns = get_namespace( cm->active_prefix );
+        capsule_namespace *ns = get_namespace( cap->prefix );
 
-        free_strv( cm->combined_exclude );
-        free_strv( cm->combined_export  );
-        free_strv( cm->combined_nowrap  );
+        free_strv( cap->meta->combined_exclude );
+        free_strv( cap->meta->combined_export  );
+        free_strv( cap->meta->combined_nowrap  );
 
-        cm->combined_exclude = cook_list( ns->exclusions );
-        cm->combined_export  = cook_list( ns->exports );
-        cm->combined_nowrap  = cook_list( ns->nowrap );
+        cap->meta->combined_exclude = cook_list( ns->exclusions );
+        cap->meta->combined_export  = cook_list( ns->exports );
+        cap->meta->combined_nowrap  = cook_list( ns->nowrap );
     }
 }
 
 static void __attribute__ ((constructor)) _init_capsule (void)
 {
-    _capsule_metadata_list = ptr_list_alloc( 16 );
+    _capsule_list = ptr_list_alloc( 16 );
 
     set_debug_flags( secure_getenv("CAPSULE_DEBUG") );
 
@@ -317,16 +314,16 @@ static void __attribute__ ((constructor)) _init_capsule (void)
     if( !(debug_flags & DEBUG_CAPSULE) )
         return;
 
-    for( unsigned int x = 0; x < _capsule_metadata_list->next; x++ )
+    for( unsigned int x = 0; x < _capsule_list->next; x++ )
     {
-        capsule_metadata *cm = _capsule_metadata_list->loc[ x ].ptr;
+        capsule cap = _capsule_list->loc[ x ].ptr;
 
-        if( !cm || cm->closed )
+        if( !cap )
             continue;
 
-        const char **soname = cm->exclude;
+        const char **soname = cap->meta->exclude;
 
-        DEBUG( DEBUG_CAPSULE, "[%02d] %s metadata:\n", x, cm->soname );
+        DEBUG( DEBUG_CAPSULE, "[%02d] %s metadata:\n", x, cap->meta->soname );
 
         while( soname && *soname )
             fprintf( stderr, "    %s\n", *(soname++) );
@@ -376,47 +373,42 @@ capsule_get_prefix (const char *dflt, const char *soname)
     return NULL;
 }
 
-static capsule_metadata *
-get_cached_metadata (const char *soname)
+static capsule
+get_capsule_by_soname (const char *soname)
 {
-    size_t i = 0;
-    capsule_metadata *meta = NULL;
-
-    for( size_t n = 0; n < _capsule_metadata_list->next; n++ )
+    for( size_t n = 0; n < _capsule_list->next; n++ )
     {
-        capsule_metadata *cm = ptr_list_nth_ptr( _capsule_metadata_list, n );
+        capsule cap = ptr_list_nth_ptr( _capsule_list, n );
 
-        if( !cm || cm->closed || strcmp( cm->soname, soname ) )
+        if( !cap || strcmp( cap->meta->soname, soname ) )
             continue;
 
-        i = n;
-        meta = cm;
-        break;
+        DUMP_METADATA(n, cap->meta);
+        return cap;
     }
 
-    if(meta)
-        DUMP_METADATA(i, meta);
-
-    return meta;
+    return NULL;
 }
 
 capsule
 capsule_init (const char *soname)
 {
-    capsule_metadata *meta = NULL;
+    capsule cap;
+
+    DEBUG( DEBUG_CAPSULE, "Initializing shim library %s", soname );
 
-    meta = get_cached_metadata( soname );
+    cap = get_capsule_by_soname( soname );
 
-    if( !meta )
+    if( !cap )
     {
         DEBUG( DEBUG_CAPSULE, "no metadata for %s registered: "
                "may be a dlopened capsule", soname );
         DEBUG( DEBUG_CAPSULE, "updating capsule metadata" );
         update_metadata( soname );
-        meta = get_cached_metadata( soname );
+        cap = get_capsule_by_soname( soname );
     }
 
-    if( !meta )
+    if( !cap )
     {
         fprintf( stderr,
                  "libcapsule: %s: Fatal error: cannot initialize shim "
@@ -425,21 +417,21 @@ capsule_init (const char *soname)
         abort();
     }
 
-    for( size_t i = 0; i < _capsule_metadata_list->next; i++ )
+    for( size_t i = 0; i < _capsule_list->next; i++ )
     {
-        capsule_metadata *cm = ptr_list_nth_ptr( _capsule_metadata_list, i );
+        capsule other = ptr_list_nth_ptr( _capsule_list, i );
 
-        if( !cm || cm->closed )
+        if( !other )
             continue;
 
         DEBUG( DEBUG_CAPSULE, " ");
-        DUMP_METADATA( i, cm );
-        DUMP_STRV( excluded, cm->combined_exclude );
-        DUMP_STRV( exported, cm->combined_export  );
-        DUMP_STRV( nowrap,   cm->combined_nowrap  );
+        DUMP_METADATA( i, other->meta );
+        DUMP_STRV( excluded, other->meta->combined_exclude );
+        DUMP_STRV( exported, other->meta->combined_export  );
+        DUMP_STRV( nowrap,   other->meta->combined_nowrap  );
     }
 
-    return meta->handle;
+    return cap;
 }
 
 #define CLEAR(f,x) ({ f( x ); x = NULL; })
@@ -449,26 +441,35 @@ capsule_close (capsule cap)
 {
     capsule_metadata *meta = cap->meta;
 
+    DEBUG( DEBUG_CAPSULE, "Uninitializing shim library %s", meta->soname );
+
+    // scrub all entries in the manifest pointing to this metadata
+    for( size_t n = 0; n < _capsule_list->next; n++ )
+    {
+        capsule other = ptr_list_nth_ptr( _capsule_list, n );
+
+        if( other == cap )
+        {
+            _capsule_list->loc[ n ].ptr = NULL;
+        }
+        else if( other != NULL )
+        {
+            // There should only be one capsule per capsule_metadata
+            assert( other->meta != meta );
+        }
+    }
+
     // free+null all the non-static memory in the metadata struct
     CLEAR( free_strv, meta->combined_exclude );
     CLEAR( free_strv, meta->combined_export  );
     CLEAR( free_strv, meta->combined_nowrap  );
     CLEAR( free     , meta->active_prefix    );
+    meta->handle = NULL;
 
     CLEAR( ptr_list_free, cap->seen.all  );
     CLEAR( ptr_list_free, cap->seen.some );
-    CLEAR( free         , meta->handle   );
-
-    // flag it as closed just in case something tries to peek at it
-    meta->closed = 1;
-
-    // scrub all entries in the manifest pointing to this metadata
-    for( size_t n = 0; n < _capsule_metadata_list->next; n++ )
-    {
-        capsule_metadata *cm = ptr_list_nth_ptr( _capsule_metadata_list, n );
-
-        if( cm == meta )
-            _capsule_metadata_list->loc[ n ].ptr = NULL;
-    }
 
+    // poison the capsule struct and free it
+    memset( cap, 'X', sizeof(struct _capsule) );
+    free( cap );
 }
diff --git a/capsule/capsule-private.h b/capsule/capsule-private.h
index 30e8c3645..5ef4865a3 100644
--- a/capsule/capsule-private.h
+++ b/capsule/capsule-private.h
@@ -15,6 +15,6 @@ struct _capsule
     capsule_metadata *meta;
 };
 
-extern ptr_list *_capsule_metadata_list;
+extern ptr_list *_capsule_list;
 extern dlsymfunc capsule_dl_symbol;
 extern dlopnfunc capsule_dl_open;
diff --git a/capsule/capsule-wrappers.c b/capsule/capsule-wrappers.c
index d2aab77d6..e943af803 100644
--- a/capsule/capsule-wrappers.c
+++ b/capsule/capsule-wrappers.c
@@ -51,12 +51,12 @@ capsule_external_dlsym (void *handle, const char *symbol)
     DEBUG( DEBUG_DLFUNC|DEBUG_WRAPPERS, "dlsym(%s)", symbol );
     void *addr = NULL;
 
-    for( size_t n = 0; n < _capsule_metadata_list->next; n++ )
+    for( size_t n = 0; n < _capsule_list->next; n++ )
     {
-        capsule_metadata *m = ptr_list_nth_ptr( _capsule_metadata_list, n );
-        // TODO: If handle != m->handle->dl_handle, should we skip it?
+        capsule cap = ptr_list_nth_ptr( _capsule_list, n );
+        // TODO: If handle != cap->dl_handle, should we skip it?
         // TODO: RTLD_NEXT isn't implemented (is it implementable?)
-        addr = capsule_dl_symbol ( m->handle->dl_handle, symbol );
+        addr = capsule_dl_symbol ( cap->dl_handle, symbol );
 
         if( addr )
         {
@@ -66,7 +66,7 @@ capsule_external_dlsym (void *handle, const char *symbol)
             // or if we are unable to determine where it came from (what?)
             if( dladdr( addr, &dso ) )
             {
-                if( !dso_is_exported( dso.dli_fname, m->combined_export ) )
+                if( !dso_is_exported( dso.dli_fname, cap->meta->combined_export ) )
                     addr = NULL;
 
                 DEBUG( DEBUG_DLFUNC|DEBUG_WRAPPERS,
@@ -127,28 +127,27 @@ capsule_external_dlopen(const char *file, int flag)
             debug_flags |= DEBUG_RELOCS;
         // This may not even be necessary, so it should not be fatal.
         // We do want to log it though as it might be an important clue:
-        for( size_t n = 0; n < _capsule_metadata_list->next; n++ )
+        for( size_t n = 0; n < _capsule_list->next; n++ )
         {
-            capsule_metadata *m = ptr_list_nth_ptr( _capsule_metadata_list, n );
-            const capsule c = m->handle;
+            const capsule c = ptr_list_nth_ptr( _capsule_list, n );
 
             if( capsule_relocate( c, NULL, &error ) != 0 )
             {
                 fprintf( stderr,
                          "relocation from %s after dlopen(%s, …) failed: %s\n",
-                         m->soname, file, error );
+                         c->meta->soname, file, error );
                 free( error );
             }
 
             if( capsule_relocate_except( c,
-                                         m->dl_wrappers,
-                                         (const char **)m->combined_nowrap,
+                                         c->meta->dl_wrappers,
+                                         (const char **)c->meta->combined_nowrap,
                                          &error ) != 0 )
             {
                 fprintf( stderr,
                          "dl-wrapper relocation from %s after "
                          "dlopen(%s, …) failed: %s\n",
-                         m->soname, file, error );
+                         c->meta->soname, file, error );
                 free( error );
             }
         }
-- 
GitLab