bug#8215: possibly uninitialized variable lower_xoff in produce_glyphless_glyph

bug#8215: possibly uninitialized variable lower_xoff in produce_glyphless_glyph

[Paul Eggert]

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.

bug#8215: possibly uninitialized variable lower_xoff in produce_glyphless_glyph

[Lars Ingebrigtsen]

Paul Eggert <eggert@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

bug#8215: possibly uninitialized variable lower_xoff in produce_glyphless_glyph

[Eli Zaretskii]

> From: Lars Ingebrigtsen <larsi@gnus.org>
> Date: Wed, 02 Jun 2021 10:06:29 +0200
> Cc: 8215@debbugs.gnu.org, Kenichi Handa <handa@m17n.org>
> 
> Paul Eggert <eggert@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.

bug#8215: possibly uninitialized variable lower_xoff in produce_glyphless_glyph

[Lars Ingebrigtsen]

Eli Zaretskii <eliz@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

bug#8215: possibly uninitialized variable lower_xoff in produce_glyphless_glyph

[Eli Zaretskii]

> From: Lars Ingebrigtsen <larsi@gnus.org>
> Cc: eggert@cs.ucla.edu,  8215@debbugs.gnu.org,  Kenichi Handa <handa@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.