[PATCH] [glx] Use goto-style error handling in __glXDRIscreenProbe()

[Ian Romanick]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

Tomas Carnecky wrote:
> This fixes a 'Result of operation is garbage or undefined' bug found
> by clang. 'framebuffer.base' was read before being initialized.

If the problem is that framebuffer.base is not initialized, I think it
would be better to just initialize it to NULL at the top.  Having
multiple goto labels opens the possibility for someone to add / modify
code and use the wrong one.

> Signed-off-by: Tomas Carnecky <tom at dbservice.com>
> ---
> 
> This patch slightly reorders the cleanup. In the original code DRICloseConnection()
> would go before dlclose(screen->driver). But since we open the dri connection before
> dlopen'ing the driver, I thought it would make sense to dlclose the driver before 
> closing the dri connection. I don't know if that can be a problem, so someone
> familiar with the code please review.

The order of these two operations shouldn't matter.

>  glx/glxdri.c |   45 +++++++++++++++++++++++----------------------
>  1 files changed, 23 insertions(+), 22 deletions(-)
> 
> diff --git a/glx/glxdri.c b/glx/glxdri.c
> index 6122653..cbbf240 100644
> --- a/glx/glxdri.c
> +++ b/glx/glxdri.c
> @@ -996,7 +996,7 @@ __glXDRIscreenProbe(ScreenPtr pScreen)
>  
>      if (!DRIOpenConnection(pScreen, &hSAREA, &BusID)) {
>  	LogMessage(X_ERROR, "AIGLX error: DRIOpenConnection failed\n");
> -	goto handle_error;
> +	goto err_free;
>      }
>  
>      fd = drmOpenOnce(NULL, BusID, &newlyopened);
> @@ -1004,12 +1004,12 @@ __glXDRIscreenProbe(ScreenPtr pScreen)
>      if (fd < 0) {
>  	LogMessage(X_ERROR, "AIGLX error: drmOpenOnce failed (%s)\n",
>  		   strerror(-fd));
> -	goto handle_error;
> +	goto err_close_dri;
>      }
>  
>      if (drmGetMagic(fd, &magic)) {
>  	LogMessage(X_ERROR, "AIGLX error: drmGetMagic failed\n");
> -	goto handle_error;
> +	goto err_close_drm;
>      }
>  
>      version = drmGetVersion(fd);
> @@ -1027,7 +1027,7 @@ __glXDRIscreenProbe(ScreenPtr pScreen)
>  
>      if (newlyopened && !DRIAuthConnection(pScreen, magic)) {
>  	LogMessage(X_ERROR, "AIGLX error: DRIAuthConnection failed\n");
> -	goto handle_error;
> +	goto err_close_drm;
>      }
>  
>      /* Get device name (like "tdfx") and the ddx version numbers.
> @@ -1039,7 +1039,7 @@ __glXDRIscreenProbe(ScreenPtr pScreen)
>  				&ddx_version.patch,
>  				&driverName)) {
>  	LogMessage(X_ERROR, "AIGLX error: DRIGetClientDriverName failed\n");
> -	goto handle_error;
> +	goto err_close_drm;
>      }
>  
>      snprintf(filename, sizeof filename, "%s/%s_dri.so",
> @@ -1049,14 +1049,14 @@ __glXDRIscreenProbe(ScreenPtr pScreen)
>      if (screen->driver == NULL) {
>  	LogMessage(X_ERROR, "AIGLX error: dlopen of %s failed (%s)\n",
>  		   filename, dlerror());
> -        goto handle_error;
> +        goto err_close_drm;
>      }
>  
>      extensions = dlsym(screen->driver, __DRI_DRIVER_EXTENSIONS);
>      if (extensions == NULL) {
>  	LogMessage(X_ERROR, "AIGLX error: %s exports no extensions (%s)\n",
>  		   driverName, dlerror());
> -	goto handle_error;
> +	goto err_dlclose;
>      }
>      
>      for (i = 0; extensions[i]; i++) {
> @@ -1075,7 +1075,7 @@ __glXDRIscreenProbe(ScreenPtr pScreen)
>  	LogMessage(X_ERROR,
>  		   "AIGLX error: %s does not export required DRI extension\n",
>  		   driverName);
> -	goto handle_error;
> +	goto err_dlclose;
>      }
>  
>      /*
> @@ -1088,7 +1088,7 @@ __glXDRIscreenProbe(ScreenPtr pScreen)
>  			  &framebuffer.size, &framebuffer.stride,
>  			  &framebuffer.dev_priv_size, &framebuffer.dev_priv)) {
>  	LogMessage(X_ERROR, "AIGLX error: XF86DRIGetDeviceInfo failed\n");
> -	goto handle_error;
> +	goto err_dlclose;
>      }
>  
>      framebuffer.width = pScreen->width;
> @@ -1100,7 +1100,7 @@ __glXDRIscreenProbe(ScreenPtr pScreen)
>      if (status != 0) {
>  	LogMessage(X_ERROR, "AIGLX error: drmMap of framebuffer failed (%s)\n",
>  		   strerror(-status));
> -	goto handle_error;
> +	goto err_dlclose;
>      }
>  
>      /* Map the SAREA region.  Further mmap regions may be setup in
> @@ -1110,7 +1110,7 @@ __glXDRIscreenProbe(ScreenPtr pScreen)
>      if (status != 0) {
>  	LogMessage(X_ERROR, "AIGLX error: drmMap of SAREA failed (%s)\n",
>  		   strerror(-status));
> -	goto handle_error;
> +	goto err_unmap_fb;
>      }
>      
>      screen->driScreen =
> @@ -1128,7 +1128,7 @@ __glXDRIscreenProbe(ScreenPtr pScreen)
>      if (screen->driScreen == NULL) {
>  	LogMessage(X_ERROR,
>  		   "AIGLX error: Calling driver entry point failed\n");
> -	goto handle_error;
> +	goto err_unmap_sarea;
>      }
>  
>      screen->base.fbconfigs = glxConvertConfigs(screen->core, driConfigs);
> @@ -1167,21 +1167,22 @@ __glXDRIscreenProbe(ScreenPtr pScreen)
>  
>      return &screen->base;
>  
> - handle_error:
> -    if (pSAREA != NULL)
> -	drmUnmap(pSAREA, SAREA_MAX);
> +err_unmap_sarea:
> +    drmUnmap(pSAREA, SAREA_MAX);
>  
> -    if (framebuffer.base != NULL)
> -	drmUnmap((drmAddress)framebuffer.base, framebuffer.size);
> +err_unmap_fb:
> +    drmUnmap((drmAddress)framebuffer.base, framebuffer.size);
>  
> -    if (fd >= 0)
> -	drmCloseOnce(fd);
> +err_dlclose:
> +    dlclose(screen->driver);
>  
> -    DRICloseConnection(pScreen);
> +err_close_drm:
> +    drmCloseOnce(fd);
>  
> -    if (screen->driver)
> -        dlclose(screen->driver);
> +err_close_dri:
> +    DRICloseConnection(pScreen);
>  
> +err_free:
>      xfree(screen);
>  
>      LogMessage(X_ERROR, "AIGLX: reverting to software rendering\n");

-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.10 (GNU/Linux)
Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org/

iEYEARECAAYFAksqfF0ACgkQX1gOwKyEAw8+/gCgh04mi2G0Y3R3ZGh+LTyP7EC+
W98AnRwTlwPxvRSTutekoCrk087qthjB
=oO8i
-----END PGP SIGNATURE-----