Bug 65210 - xf86: return NULL for xf86CompatOutput if config->compat_output is -1
Summary: xf86: return NULL for xf86CompatOutput if config->compat_output is -1
Alias: None
Product: xorg
Classification: Unclassified
Component: Server/General (show other bugs)
Version: git
Hardware: All All
: medium normal
Assignee: Keith Packard
QA Contact: Xorg Project Team
Keywords: patch
Depends on:
Blocks: 65212
  Show dependency treegraph
Reported: 2013-05-31 12:45 UTC by vdb128
Modified: 2018-06-13 18:06 UTC (History)
0 users

See Also:
i915 platform:
i915 features:

return NULL for xf86CompatOutput if config->compat_output < 0 (1.91 KB, text/plain)
2013-05-31 12:45 UTC, vdb128
no flags Details
return NULL for xf86CompatOutput if config->compat_output < 0 (1.91 KB, patch)
2013-05-31 12:55 UTC, vdb128
no flags Details | Splinter Review
Xorg 2010 log: EDID monitor connected at output 0. (152.69 KB, text/plain)
2013-05-31 12:57 UTC, vdb128
no flags Details
Xorg 2010 log: EDID monitor connected at output 2. (41.29 KB, text/plain)
2013-05-31 12:58 UTC, vdb128
no flags Details

Description vdb128 2013-05-31 12:45:21 UTC
Created attachment 80090 [details]
return NULL for xf86CompatOutput if config->compat_output < 0

Solves the xserver/hw/xfree86/modes compat_output == -1 bad dereference.

The commit 37d956e3ac9513b74078882dff489f9b0a7a5a28 into xf86Crtc.c 
xf86CrtcConfigInit() initializes 

    config->compat_output = -1;

So an unset compat_output is now invalid, a good property since the 
previous initial compat_output == 0 is not guaranteed available.  It 
usually is available because a driver first calls xf86OutputCreate()
followed by xf86InitialConfiguration().

This change exposes a bug.  During initial configuration a monitor 
detection is attempted and if an EDID block is available the driver 
calls xf86OutputSetEDID().  There a bad dereference occurs:

xf86InitialConfiguration(ScrnInfoPtr scrn, Bool canGrow)
-> xf86ProbeOutputModes(scrn, width, height);
 --> output_modes = (*output->funcs->get_modes) (output);
  ---> xf86OutputSetEDID(xf86OutputPtr output, xf86MonPtr edid_mon)
   ----> if (output == xf86CompatOutput(scrn))       XX now bad dereference
    -----> return config->output[config->compat_output];   XX output[-1]
   ---->     xf86SetDDCproperties(scrn, edid_mon);   XX for screen monitor
-> xf86SetScrnInfoModes(scrn);
 --> output = SetCompatOutput(config);               XX sets compat_output
  ---> config->compat_output = compat;

Hence previously the screen monitor properties were always copied from 
output 0 since compat_output is only set at the end of above function.  
Two old Xorg.log files illustrate the previous behaviour.  

The suggestion

> If there is no compat output, config->compat_output is -1 and xf86CompatOutput
> reads before the beginning of the outputs array.

> Signed-off-by: Aaron Plattner <aplattner@nvidia.com>

> diff --git a/hw/xfree86/modes/xf86Crtc.h b/hw/xfree86/modes/xf86Crtc.h
> index 802303f..1ac8485 100644
> --- a/hw/xfree86/modes/xf86Crtc.h
> +++ b/hw/xfree86/modes/xf86Crtc.h
> @@ -731,6 +731,8 @@ xf86CompatOutput(ScrnInfoPtr pScrn)
>  {
>      xf86CrtcConfigPtr config = XF86_CRTC_CONFIG_PTR(pScrn);
> +    if (config->compat_output < 0)
> +        return NULL;
>      return config->output[config->compat_output];
>  }

Tested-by: Servaas Vandenberghe avoids the bad dereference.

Comment 1 vdb128 2013-05-31 12:55:33 UTC
Created attachment 80091 [details] [review]
return NULL for xf86CompatOutput if config->compat_output < 0
Comment 2 vdb128 2013-05-31 12:57:56 UTC
Created attachment 80093 [details]
Xorg 2010 log: EDID monitor connected at output 0.

During initial configuration compat_output == 0 so 
xf86SetDDCproperties() is called.  Search for 'Printing DDC '.
Comment 3 vdb128 2013-05-31 12:58:40 UTC
Created attachment 80094 [details]
Xorg 2010 log: EDID monitor connected at output 2.

Since compat_output == 0 during initial configuration 
xf86SetDDCproperties() is never called.  Search for 'EDID'.
Comment 4 Alan Coopersmith 2013-05-31 14:19:16 UTC
Just FYI, patches mailed to xorg-devel, as described on 
get applied to the X server sources with an average of several years
less delay than those stuck in bugzilla.   (Sorry, we're overloaded
and don't have developers to spare going through bugzilla looking for
Comment 5 Adam Jackson 2018-06-13 18:06:26 UTC
commit 28159eff6badf6181b255f26d1f444abe81c05b7
Author: Jason Gerecke <killertofu@gmail.com>
Date:   Thu Apr 30 18:06:14 2015 -0700

    xfree86: Return NULL from xf86CompatOutput if no compat_output is defined
    If no compat_output is defined, we inadvertently (attempt to) return
    whatever data is at index -1. Instead, return NULL since that's what
    callers are expecting.
    Reviewed-by: Adam Jackson <ajax@redhat.com>
    Signed-off-by: Jason Gerecke <jason.gerecke@wacom.com>

Use of freedesktop.org services, including Bugzilla, is subject to our Code of Conduct. How we collect and use information is described in our Privacy Policy.