[cogl] Don't use the 'NULL' GModule to resolve GL symbols

From: Neil Roberts <nroberts src gnome org>
To: commits-list gnome org
Cc:
Subject: [cogl] Don't use the 'NULL' GModule to resolve GL symbols
Date: Wed, 27 Jul 2011 15:23:15 +0000 (UTC)
commit d259a87602516f6882138e6bc031d0647cb15140
Author: Neil Roberts <neil linux intel com>
Date:   Wed Jul 27 12:30:02 2011 +0100

    Don't use the 'NULL' GModule to resolve GL symbols
    
    Previously, _cogl_get_proc_address had a fallback to resolve the
    symbol using g_module_open(NULL) to get the symbol from anywhere in
    the address space. The EGL backend ends up using this on some drivers
    because eglGetProcAddress isn't meant to return a pointer for core
    functions. This causes problems if something in the process is linking
    against a different GL library, for example Cairo may be linking
    against libGL itself. In this case it may end up resolving symbols
    from the GL library even if GLES is being used.
    
    This patch removes the fallback. The EGL version now has its own
    fallback instead which passes the existing libgl_module from the
    renderer to g_module_symbol so that it should only get symbols from
    that library or its dependency chain. The GLX and WGL winsys only call
    glXGetProcAddress and wglGetProcAddress. The stub winsys does however
    continue using the global symbol lookup.
    
    The internal _cogl_get_proc_address function has been renamed to
    _cogl_renderer_get_proc_address because it needs a connected renderer
    to work so it could be considered to be a renderer method. The pointer
    to the renderer is passed down to the winsys backends so that it can
    use the data attached to the renderer to get the module pointers.
    
    https://bugzilla.gnome.org/show_bug.cgi?id=655412
    
    Reviewed-by: Robert Bragg <robert linux intel com>

 cogl/cogl-feature-private.c       |    7 +++--
 cogl/cogl-feature-private.h       |    2 +-
 cogl/cogl-glx-renderer-private.h  |    3 ++
 cogl/cogl-renderer-private.h      |    4 +++
 cogl/cogl-renderer.c              |    9 ++++++
 cogl/cogl.c                       |    7 +---
 cogl/driver/gl/cogl-gl.c          |    5 ++-
 cogl/driver/gles/cogl-gles.c      |    5 ++-
 cogl/winsys/cogl-winsys-egl.c     |   19 ++++++++++---
 cogl/winsys/cogl-winsys-glx.c     |   52 ++++++++-----------------------------
 cogl/winsys/cogl-winsys-private.h |    7 +---
 cogl/winsys/cogl-winsys-sdl.c     |    3 +-
 cogl/winsys/cogl-winsys-stub.c    |   20 +++++++++++++-
 cogl/winsys/cogl-winsys-wgl.c     |    7 +++--
 cogl/winsys/cogl-winsys.c         |   29 --------------------
 15 files changed, 80 insertions(+), 99 deletions(-)
---
diff --git a/cogl/cogl-feature-private.c b/cogl/cogl-feature-private.c
index 053bd02..44539da 100644
--- a/cogl/cogl-feature-private.c
+++ b/cogl/cogl-feature-private.c
@@ -31,9 +31,10 @@
 #include "cogl-context-private.h"
 
 #include "cogl-feature-private.h"
+#include "cogl-renderer-private.h"
 
 gboolean
