GNU bug report logs - #8215
possibly uninitialized variable lower_xoff in produce_glyphless_glyph

Previous Next

To add a comment to this bug, you must first unarchive it, by sending
a message to control AT debbugs.gnu.org, with unarchive 8215 in the body.
You can then email your comments to 8215 AT debbugs.gnu.org in the normal way.

Toggle the display of automated, internal messages from the tracker.

Message #5 received at submit <at> debbugs.gnu.org (full text, mbox):

From: Paul Eggert <eggert <at> cs.ucla.edu>
To: bug-gnu-emacs <at> gnu.org
Cc: Kenichi Handa <handa <at> m17n.org>
Subject: possibly uninitialized variable lower_xoff in produce_glyphless_glyph
Date: Wed, 09 Mar 2011 14:00:12 -0800

I found this problem by compiling Emacs with GCC's -Wuninitialized flag.

The following code in the Emacs trunk src/xdisp.c's
produce_glyphless_glyph function might be using an uninitialized
variable:

      if (base_width >= width)
	{
	  /* Align the upper to the left, the lower to the right.  */
	  it->pixel_width = base_width;
	  lower_xoff = base_width - 2 - metrics_lower.width;
	}
      else
	{
	  /* Center the shorter one.  */
	  it->pixel_width = width;
	  if (metrics_upper.width >= metrics_lower.width)
	    lower_xoff = (width - metrics_lower.width) / 2;
	  else
	    upper_xoff = (width - metrics_upper.width) / 2;
	}
  ...
  if (it->glyph_row)
    append_glyphless_glyph (it, face_id, for_no_font, len,
			    upper_xoff, upper_yoff,
			    lower_xoff, lower_yoff);

The last call uses lower_xoff, but the last "else" does not initialize
lower_xoff.  The bug cannot occur if it->glyph_row is NULL, but I
don't see why that would necessarily be.  So I'm filing a bug report
so that someone who is more expert in this code can take a look at it.
In the meantime, I plan to work around the problem by initializing
lower_xoff to 0, with a FIXME explaining the situation: this shouldn't
introduce a bug, because at worst it will replace undefined behavior
with defined behavior.

I'm CC'ing this to Kenichi Handa, who committed the code in question.

Message #8 received at 8215 <at> debbugs.gnu.org (full text, mbox):

From: Paul Eggert <eggert <at> cs.ucla.edu>
To: 8229 <at> debbugs.gnu.org, 8215 <at> debbugs.gnu.org, 8211 <at> debbugs.gnu.org
Subject: committed the workaround
Date: Wed, 23 Mar 2011 16:19:24 -0700

I committed my abovementioned workaround into the
Emacs trunk on 2011-03-11 (bzr 103589).  I don't
consider this a fix, though, so I'm leaving this
bug report open.

Message #11 received at 8215 <at> debbugs.gnu.org (full text, mbox):

From: Lars Ingebrigtsen <larsi <at> gnus.org>
To: Paul Eggert <eggert <at> cs.ucla.edu>
Cc: 8211 <at> debbugs.gnu.org, 8229 <at> debbugs.gnu.org, 8215 <at> debbugs.gnu.org
Subject: Re: bug#8229: possibly uninitialized variable in load_charset
Date: Wed, 02 Jun 2021 10:03:14 +0200

Paul Eggert <eggert <at> cs.ucla.edu> writes:

> I committed my abovementioned workaround into the
> Emacs trunk on 2011-03-11 (bzr 103589).  I don't
> consider this a fix, though, so I'm leaving this
> bug report open.

This was ten years ago:

commit 0ac2c2991c1cba4e3c6e5f7b62c7d61b01d69994
Author:     Paul Eggert <eggert <at> cs.ucla.edu>
AuthorDate: Mon Mar 7 16:46:23 2011 -0800
Commit:     Paul Eggert <eggert <at> cs.ucla.edu>
CommitDate: Mon Mar 7 16:46:23 2011 -0800

    * charset.c (load_charset): Abort instead of using uninitialized var.

The code is still pretty much identical, as far as I can tell.  Should
this report be closed now?

-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no

Message #14 received at 8215 <at> debbugs.gnu.org (full text, mbox):

From: Lars Ingebrigtsen <larsi <at> gnus.org>
To: Paul Eggert <eggert <at> cs.ucla.edu>
Cc: 8215 <at> debbugs.gnu.org, Kenichi Handa <handa <at> m17n.org>
Subject: Re: bug#8215: possibly uninitialized variable lower_xoff in
 produce_glyphless_glyph
Date: Wed, 02 Jun 2021 10:06:29 +0200

Paul Eggert <eggert <at> cs.ucla.edu> writes:

> In the meantime, I plan to work around the problem by initializing
> lower_xoff to 0, with a FIXME explaining the situation: this shouldn't
> introduce a bug, because at worst it will replace undefined behavior
> with defined behavior.

It looks like this code is still in place now, ten years later:

diff --git a/src/xdisp.c b/src/xdisp.c
index 44cb713011..44a317b578 100644
--- a/src/xdisp.c
+++ b/src/xdisp.c
@@ -22292,7 +22292,13 @@ produce_glyphless_glyph (struct it *it, int for_no_font, Lisp_Object acronym)
 	  if (metrics_upper.width >= metrics_lower.width)
 	    lower_xoff = (width - metrics_lower.width) / 2;
 	  else
