[PATCH] omGeneric: Fix NULL pointer dereferences in set_fontset_extents

[Ismael Luceno]

On Wed, 15 Jul 2015 15:57:17 -0300
Ismael Luceno <ismael at iodev.co.uk> wrote:
> On Wed, 15 Jul 2015 07:24:59 -0700
> Alan Coopersmith <alan.coopersmith at oracle.com> wrote:
> > On 07/14/15 09:36 PM, Ismael Luceno wrote:
> > > Signed-off-by: Ismael Luceno <ismael at iodev.co.uk>
> > > ---
> > >   modules/om/generic/omGeneric.c | 4 ++--
> > >   1 file changed, 2 insertions(+), 2 deletions(-)
> > >
> > > diff --git a/modules/om/generic/omGeneric.c
> > > b/modules/om/generic/omGeneric.c index a835f00..f0861e8 100644
> > > --- a/modules/om/generic/omGeneric.c
> > > +++ b/modules/om/generic/omGeneric.c
> > > @@ -398,7 +398,7 @@ set_fontset_extents(
> > >   		font_data = font_set->vmap;
> > >   		font_data_count = font_set->vmap_num;
> > >   		for( ; font_data_count-- ; font_data++) {
> > > -		    if(font_data->font != NULL) {
> > > +		    if(font_data && font_data->font) {
> > >   			check_fontset_extents(&overall,
> > > &logical_ascent, &logical_descent,
> > >   					      font_data->font);
> > 
> > This segment is wrapped in a check that should already handle it:
> >              if(font_set->vmap_num > 0) {
> > 
> > Have you actually encountered cases where font_set->vmap is NULL but
> > font_set->vmap_num is > 0 ?
> 
> Yes, I found the problem with dillo compiled against fltk 1.3.3. I
> thought it was strange, and there should be more to it, but needed a
> quick fix.
> 
> > 
> > > @@ -410,7 +410,7 @@ set_fontset_extents(
> > >   		font_data = (FontData) font_set->vrotate;
> > >   		font_data_count = font_set->vrotate_num;
> > >   		for( ; font_data_count-- ; font_data++) {
> > > -		    if(font_data->font != NULL) {
> > > +		    if(font_data && font_data->font) {
> > >   			check_fontset_extents(&overall,
> > > &logical_ascent, &logical_descent,
> > >   					      font_data->font);
> > 
> > This one is more definitively handled in the wrapping check:
> > 
> >              if(font_set->vrotate_num > 0 && font_set->vrotate !=
> > NULL) {
> > 
> > I can't see any way font_data could ever be NULL here.
> > 
> 
> I've been thinking it may be a bug in GCC (5.1), I see it optimizes
> out a lot around there, but had no time to confirm it yet...
> 
> I can upload the output if you want to look at the machine code.
> 
> I will try building with different flags to point out the problem
> (might take some time, there's a lot to test and I'm quite busy with
> other stuff).

It seems to be an issue with GCC's speculative load scheduling, but
I'm not entirely sure that what GCC is doing is wrong...