Re: [PATCH v3 02/25] winansi: avoid use of uninitialized value

[Johannes Schindelin <Johannes.Schindelin@gmx.de>]

[-- Attachment #1: Type: text/plain, Size: 4181 bytes --]

Hi René,

On Wed, 3 May 2017, René Scharfe wrote:

> Am 02.05.2017 um 18:01 schrieb Johannes Schindelin:
> > 
> > diff --git a/compat/winansi.c b/compat/winansi.c
> > index 793420f9d0d..fd6910746c8 100644
> > --- a/compat/winansi.c
> > +++ b/compat/winansi.c
> > @@ -105,6 +105,8 @@ static int is_console(int fd)
> >    if (!fd) {
> >     if (!GetConsoleMode(hcon, &mode))
> >   			return 0;
> > +		sbi.wAttributes = FOREGROUND_BLUE | FOREGROUND_GREEN |
> > +			FOREGROUND_RED;
> >    } else if (!GetConsoleScreenBufferInfo(hcon, &sbi))
> >     return 0;
> >   
> 
> So is_console is called with fd being 1 (stdout), 2 (stderr) and 0
> (stdin), in that order.

Correct. I guess this is the important part missing from the commit
message.

Oh, I also saw that I talked about stdout in the commit message, but !fd
tests for stdin!

> If the first two calls abort early for some reason we may end up here.

Yep. "some reason" being: there is no console attached to stdout nor
stderr.

> The added code is for white text on black background.  An alias for that
> combination, FOREGROUND_ALL, is defined a few lines down; for a minimal
> fix it's not worth moving it up.  attr and plain_attr are both
> initialized to sbi.wAttributes.

Exactly.

> That as a bit more complicated than it looked initially.  The order of
> calls is important, "stdout" in the commit message includes stderr as
> well and it doesn't just affect plain_attr.

Right, it also affects the "current" attributes.

> Would a value of 0 (black text on black background) suffice if only
> stdin is connected to a console?  Colors don't matter if there is
> nothing to see, right?

I think that would make it both easier to understand the patch and to
catch regressions in case anybody feels the order of the is_console()
calls should be changed...

This is my current squash! commit (the original commit message will be
replaced by the commit message body of this commit):

-- snipsnap --
Subject: [PATCH] squash! winansi: avoid use of uninitialized value

winansi: avoid use of uninitialized value

To initialize the foreground color attributes of "plain text", our ANSI
emulation tries to infer them from the currently attached console while
running the is_console() function. This function first tries to detect any
console attached to stdout, then it is called with stderr.

If neither stdout nor stderr has any console attached, it does not
actually matter what we use for "plain text" attributes, as we never need
to output any text to any console in that case.

However, after working on stdout and stderr, is_console() is called with
stdin, and it still tries to initialize the "plain text" attributes if
they had not been initialized earlier. In this case, we cannot detect any
attributes, and we used an uninitialized value for them.

Naturally, Coverity complained about this use case because it could not
reason about the code deeply enough to figure out that we do not even use
those attributes in that case.

Let's just initialize the value to 0 in that case, both to avoid future
Coverity reports, and to help catch future regressions in case anybody
changes the order of the is_console() calls (which would make the text
black on black).

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
 compat/winansi.c | 9 +++++++--
 1 file changed, 7 insertions(+), 2 deletions(-)

diff --git a/compat/winansi.c b/compat/winansi.c
index 861b79d8c31..a11a0f16d27 100644
--- a/compat/winansi.c
+++ b/compat/winansi.c
@@ -105,8 +105,13 @@ static int is_console(int fd)
 	if (!fd) {
 		if (!GetConsoleMode(hcon, &mode))
 			return 0;
-		sbi.wAttributes = FOREGROUND_BLUE | FOREGROUND_GREEN |
-			FOREGROUND_RED;
+		/*
+		 * This code path is only reached if there is no console
+		 * attached to stdout/stderr, i.e. we will not need to output
+		 * any text to any console, therefore we might just as well
+		 * use black as foreground color.
+		 */
+		sbi.wAttributes = 0;
 	} else if (!GetConsoleScreenBufferInfo(hcon, &sbi))
 		return 0;
 
-- 
2.12.2.windows.2.800.gede8f145e06