Problem report #16

Problem report #16

[Dan Nicolaescu]

CID: 16
Checker: FORWARD_NULL (help)
File: emacs/src/fontset.c
Function: fs_load_font
Description: Variable "face" tracked as NULL was dereferenced.

Event var_compare_op: Added "face" due to comparison "face != 0"
Also see events: [var_deref_op]
At conditional (1): "face != 0" taking false path

667  	  if (face)
668  	    id = face->fontset;

At conditional (2): "id < 0" taking true path

669  	  if (id < 0)
670  	    fontset = Qnil;
671  	  else
672  	    fontset = FONTSET_FROM_ID (id);
673  	

At conditional (3): "fontset != Qnil" taking true path
At conditional (4): "(fontset & -8)->parent != Qnil" taking true path

674  	  if (!NILP (fontset)
675  	      && !BASE_FONTSET_P (fontset))
676  	    {
677  	      elt = FONTSET_REF_VIA_BASE (fontset, c);

At conditional (5): "elt != Qnil" taking true path

678  	      if (!NILP (elt))
679  		{
680  		  /* A suitable face for C is already recorded, which means
681  		     that a proper font is already loaded.  */
682  		  int face_id = XINT (elt);
683  	
684  		  xassert (face_id == face->id);

At conditional (6): "face_id < ((f)->face_cache)->used" taking false path

685  		  face = FACE_FROM_ID (f, face_id);

Event var_deref_op: Variable "face" tracked as NULL was dereferenced.
Also see events: [var_compare_op]

686  		  return (*get_font_info_func) (f, face->font_info_id);
687  		}

Re: Problem report #16

[Stuart D. Herring]

> At conditional (2): "id < 0" taking true path
>
> 669  	  if (id < 0)
> 670  	    fontset = Qnil;

OK, so Qnil -> fontset.  Got it.

> 671  	  else
> 672  	    fontset = FONTSET_FROM_ID (id);
> 673
>
> At conditional (3): "fontset != Qnil" taking true path

Um, what?  No bug, duh.

Davis

-- 
This product is sold by volume, not by mass.  If it appears too dense or
too sparse, it is because mass-energy conversion has occurred during
shipping.

Re: Problem report #16

[Dan Nicolaescu]

Dan Nicolaescu <dann@ics.uci.edu> writes:

  > CID: 16
  > Checker: FORWARD_NULL (help)
  > File: emacs/src/fontset.c
  > Function: fs_load_font
  > Description: Variable "face" tracked as NULL was dereferenced.
  > 
  > Event var_compare_op: Added "face" due to comparison "face != 0"
  > Also see events: [var_deref_op]
  > At conditional (1): "face != 0" taking false path
  > 
  > 667  	  if (face)
  > 668  	    id = face->fontset;
  > 
  > At conditional (2): "id < 0" taking true path
  > 
  > 669  	  if (id < 0)
  > 670  	    fontset = Qnil;
  > 671  	  else
  > 672  	    fontset = FONTSET_FROM_ID (id);
  > 673  	
  > 
  > At conditional (3): "fontset != Qnil" taking true path
  > At conditional (4): "(fontset & -8)->parent != Qnil" taking true path
  > 
  > 674  	  if (!NILP (fontset)
  > 675  	      && !BASE_FONTSET_P (fontset))
  > 676  	    {
  > 677  	      elt = FONTSET_REF_VIA_BASE (fontset, c);
  > 
  > At conditional (5): "elt != Qnil" taking true path
  > 
  > 678  	      if (!NILP (elt))
  > 679  		{
  > 680  		  /* A suitable face for C is already recorded, which means
  > 681  		     that a proper font is already loaded.  */
  > 682  		  int face_id = XINT (elt);
  > 683  	
  > 684  		  xassert (face_id == face->id);
  > 
  > At conditional (6): "face_id < ((f)->face_cache)->used" taking false path
  > 
  > 685  		  face = FACE_FROM_ID (f, face_id);
  > 
  > Event var_deref_op: Variable "face" tracked as NULL was dereferenced.
  > Also see events: [var_compare_op]
  > 
  > 686  		  return (*get_font_info_func) (f, face->font_info_id);
  > 687  		}


This problem could happen if fs_load_font was called with face=NULL
and id>0. Can that happen? 

Re: Problem report #16

[Stuart D. Herring]

There's actually several possible problems here.

>   > At conditional (1): "face != 0" taking false path
>   >
>   > 667  	  if (face)
>   > 668  	    id = face->fontset;

This is a red herring -- face is reassigned later.

>   > At conditional (2): "id < 0" taking true path
>   >
>   > 669  	  if (id < 0)
>   > 670  	    fontset = Qnil;
>   > 671  	  else
>   > 672  	    fontset = FONTSET_FROM_ID (id);

Here's one thing I don't like: FONTSET_FROM_ID doesn't do any safety
checks on id.  Can we trust this function to always get reasonable id
values?

>   > 682  		  int face_id = XINT (elt);
>   > 684  		  xassert (face_id == face->id);

Dan wrote (although not here in his message):
> This problem could happen if fs_load_font was called with face=NULL
> and id>0. Can that happen?

Here's where Dan's point is relevant: if face==NULL and id>0, then it
seems quite possible for this line to be reached...

>   > 685  		  face = FACE_FROM_ID (f, face_id);

...just before assigning face, so it'd still be NULL.  But there's more:
FACE_FROM_ID can fail and return NULL:

>   > Event var_deref_op: Variable "face" tracked as NULL was dereferenced.
>   > Also see events: [var_compare_op]
>   >
>   > 686  		  return (*get_font_info_func) (f, face->font_info_id);

So this part is dangerous if and only if face_id, derived from the
fontset, can be messed up.  Nothing to do with face's value at entry to
the function.  Someone who understands fontsets/font loading, comment?

Davis

-- 
This product is sold by volume, not by mass.  If it appears too dense or
too sparse, it is because mass-energy conversion has occurred during
shipping.

Re: Problem report #16

[Kenichi Handa]

In article <41269.128.165.123.132.1144866216.squirrel@webmail.lanl.gov>, "Stuart D. Herring" <herring@lanl.gov> writes:

> There's actually several possible problems here.
>> > At conditional (1): "face != 0" taking false path
>> >
>> > 667  	  if (face)
>> > 668  	    id = face->fontset;

> This is a red herring -- face is reassigned later.

Why? face is reassigned as a temporary value folder just
before return (L686).

>> > At conditional (2): "id < 0" taking true path
>> >
>> > 669  	  if (id < 0)
>> > 670  	    fontset = Qnil;
>> > 671  	  else
>> > 672  	    fontset = FONTSET_FROM_ID (id);

> Here's one thing I don't like: FONTSET_FROM_ID doesn't do any safety
> checks on id.  Can we trust this function to always get reasonable id
> values?

That's the other way round.  FONTSET_FROM_ID should be
called only when ID is valid.  And, here, it is assured;
that is to say, if face != NULL, face->fontset must be -1 or
valid, and if face == NULL, the given ID must be -1 or
valid.

>> > 682  		  int face_id = XINT (elt);
>> > 684  		  xassert (face_id == face->id);

> Dan wrote (although not here in his message):
>> This problem could happen if fs_load_font was called with face=NULL
>> and id>0. Can that happen?

> Here's where Dan's point is relevant: if face==NULL and id>0, then it
> seems quite possible for this line to be reached...

>> > 685  		  face = FACE_FROM_ID (f, face_id);

There is just one place calling fs_load_font() with
face==NULL and id>=0; list_fontsets() in fontset.c.  But
that function calls fs_load_font() (via FS_LOAD_FONT macro)
only if BASE_FONTSET_P (fontset) is true, in which
case, this line is never reached.

> ...just before assigning face, so it'd still be NULL.  But there's more:
> FACE_FROM_ID can fail and return NULL:

>> > Event var_deref_op: Variable "face" tracked as NULL was dereferenced.
>> > Also see events: [var_compare_op]
>> >
>> > 686  		  return (*get_font_info_func) (f, face->font_info_id);

> So this part is dangerous if and only if face_id, derived from the
> fontset, can be messed up.

face_id derived from the fontset should be always valid.  If
not, that means there's a bug somewhere else.

Anyway, fs_load_font() is now very complicated because of
repeated changes in the long history.  So, I rewrote
fontset.c in emacs-unicode-2 branch.  It will be helpful if
the similar report is issued on that branch because it
contains many new codes.

---
Kenichi Handa
handa@m17n.org