-	    upper_xoff = (width - metrics_upper.width) / 2;
+	    {
+	      /* FIXME: This code doesn't look right.  It formerly was
+		 missing the "lower_xoff = 0;", which couldn't have
+		 been right since it left lower_xoff uninitialized.  */
+	      lower_xoff = 0;
+	      upper_xoff = (width - metrics_upper.width) / 2;
+	    }
 	}
 
       /* +5 is for horizontal bars of a box plus 1-pixel spaces at

Anybody have any insight into whether this is correct or not now?

-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no

Message #17 received at 8215 <at> debbugs.gnu.org (full text, mbox):

From: Eli Zaretskii <eliz <at> gnu.org>
To: Lars Ingebrigtsen <larsi <at> gnus.org>
Cc: Kenichi Handa <handa <at> gnu.org>, eggert <at> cs.ucla.edu, 8215 <at> debbugs.gnu.org
Subject: Re: bug#8215: possibly uninitialized variable lower_xoff in
 produce_glyphless_glyph
Date: Wed, 02 Jun 2021 16:17:11 +0300

> From: Lars Ingebrigtsen <larsi <at> gnus.org>
> Date: Wed, 02 Jun 2021 10:06:29 +0200
> Cc: 8215 <at> debbugs.gnu.org, Kenichi Handa <handa <at> m17n.org>
> 
> Paul Eggert <eggert <at> cs.ucla.edu> writes:
> 
> > In the meantime, I plan to work around the problem by initializing
> > lower_xoff to 0, with a FIXME explaining the situation: this shouldn't
> > introduce a bug, because at worst it will replace undefined behavior
> > with defined behavior.
> 
> It looks like this code is still in place now, ten years later:
> 
> diff --git a/src/xdisp.c b/src/xdisp.c
> index 44cb713011..44a317b578 100644
> --- a/src/xdisp.c
> +++ b/src/xdisp.c
> @@ -22292,7 +22292,13 @@ produce_glyphless_glyph (struct it *it, int for_no_font, Lisp_Object acronym)
>  	  if (metrics_upper.width >= metrics_lower.width)
>  	    lower_xoff = (width - metrics_lower.width) / 2;
>  	  else
> -	    upper_xoff = (width - metrics_upper.width) / 2;
> +	    {
> +	      /* FIXME: This code doesn't look right.  It formerly was
> +		 missing the "lower_xoff = 0;", which couldn't have
> +		 been right since it left lower_xoff uninitialized.  */
> +	      lower_xoff = 0;
> +	      upper_xoff = (width - metrics_upper.width) / 2;
> +	    }
>  	}
>  
>        /* +5 is for horizontal bars of a box plus 1-pixel spaces at
> 
> Anybody have any insight into whether this is correct or not now?

I fixed this (and removed the FIXME with the incorrect
initialization).  Bottom line: it was a typo.

Message #22 received at 8215 <at> debbugs.gnu.org (full text, mbox):

From: Lars Ingebrigtsen <larsi <at> gnus.org>
To: Eli Zaretskii <eliz <at> gnu.org>
Cc: Kenichi Handa <handa <at> gnu.org>, eggert <at> cs.ucla.edu, 8215 <at> debbugs.gnu.org
Subject: Re: bug#8215: possibly uninitialized variable lower_xoff in
 produce_glyphless_glyph
Date: Sun, 06 Jun 2021 11:00:04 +0200

Eli Zaretskii <eliz <at> gnu.org> writes:

> I fixed this (and removed the FIXME with the incorrect
> initialization).  Bottom line: it was a typo.

Nice catch.  :-)  I tried reading that function for a couple of minutes
to try to make sense how lower_xoff should have been initialised, but I
had to admit defeat.

-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no

Message #25 received at 8215 <at> debbugs.gnu.org (full text, mbox):

From: Eli Zaretskii <eliz <at> gnu.org>
To: Lars Ingebrigtsen <larsi <at> gnus.org>
Cc: handa <at> gnu.org, eggert <at> cs.ucla.edu, 8215 <at> debbugs.gnu.org
Subject: Re: bug#8215: possibly uninitialized variable lower_xoff in
 produce_glyphless_glyph
Date: Sun, 06 Jun 2021 12:13:41 +0300

> From: Lars Ingebrigtsen <larsi <at> gnus.org>
> Cc: eggert <at> cs.ucla.edu,  8215 <at> debbugs.gnu.org,  Kenichi Handa <handa <at> gnu.org>
> Date: Sun, 06 Jun 2021 11:00:04 +0200
> 
> Nice catch.  :-)  I tried reading that function for a couple of minutes
> to try to make sense how lower_xoff should have been initialised, but I
> had to admit defeat.

The reason we didn't have the fix earlier is that the problem is only
visible when the default face's font is variable-pitch, otherwise the
offending code is never executed.  So it was hard to understand what
that "workaround" initialization caused.

Let me know if I should add some comments there to make the code's
intent and purpose more clear.

Previous Next

GNU bug tracking system
Copyright (C) 1999 Darren O. Benham, 1997,2003 nCipher Corporation Ltd, 1994-97 Ian Jackson.