-_cogl_feature_check (const CoglWinsysVtable *winsys,
+_cogl_feature_check (CoglRenderer *renderer,
                      const char *driver_prefix,
                      const CoglFeatureData *data,
                      int gl_major,
@@ -122,7 +123,7 @@ _cogl_feature_check (const CoglWinsysVtable *winsys,
 
       full_function_name = g_strconcat (data->functions[func_num].name,
                                         suffix, NULL);
-      func = _cogl_get_proc_address (winsys, full_function_name);
+      func = _cogl_renderer_get_proc_address (renderer, full_function_name);
       g_free (full_function_name);
 
       if (func == NULL)
@@ -189,7 +190,7 @@ _cogl_feature_check_ext_functions (CoglContext *context,
   int i;
 
   for (i = 0; i < G_N_ELEMENTS (cogl_feature_ext_functions_data); i++)
-    _cogl_feature_check (_cogl_context_get_winsys (context),
+    _cogl_feature_check (context->display->renderer,
                          "GL", cogl_feature_ext_functions_data + i,
                          gl_major, gl_minor, context->driver,
                          gl_extensions,
diff --git a/cogl/cogl-feature-private.h b/cogl/cogl-feature-private.h
index 1cf3eed..c2cec7c 100644
--- a/cogl/cogl-feature-private.h
+++ b/cogl/cogl-feature-private.h
@@ -81,7 +81,7 @@ struct _CoglFeatureData
 };
 
 gboolean
-_cogl_feature_check (const CoglWinsysVtable *winsys,
+_cogl_feature_check (CoglRenderer *renderer,
                      const char *driver_prefix,
                      const CoglFeatureData *data,
                      int gl_major,
diff --git a/cogl/cogl-glx-renderer-private.h b/cogl/cogl-glx-renderer-private.h
index 1016235..2d36a5f 100644
--- a/cogl/cogl-glx-renderer-private.h
+++ b/cogl/cogl-glx-renderer-private.h
@@ -94,6 +94,9 @@ typedef struct _CoglGLXRenderer
   XVisualInfo *
   (* glXGetVisualFromFBConfig) (Display *dpy, GLXFBConfig config);
 
+  void *
+  (* glXGetProcAddress) (const GLubyte *procName);
+
   /* Function pointers for GLX specific extensions */
 #define COGL_WINSYS_FEATURE_BEGIN(a, b, c, d, e, f)
 
diff --git a/cogl/cogl-renderer-private.h b/cogl/cogl-renderer-private.h
index dc279ba..f85a843 100644
--- a/cogl/cogl-renderer-private.h
+++ b/cogl/cogl-renderer-private.h
@@ -79,4 +79,8 @@ _cogl_renderer_remove_native_filter (CoglRenderer *renderer,
                                      CoglNativeFilterFunc func,
                                      void *data);
 
+void *
+_cogl_renderer_get_proc_address (CoglRenderer *renderer,
+                                 const char *name);
+
 #endif /* __COGL_RENDERER_PRIVATE_H */
diff --git a/cogl/cogl-renderer.c b/cogl/cogl-renderer.c
index be73adf..5a4f570 100644
--- a/cogl/cogl-renderer.c
+++ b/cogl/cogl-renderer.c
@@ -379,3 +379,12 @@ cogl_renderer_get_winsys_id (CoglRenderer *renderer)
 
   return renderer->winsys_vtable->id;
 }
+
+void *
+_cogl_renderer_get_proc_address (CoglRenderer *renderer,
+                                 const char *name)
+{
+  const CoglWinsysVtable *winsys = _cogl_renderer_get_winsys (renderer);
+
+  return winsys->renderer_get_proc_address (renderer, name);
+}
diff --git a/cogl/cogl.c b/cogl/cogl.c
index 15b9148..e3eb3c3 100644
--- a/cogl/cogl.c
+++ b/cogl/cogl.c
@@ -46,6 +46,7 @@
 #include "cogl-texture-driver.h"
 #include "cogl-attribute-private.h"
 #include "cogl-framebuffer-private.h"
+#include "cogl-renderer-private.h"
 
 #ifndef GL_PACK_INVERT_MESA
 #define GL_PACK_INVERT_MESA 0x8758
@@ -92,13 +93,9 @@ cogl_gl_error_to_string (GLenum error_code)
 CoglFuncPtr
 cogl_get_proc_address (const char* name)
 {
-  const CoglWinsysVtable *winsys;
-
   _COGL_GET_CONTEXT (ctx, NULL);
 
-  winsys = _cogl_context_get_winsys (ctx);
-
-  return _cogl_get_proc_address (winsys, name);
+  return _cogl_renderer_get_proc_address (ctx->display->renderer, name);
 }
 
 gboolean
diff --git a/cogl/driver/gl/cogl-gl.c b/cogl/driver/gl/cogl-gl.c
index 3ed4b04..589d3da 100644
--- a/cogl/driver/gl/cogl-gl.c
+++ b/cogl/driver/gl/cogl-gl.c
@@ -33,6 +33,7 @@
 #include "cogl-internal.h"
 #include "cogl-context-private.h"
 #include "cogl-feature-private.h"
+#include "cogl-renderer-private.h"
 
 static gboolean
 _cogl_get_gl_version (int *major_out, int *minor_out)
@@ -137,8 +138,8 @@ _cogl_gl_update_features (CoglContext *context,
      function because we need to use it to determine what functions we
      can expect */
   context->glGetString =
-    (void *) _cogl_get_proc_address (_cogl_context_get_winsys (context),
-                                     "glGetString");
+    (void *) _cogl_renderer_get_proc_address (context->display->renderer,
+                                              "glGetString");
 
   if (!check_gl_version (context, error))
     return FALSE;
diff --git a/cogl/driver/gles/cogl-gles.c b/cogl/driver/gles/cogl-gles.c
index 8418a7a..1dbe433 100644
--- a/cogl/driver/gles/cogl-gles.c
+++ b/cogl/driver/gles/cogl-gles.c
@@ -31,6 +31,7 @@
 #include "cogl-internal.h"
 #include "cogl-context-private.h"
 #include "cogl-feature-private.h"
+#include "cogl-renderer-private.h"
 
 gboolean
 _cogl_gles_update_features (CoglContext *context,
@@ -45,8 +46,8 @@ _cogl_gles_update_features (CoglContext *context,
      function because we need to use it to determine what functions we
      can expect */
   context->glGetString =
-    (void *) _cogl_get_proc_address (_cogl_context_get_winsys (context),
-                                     "glGetString");
+    (void *) _cogl_renderer_get_proc_address (context->display->renderer,
+                                              "glGetString");
 
   COGL_NOTE (WINSYS,
              "Checking features\n"
diff --git a/cogl/winsys/cogl-winsys-egl.c b/cogl/winsys/cogl-winsys-egl.c
index c076333..14de1e3 100644
--- a/cogl/winsys/cogl-winsys-egl.c
+++ b/cogl/winsys/cogl-winsys-egl.c
@@ -212,9 +212,19 @@ static const CoglFeatureData winsys_feature_data[] =
   };
 
 static CoglFuncPtr
-_cogl_winsys_get_proc_address (const char *name)
+_cogl_winsys_renderer_get_proc_address (CoglRenderer *renderer,
+                                        const char *name)
 {
-  return (CoglFuncPtr) eglGetProcAddress (name);
+  void *ptr;
+
+  ptr = eglGetProcAddress (name);
+
+  /* eglGetProcAddress doesn't support fetching core API so we need to
+     get that separately with GModule */
+  if (ptr == NULL)
+    g_module_symbol (renderer->libgl_module, name, &ptr);
+
+  return ptr;
 }
 
 #ifdef COGL_HAS_EGL_PLATFORM_ANDROID_SUPPORT
@@ -342,7 +352,6 @@ static void
 check_egl_extensions (CoglRenderer *renderer)
 {
   CoglRendererEGL *egl_renderer = renderer->winsys;
-  const CoglWinsysVtable *winsys = renderer->winsys_vtable;
   const char *egl_extensions;
   int i;
 
@@ -352,7 +361,7 @@ check_egl_extensions (CoglRenderer *renderer)
 
   egl_renderer->private_features = 0;
   for (i = 0; i < G_N_ELEMENTS (winsys_feature_data); i++)
-    if (_cogl_feature_check (winsys,
+    if (_cogl_feature_check (renderer,
                              "EGL", winsys_feature_data + i, 0, 0,
                              COGL_DRIVER_GL, /* the driver isn't used */
                              egl_extensions,
@@ -1657,7 +1666,7 @@ static CoglWinsysVtable _cogl_winsys_vtable =
   {
     .id = COGL_WINSYS_ID_EGL,
     .name = "EGL",
-    .get_proc_address = _cogl_winsys_get_proc_address,
+    .renderer_get_proc_address = _cogl_winsys_renderer_get_proc_address,
     .renderer_connect = _cogl_winsys_renderer_connect,
     .renderer_disconnect = _cogl_winsys_renderer_disconnect,
     .display_setup = _cogl_winsys_display_setup,
diff --git a/cogl/winsys/cogl-winsys-glx.c b/cogl/winsys/cogl-winsys-glx.c
index 12d7a54..a092bc7 100644
--- a/cogl/winsys/cogl-winsys-glx.c
+++ b/cogl/winsys/cogl-winsys-glx.c
@@ -56,8 +56,6 @@
 #include <GL/glx.h>
 #include <X11/Xlib.h>
 
-typedef CoglFuncPtr (*GLXGetProcAddressProc) (const GLubyte *procName);
-
 #ifdef HAVE_DRM
 #include <drm.h>
 #include <sys/ioctl.h>
@@ -138,44 +136,12 @@ static const CoglFeatureData winsys_feature_data[] =
   };
 
 static CoglFuncPtr
-_cogl_winsys_get_proc_address (const char *name)
+_cogl_winsys_renderer_get_proc_address (CoglRenderer *renderer,
+                                        const char *name)
 {
-  static GLXGetProcAddressProc get_proc_func = NULL;
-  static void *dlhand = NULL;
-
-  if (get_proc_func == NULL && dlhand == NULL)
-    {
-      dlhand = dlopen (NULL, RTLD_LAZY);
-
-      if (!dlhand)
-        {
-          g_warning ("Failed to dlopen (NULL, RTDL_LAZY): %s", dlerror ());
-          return NULL;
-        }
-
-      dlerror ();
-
-      get_proc_func =
-        (GLXGetProcAddressProc) dlsym (dlhand, "glXGetProcAddress");
-
-      if (dlerror () != NULL)
-        {
-          get_proc_func =
-            (GLXGetProcAddressProc) dlsym (dlhand, "glXGetProcAddressARB");
-        }
-
-      if (dlerror () != NULL)
-        {
-          get_proc_func = NULL;
-          g_warning ("failed to bind GLXGetProcAddress "
-                     "or GLXGetProcAddressARB");
-        }
-    }
-
-  if (get_proc_func)
-    return get_proc_func ((GLubyte *) name);
+  CoglGLXRenderer *glx_renderer = renderer->winsys;
 
-  return NULL;
+  return glx_renderer->glXGetProcAddress ((const GLubyte *) name);
 }
 
 static CoglOnscreen *
@@ -316,7 +282,11 @@ resolve_core_glx_functions (CoglRenderer *renderer,
       !g_module_symbol (glx_renderer->libgl_module, "glXDestroyWindow",
                         (void **) &glx_renderer->glXDestroyWindow) ||
       !g_module_symbol (glx_renderer->libgl_module, "glXQueryExtensionsString",
-                        (void **) &glx_renderer->glXQueryExtensionsString))
+                        (void **) &glx_renderer->glXQueryExtensionsString) ||
+      (!g_module_symbol (glx_renderer->libgl_module, "glXGetProcAddress",
+                         (void **) &glx_renderer->glXGetProcAddress) &&
+       !g_module_symbol (glx_renderer->libgl_module, "glXGetProcAddressARB",
+                         (void **) &glx_renderer->glXGetProcAddress)))
     {
       g_set_error (error, COGL_WINSYS_ERROR,
                    COGL_WINSYS_ERROR_INIT,
@@ -419,7 +389,7 @@ update_winsys_features (CoglContext *context, GError **error)
                   TRUE);
 
   for (i = 0; i < G_N_ELEMENTS (winsys_feature_data); i++)
-    if (_cogl_feature_check (_cogl_context_get_winsys (context),
+    if (_cogl_feature_check (context->display->renderer,
                              "GLX", winsys_feature_data + i, 0, 0,
                              COGL_DRIVER_GL, /* the driver isn't used */
                              glx_extensions,
@@ -1992,7 +1962,7 @@ static CoglWinsysVtable _cogl_winsys_vtable =
   {
     .id = COGL_WINSYS_ID_GLX,
     .name = "GLX",
-    .get_proc_address = _cogl_winsys_get_proc_address,
+    .renderer_get_proc_address = _cogl_winsys_renderer_get_proc_address,
     .renderer_connect = _cogl_winsys_renderer_connect,
     .renderer_disconnect = _cogl_winsys_renderer_disconnect,
     .display_setup = _cogl_winsys_display_setup,
diff --git a/cogl/winsys/cogl-winsys-private.h b/cogl/winsys/cogl-winsys-private.h
index 5822479..88df22d 100644
--- a/cogl/winsys/cogl-winsys-private.h
+++ b/cogl/winsys/cogl-winsys-private.h
@@ -63,7 +63,8 @@ typedef struct _CoglWinsysVtable
   /* Required functions */
 
   CoglFuncPtr
-  (*get_proc_address) (const char *name);
+  (*renderer_get_proc_address) (CoglRenderer *renderer,
+                                const char *name);
 
   gboolean
   (*renderer_connect) (CoglRenderer *renderer, GError **error);
@@ -158,8 +159,4 @@ typedef struct _CoglWinsysVtable
 gboolean
 _cogl_winsys_has_feature (CoglWinsysFeature feature);
 
-CoglFuncPtr
-_cogl_get_proc_address (const CoglWinsysVtable *winsys,
-                        const char *name);
-
 #endif /* __COGL_WINSYS_PRIVATE_H */
diff --git a/cogl/winsys/cogl-winsys-sdl.c b/cogl/winsys/cogl-winsys-sdl.c
index 9c937e4..bb8908b 100644
--- a/cogl/winsys/cogl-winsys-sdl.c
+++ b/cogl/winsys/cogl-winsys-sdl.c
@@ -28,7 +28,8 @@
 #include "cogl.h"
 
 CoglFuncPtr
-_cogl_winsys_get_proc_address (const char *name)
+_cogl_winsys_get_proc_address (CoglRenderer *renderer,
+                               const char *name)
 {
   return NULL;
 }
diff --git a/cogl/winsys/cogl-winsys-stub.c b/cogl/winsys/cogl-winsys-stub.c
index 565814a..81d8034 100644
--- a/cogl/winsys/cogl-winsys-stub.c
+++ b/cogl/winsys/cogl-winsys-stub.c
@@ -45,8 +45,24 @@ static int _cogl_winsys_stub_dummy_ptr;
  */
 
 static CoglFuncPtr
-_cogl_winsys_get_proc_address (const char *name)
+_cogl_winsys_renderer_get_proc_address (CoglRenderer *renderer,
+                                        const char *name)
 {
+  static GModule *module = NULL;
+
+  /* this should find the right function if the program is linked against a
+   * library providing it */
+  if (G_UNLIKELY (module == NULL))
+    module = g_module_open (NULL, 0);
+
+  if (module)
+    {
+      void *symbol;
+
+      if (g_module_symbol (module, name, &symbol))
+        return symbol;
+    }
+
   return NULL;
 }
 
@@ -134,7 +150,7 @@ static CoglWinsysVtable _cogl_winsys_vtable =
   {
     .id = COGL_WINSYS_ID_STUB,
     .name = "STUB",
-    .get_proc_address = _cogl_winsys_get_proc_address,
+    .renderer_get_proc_address = _cogl_winsys_renderer_get_proc_address,
     .renderer_connect = _cogl_winsys_renderer_connect,
     .renderer_disconnect = _cogl_winsys_renderer_disconnect,
     .display_setup = _cogl_winsys_display_setup,
diff --git a/cogl/winsys/cogl-winsys-wgl.c b/cogl/winsys/cogl-winsys-wgl.c
index d07391d..fac0161 100644
--- a/cogl/winsys/cogl-winsys-wgl.c
+++ b/cogl/winsys/cogl-winsys-wgl.c
@@ -122,7 +122,8 @@ static const CoglFeatureData winsys_feature_data[] =
   };
 
 static CoglFuncPtr
-_cogl_winsys_get_proc_address (const char *name)
+_cogl_winsys_renderer_get_proc_address (CoglRenderer *renderer,
+                                        const char *name)
 {
   return (CoglFuncPtr) wglGetProcAddress ((LPCSTR) name);
 }
@@ -569,7 +570,7 @@ update_winsys_features (CoglContext *context, GError **error)
       COGL_NOTE (WINSYS, "  WGL Extensions: %s", wgl_extensions);
 
       for (i = 0; i < G_N_ELEMENTS (winsys_feature_data); i++)
-        if (_cogl_feature_check (_cogl_context_get_winsys (context),
+        if (_cogl_feature_check (context->display->renderer,
                                  "WGL", winsys_feature_data + i, 0, 0,
                                  COGL_DRIVER_GL,
                                  wgl_extensions,
@@ -833,7 +834,7 @@ _cogl_winsys_wgl_get_vtable (void)
 
       vtable.id = COGL_WINSYS_ID_EGL;
       vtable.name = "WGL";
-      vtable.get_proc_address = _cogl_winsys_get_proc_address;
+      vtable.renderer_get_proc_address = _cogl_winsys_renderer_get_proc_address;
       vtable.renderer_connect = _cogl_winsys_renderer_connect;
       vtable.renderer_disconnect = _cogl_winsys_renderer_disconnect;
       vtable.display_setup = _cogl_winsys_display_setup;
diff --git a/cogl/winsys/cogl-winsys.c b/cogl/winsys/cogl-winsys.c
index e75b906..b5669db 100644
--- a/cogl/winsys/cogl-winsys.c
+++ b/cogl/winsys/cogl-winsys.c
@@ -45,32 +45,3 @@ _cogl_winsys_has_feature (CoglWinsysFeature feature)
 
   return COGL_FLAGS_GET (ctx->winsys_features, feature);
 }
-
-/* XXX: we would call this _cogl_winsys_get_proc_address but that
- * currently collides with the per winsys implementation names we use.
- * */
-CoglFuncPtr
-_cogl_get_proc_address (const CoglWinsysVtable *winsys,
-                        const char *name)
-{
-  void *address = winsys->get_proc_address (name);
-  static GModule *module = NULL;
-
-  if (address)
-    return address;
-
-  /* this should find the right function if the program is linked against a
-   * library providing it */
-  if (G_UNLIKELY (module == NULL))
-    module = g_module_open (NULL, 0);
-
-  if (module)
-    {
-      gpointer symbol;
-
-      if (g_module_symbol (module, name, &symbol))
-        return symbol;
-    }
-
-  return NULL;
-}
[Date Prev][Date Next] [Thread Prev][Thread Next] [Thread Index] [Date Index] [Author Index]