git/list[1] front-page[2] threads[3] people[4] search[5] about
 

[PATCH 2/2] color_parse_mem: initialize "struct color" temporary

From
Jeff King <peff@peff.net>
Date
Aug 31, 2016, 03:43 UTC
Message-ID
<20160831034307.vfrxdhbos26cqxka@sigill.intra.peff.net>
In-Reply-To
<20160831033934.m2at7ci5f6lty5nb@sigill.intra.peff.net>

Compiling color.c with gcc 6.2.0 using -O3 produces some -Wmaybe-uninitialized false positives:

    color.c: In function ‘color_parse_mem’:
    color.c:189:10: warning: ‘bg.blue’ may be used uninitialized in this function [-Wmaybe-uninitialized]
       out += xsnprintf(out, len, "%c8;2;%d;%d;%d", type,
              ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
          c->red, c->green, c->blue);
          ~~~~~~~~~~~~~~~~~~~~~~~~~~
    color.c:208:15: note: ‘bg.blue’ was declared here
      struct color bg = { COLOR_UNSPECIFIED };
                   ^~
    [ditto for bg.green, bg.red, fg.blue, etc]

This is doubly confusing, because the declaration shows it being initialized! Even though we do not explicitly initialize the color components, an incomplete initializer sets the unmentioned members to zero.

What the warning doesn't show is that we later do this:
  struct color c;
  if (!parse_color(&c, ...)) {
          if (fg.type == COLOR_UNSPECIFIED)
                fg = c;
          ...
  }

gcc is clever enough to realize that a struct assignment from an uninitialized variable taints the destination. But unfortunately it's _not_ clever enough to realize that we only look at those members when type is set to COLOR_RGB, in which case they are always initialized.

With -O2, gcc does not look into parse_color() and must assume that "c" emerges fully initialized. With -O3, it inlines parse_color(), and learns just enough to get confused.

We can silence the false positive by initializing the temporary "c". This also future-proofs us against violating the type assumptions (the result would probably still be buggy, but in a deterministic way).

Signed-off-by: Jeff King <peff@peff.net>
---
Of course it's possible that I am wrong and gcc is right, but I just
don't see it (and that was the real reason I dug; I don't care _that_
much about -O3 warnings, but I wanted to see if gcc had found a real
bug).
 color.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/color.c b/color.c
index 81c2676..1b95e6b 100644
--- a/color.c
+++ b/color.c
@@ -215,7 +215,7 @@ int color_parse_mem(const char *value, int value_len, char *dst)
 	/* [fg [bg]] [attr]... */
 	while (len > 0) {
 		const char *word = ptr;
-		struct color c;
+		struct color c = { COLOR_UNSPECIFIED };
 		int val, wordlen = 0;
 
 		while (len > 0 && !isspace(word[wordlen])) {
-- 
2.10.0.rc2.125.gcfb3d08
Previous: Jeff KingNext: Junio C Hamano
Message 3 of 5 in “squelch some "gcc -O3 -Wmaybe-uninitialized" warnings”
  1. 0/2 squelch some "gcc -O3 -Wmaybe-uninitialized" warningsJeff King, Aug 31, 2016
  2. 1/2 error_errno: use constant return similar to error()Jeff King, Aug 31, 2016
  3. 2/2 color_parse_mem: initialize "struct color" temporaryJeff King, Aug 31, 2016
  4. Junio C HamanoAug 31, 2016
  5. Jeff KingSep 1, 2016

Read the whole thread, see it on lore, or plain text.

$ cat FOOTERMessages come from the public archive at lore.kernel.org/git, fetched every hour. The front page is chosen and written each morning by an AI editor and can be wrong; the threads themselves are the record. About and API. For agents: an MCP server at https://gitlist.dev/mcp, and any thread, story or person page as Markdown by adding .md to its URL (or sending Accept: text/markdown). Details in /llms.txt.