[Mesa-dev] [PATCH 1/4] common: Check for extensions before resolving symbols

[Eric Engestrom]

On Tuesday, 2017-05-02 11:52:06 +0100, Daniel Stone wrote:
> eglGetProcAddress is allowed to return any old garbage for symbols it
> doesn't know about. To avoid any mishaps, check for the appropriate
> extension presence (split into EGL client extension, EGL display
> extension, and GL extension, checks) before we look up any symbols
> through it.
> 
> The walk through the extension list is taken from libepoxy.
> 
> Signed-off-by: Daniel Stone <daniels at collabora.com>

I had essentially the same change locally (but only for egl, not gl) :)

A couple nit picks below, but this patch is:
Reviewed-by: Eric Engestrom <eric.engestrom at imgtec.com>

> ---
>  common.c | 63 +++++++++++++++++++++++++++++++++++++++++++++++++++------------
>  1 file changed, 51 insertions(+), 12 deletions(-)
> 
> diff --git a/common.c b/common.c
> index 610ff87..bf2f78e 100644
> --- a/common.c
> +++ b/common.c
> @@ -1,5 +1,6 @@
>  /*
>   * Copyright (c) 2017 Rob Clark <rclark at redhat.com>
> + * Copyright © 2013 Intel Corporation

2013? Is that for the has_ext() code from libepoxy?

>   *
>   * Permission is hereby granted, free of charge, to any person obtaining a
>   * copy of this software and associated documentation files (the "Software"),
> @@ -23,6 +24,7 @@
>  
>  #include <errno.h>
>  #include <fcntl.h>
> +#include <stdbool.h>
>  #include <stdio.h>
>  #include <stdlib.h>
>  #include <string.h>
> @@ -78,6 +80,26 @@ const struct gbm * init_gbm(int drm_fd, int w, int h, uint64_t modifier)
>  	return &gbm;
>  }
>  
> +static bool has_ext(const char *extension_list, const char *ext)
> +{
> +	const char *ptr = extension_list;
> +	int len = strlen(ext);
> +
> +	if (ptr == NULL || *ptr == '\0')
> +		return false;
> +
> +	while (true) {
> +		ptr = strstr(ptr, ext);
> +		if (!ptr)
> +			return false;
> +
> +		if (ptr[len] == ' ' || ptr[len] == '\0')
> +			return true;
> +
> +		ptr += len;
> +	}
> +}
> +
>  int init_egl(struct egl *egl, const struct gbm *gbm)
>  {
>  	EGLint major, minor, n;
> @@ -96,19 +118,24 @@ int init_egl(struct egl *egl, const struct gbm *gbm)
>  		EGL_RENDERABLE_TYPE, EGL_OPENGL_ES2_BIT,
>  		EGL_NONE
>  	};
> +	const char *egl_exts_client, *egl_exts_dpy, *gl_exts;
> +
> +#define get_proc_client(name, ext) do { \
> +		if (has_ext(egl_exts_client, ext)) \
> +			egl->name = (void *)eglGetProcAddress(#name); \
> +	} while (0)
> +#define get_proc_dpy(name, ext) do { \
> +		if (has_ext(egl_exts_dpy, ext)) \
> +			egl->name = (void *)eglGetProcAddress(#name); \
> +	} while (0)
>  
> -#define get_proc(name) do { \
> -		egl->name = (void *)eglGetProcAddress(#name); \
> +#define get_proc_gl(name, ext) do { \
> +		if (has_ext(gl_exts, ext)) \
> +			egl->name = (void *)eglGetProcAddress(#name); \
>  	} while (0)
>  
> -	get_proc(eglGetPlatformDisplayEXT);
> -	get_proc(eglCreateImageKHR);
> -	get_proc(eglDestroyImageKHR);
> -	get_proc(glEGLImageTargetTexture2DOES);
> -	get_proc(eglCreateSyncKHR);
> -	get_proc(eglDestroySyncKHR);
> -	get_proc(eglWaitSyncKHR);
> -	get_proc(eglDupNativeFenceFDANDROID);
> +	egl_exts_client = eglQueryString(EGL_NO_DISPLAY, EGL_EXTENSIONS);
> +	get_proc_client(eglGetPlatformDisplayEXT, "EGL_EXT_platform_base");
>  
>  	if (egl->eglGetPlatformDisplayEXT) {
>  		egl->display = egl->eglGetPlatformDisplayEXT(EGL_PLATFORM_GBM_KHR,
> @@ -122,6 +149,14 @@ int init_egl(struct egl *egl, const struct gbm *gbm)
>  		return -1;
>  	}
>  
> +	egl_exts_dpy = eglQueryString(egl->display, EGL_EXTENSIONS);
> +	get_proc_dpy(eglCreateImageKHR, "EGL_KHR_image_base");
> +	get_proc_dpy(eglDestroyImageKHR, "EGL_KHR_image_base");
> +	get_proc_dpy(eglCreateSyncKHR, "EGL_KHR_fence_sync");
> +	get_proc_dpy(eglDestroySyncKHR, "EGL_KHR_fence_sync");
> +	get_proc_dpy(eglWaitSyncKHR, "EGL_KHR_fence_sync");
> +	get_proc_dpy(eglDupNativeFenceFDANDROID, "EGL_ANDROID_native_fence_sync");

Moving the extension name as a first param makes it more readable IMO,
as does dropping the quotes (using `#` in the macro):

	get_proc_dpy(EGL_KHR_image_base, eglCreateImageKHR);
	get_proc_dpy(EGL_KHR_image_base, eglDestroyImageKHR);
	get_proc_dpy(EGL_KHR_fence_sync, eglCreateSyncKHR);
	get_proc_dpy(EGL_KHR_fence_sync, eglDestroySyncKHR);
	get_proc_dpy(EGL_KHR_fence_sync, eglWaitSyncKHR);
	get_proc_dpy(EGL_ANDROID_native_fence_sync, eglDupNativeFenceFDANDROID);

> +
>  	printf("Using display %p with EGL version %d.%d\n",
>  			egl->display, major, minor);
>  
> @@ -129,7 +164,8 @@ int init_egl(struct egl *egl, const struct gbm *gbm)
>  	printf("EGL information:\n");
>  	printf("  version: \"%s\"\n", eglQueryString(egl->display, EGL_VERSION));
>  	printf("  vendor: \"%s\"\n", eglQueryString(egl->display, EGL_VENDOR));
> -	printf("  extensions: \"%s\"\n", eglQueryString(egl->display, EGL_EXTENSIONS));
> +	printf("  client extensions: \"%s\"\n", egl_exts_client);
> +	printf("  display extensions: \"%s\"\n", egl_exts_dpy);
>  	printf("===================================\n");
>  
>  	if (!eglBindAPI(EGL_OPENGL_ES_API)) {
> @@ -159,14 +195,17 @@ int init_egl(struct egl *egl, const struct gbm *gbm)
>  	/* connect the context to the surface */
>  	eglMakeCurrent(egl->display, egl->surface, egl->surface, egl->context);
>  
> +	gl_exts = (char *) glGetString(GL_EXTENSIONS);
>  	printf("OpenGL ES 2.x information:\n");
>  	printf("  version: \"%s\"\n", glGetString(GL_VERSION));
>  	printf("  shading language version: \"%s\"\n", glGetString(GL_SHADING_LANGUAGE_VERSION));
>  	printf("  vendor: \"%s\"\n", glGetString(GL_VENDOR));
>  	printf("  renderer: \"%s\"\n", glGetString(GL_RENDERER));
> -	printf("  extensions: \"%s\"\n", glGetString(GL_EXTENSIONS));
> +	printf("  extensions: \"%s\"\n", gl_exts);
>  	printf("===================================\n");
>  
> +	get_proc_gl(glEGLImageTargetTexture2DOES, "GL_OES_EGL_image");
> +
>  	return 0;
>  }
>  
> -- 
> 2.12.2
>