[PATCH libX11 2/3] XKB: Avoid a possible NULL dereference

[Peter Hutterer]

On Wed, May 04, 2011 at 12:41:57PM -0700, Jeremy Huddleston wrote:
> This is the one that I especially wanted to review on the list because it
> doesn't sit well with me.
> 
> Hitting the default switch-case in the first run through the loop results
> in a NULL dereference (rbounds) when calling _XkbCheckBounds.  In
> subsequent runs, we get lucky in that rbounds was set in an earlier
> iteration through the loop.  I'm not really familiar enough with XKB to
> know what is going on here.
> 
> Should we be doing this:
> rbounds= &tbounds;

I think this assigment is the better choice. AIUI, theoretically if we skip
the check we may have a default doodad that's outside the section
boundaries. Not that it matters much (it's geometry, it doesn't really
matter anyway), but by using the above you at least clip it down to the
allowed area.

given that we haven't seen any actual issues yet, it suggests that this code
is never hit by our current layouts.

Cheers,
  Peter
 
> or should we just be skipping the check (which is what I accidentally pushed)?
> 
> http://people.freedesktop.org/~jeremyhu/analyzer/yuffie/20110504-0000/libX11/report-F0RlNS.html#EndPath
> 
> On May 4, 2011, at 11:50, Jeremy Huddleston wrote:
> 
> > 
> > XKBGeom.c:191:25: warning: Access to field 'x1' results in a dereference of a null pointer (loaded from variable 'rbounds')
> >        _XkbCheckBounds(bounds,rbounds->x1,rbounds->y1);
> >                               ^~~~~~~
> > 
> > Found-by: clang static analyzer
> > Signed-off-by: Jeremy Huddleston <jeremyhu at apple.com>
> > ---
> > src/xkb/XKBGeom.c |    4 ++--
> > 1 files changed, 2 insertions(+), 2 deletions(-)
> > 
> > diff --git a/src/xkb/XKBGeom.c b/src/xkb/XKBGeom.c
> > index e9e36d0..2365f48 100644
> > --- a/src/xkb/XKBGeom.c
> > +++ b/src/xkb/XKBGeom.c
> > @@ -147,7 +147,7 @@ register int	i;
> > XkbShapePtr	shape;
> > XkbRowPtr	row;
> > XkbDoodadPtr	doodad;
> > -XkbBoundsPtr	bounds,rbounds=NULL;
> > +XkbBoundsPtr	bounds,rbounds;
> > 
> >     if ((!geom)||(!section))
> > 	return False;
> > @@ -186,7 +186,7 @@ XkbBoundsPtr	bounds,rbounds=NULL;
> > 	    default:
> > 		tbounds.x1= tbounds.x2= doodad->any.left;
> > 		tbounds.y1= tbounds.y2= doodad->any.top;
> > -		break;
> > +		continue;
> > 	}
> > 	_XkbCheckBounds(bounds,rbounds->x1,rbounds->y1);
> > 	_XkbCheckBounds(bounds,rbounds->x2,rbounds->y2);
> > -- 
> > 1.7.4.1