[PATCH:libXxf86vm 1/4] Improve error handling in XF86VidModeGetMonitor()

[walter harms]

Am 14.04.2013 03:48, schrieb Alan Coopersmith:
> Ensure that when we return an error we unlock the display first, and
> NULL out any pointers we freed in error cleanup.
> 
> Instead of adding these fixes to every error check, instead combine
> the error handling cleanup into a single copy.
> 
> Signed-off-by: Alan Coopersmith <alan.coopersmith at oracle.com>
> ---
>  src/XF86VMode.c |   82 ++++++++++++++++++++++++++-----------------------------
>  1 file changed, 39 insertions(+), 43 deletions(-)
> 
> diff --git a/src/XF86VMode.c b/src/XF86VMode.c
> index c0e50e6..165f8ba 100644
> --- a/src/XF86VMode.c
> +++ b/src/XF86VMode.c
> @@ -856,6 +856,7 @@ XF86VidModeGetMonitor(Display* dpy, int screen, XF86VidModeMonitor* monitor)
>      xXF86VidModeGetMonitorReq *req;
>      CARD32 syncrange;
>      int i;
> +    Bool result = True;
>  
>      XF86VidModeCheckExtension (dpy, info, False);
>  
> @@ -875,63 +876,58 @@ XF86VidModeGetMonitor(Display* dpy, int screen, XF86VidModeMonitor* monitor)
>      monitor->bandwidth = (float)rep.bandwidth / 1e6;
>  #endif
>      if (rep.vendorLength) {
> -	if (!(monitor->vendor = (char *)Xcalloc(rep.vendorLength + 1, 1))) {
> -	    _XEatData(dpy, (rep.nhsync + rep.nvsync) * 4 +
> -		      ((rep.vendorLength+3) & ~3) + ((rep.modelLength+3) & ~3));
> -	    return False;
> -	}
> +	monitor->vendor = Xcalloc(rep.vendorLength + 1, 1);
> +	if (monitor->vendor == NULL)
> +	    result = False;
>      } else {
>  	monitor->vendor = NULL;
>      }

i do not understand this, do you realy want monitor->vendor = NULL;
when monitor->vendor = Xcalloc(rep.vendorLength + 1, 1); succeeds ?`


> -    if (rep.modelLength) {
> -	if (!(monitor->model = Xcalloc(rep.modelLength + 1, 1))) {
> -	    _XEatData(dpy, (rep.nhsync + rep.nvsync) * 4 +
> -		      ((rep.vendorLength+3) & ~3) + ((rep.modelLength+3) & ~3));
> -	    if (monitor->vendor)
> -		Xfree(monitor->vendor);
> -	    return False;
> -	}
> +    if (result && rep.modelLength) {
> +	monitor->model = Xcalloc(rep.modelLength + 1, 1);
> +	if (monitor->model == NULL)
> +	    result = False;
>      } else {
>  	monitor->model = NULL;
>      }
> -    if (!(monitor->hsync = Xcalloc(rep.nhsync, sizeof(XF86VidModeSyncRange)))) {
> -	_XEatData(dpy, (rep.nhsync + rep.nvsync) * 4 +
> -		  ((rep.vendorLength+3) & ~3) + ((rep.modelLength+3) & ~3));
> -
> -	if (monitor->vendor)
> -	    Xfree(monitor->vendor);
> -	if (monitor->model)
> -	    Xfree(monitor->model);
> -	return False;
> +    if (result) {
> +	monitor->hsync = Xcalloc(rep.nhsync, sizeof(XF86VidModeSyncRange));
> +	monitor->vsync = Xcalloc(rep.nvsync, sizeof(XF86VidModeSyncRange));
> +	if ((monitor->hsync == NULL) || (monitor->vsync == NULL))
> +	    result = False;
> +    } else {
> +	monitor->hsync = monitor->vsync = NULL;
>      }
> -    if (!(monitor->vsync = Xcalloc(rep.nvsync, sizeof(XF86VidModeSyncRange)))) {
> +    if (result == False) {
>  	_XEatData(dpy, (rep.nhsync + rep.nvsync) * 4 +
>  		  ((rep.vendorLength+3) & ~3) + ((rep.modelLength+3) & ~3));
> -	if (monitor->vendor)
> -	    Xfree(monitor->vendor);
> -	if (monitor->model)
> -	    Xfree(monitor->model);
> +	Xfree(monitor->vendor);
> +	monitor->vendor = NULL;
> +	Xfree(monitor->model);
> +	monitor->model = NULL;
>  	Xfree(monitor->hsync);
> -	return False;
> -    }
> -    for (i = 0; i < rep.nhsync; i++) {
> -	_XRead(dpy, (char *)&syncrange, 4);
> -	monitor->hsync[i].lo = (float)(syncrange & 0xFFFF) / 100.0;
> -	monitor->hsync[i].hi = (float)(syncrange >> 16) / 100.0;
> +	monitor->hsync = NULL;
> +	Xfree(monitor->vsync);
> +	monitor->vsync = NULL;
>      }
> -    for (i = 0; i < rep.nvsync; i++) {
> -	_XRead(dpy, (char *)&syncrange, 4);
> -	monitor->vsync[i].lo = (float)(syncrange & 0xFFFF) / 100.0;
> -	monitor->vsync[i].hi = (float)(syncrange >> 16) / 100.0;
> +    else {
> +	for (i = 0; i < rep.nhsync; i++) {
> +	    _XRead(dpy, (char *)&syncrange, 4);
> +	    monitor->hsync[i].lo = (float)(syncrange & 0xFFFF) / 100.0;
> +	    monitor->hsync[i].hi = (float)(syncrange >> 16) / 100.0;
> +	}
> +	for (i = 0; i < rep.nvsync; i++) {
> +	    _XRead(dpy, (char *)&syncrange, 4);
> +	    monitor->vsync[i].lo = (float)(syncrange & 0xFFFF) / 100.0;
> +	    monitor->vsync[i].hi = (float)(syncrange >> 16) / 100.0;
> +	}
> +	if (rep.vendorLength)
> +	    _XReadPad(dpy, monitor->vendor, rep.vendorLength);
> +	if (rep.modelLength)
> +	    _XReadPad(dpy, monitor->model, rep.modelLength);
>      }
> -    if (rep.vendorLength)
> -	_XReadPad(dpy, monitor->vendor, rep.vendorLength);
> -    if (rep.modelLength)
> -	_XReadPad(dpy, monitor->model, rep.modelLength);
> -
>      UnlockDisplay(dpy);
>      SyncHandle();
> -    return True;
> +    return result;
>  }
>  
>  Bool