Bug#830313: watch: segfaults with --color

[Josh Triplett]

Heh, yes. :)

- Josh Triplett

On Sat, Jul 09, 2016 at 05:53:35AM +, Craig Small wrote:
> I messed up the manual patch when it wouldn't apply. I put the return
> before the attrset() That'll do it!
> 
>  - Craig
> 
> 
> On Sat, Jul 9, 2016 at 3:38 PM Craig Small  wrote:
> 
> > there seems to be a problem, watch is now not interpreting any ansi
> > sequences.
> > im bisecting it now to work out what went wrong, one of the patches didnt
> > apply cleanly so i suspect that one.
> >
> >
> > On Sat, Jul 9, 2016 at 3:12 PM Josh Triplett 
> > wrote:
> >
> >> On Sat, Jul 09, 2016 at 04:59:07AM +, Craig Small wrote:
> >> > Hi Josh,
> >> >   Thanks for looking into this, I only do some simple use of watch so
> >> don't
> >> > see the problems. I agree, if it doesn't understand something then stop
> >> > messing around and drop out.
> >> >
> >> > Patch 0001 was already fixed upstream commit 6fcb6900 has a similar fix
> >> > The other three patches have been applied upstream.
> >>
> >> Thanks!
> >>
> >> It looks like the handling for \e[m still has a bug in it, as it has a
> >> condition for that both before and after the loop.  The one after the
> >> loop looks wrong; I think the one before the loop is the only one that
> >> makes sense.
> >>
> >> - Josh Triplett
> >>
> >

Bug#830313: watch: segfaults with --color

[Craig Small]

I messed up the manual patch when it wouldn't apply. I put the return
before the attrset() That'll do it!

 - Craig


On Sat, Jul 9, 2016 at 3:38 PM Craig Small  wrote:

> there seems to be a problem, watch is now not interpreting any ansi
> sequences.
> im bisecting it now to work out what went wrong, one of the patches didnt
> apply cleanly so i suspect that one.
>
>
> On Sat, Jul 9, 2016 at 3:12 PM Josh Triplett 
> wrote:
>
>> On Sat, Jul 09, 2016 at 04:59:07AM +, Craig Small wrote:
>> > Hi Josh,
>> >   Thanks for looking into this, I only do some simple use of watch so
>> don't
>> > see the problems. I agree, if it doesn't understand something then stop
>> > messing around and drop out.
>> >
>> > Patch 0001 was already fixed upstream commit 6fcb6900 has a similar fix
>> > The other three patches have been applied upstream.
>>
>> Thanks!
>>
>> It looks like the handling for \e[m still has a bug in it, as it has a
>> condition for that both before and after the loop.  The one after the
>> loop looks wrong; I think the one before the loop is the only one that
>> makes sense.
>>
>> - Josh Triplett
>>
>

Bug#830313: watch: segfaults with --color

[Craig Small]

there seems to be a problem, watch is now not interpreting any ansi
sequences.
im bisecting it now to work out what went wrong, one of the patches didnt
apply cleanly so i suspect that one.


On Sat, Jul 9, 2016 at 3:12 PM Josh Triplett  wrote:

> On Sat, Jul 09, 2016 at 04:59:07AM +, Craig Small wrote:
> > Hi Josh,
> >   Thanks for looking into this, I only do some simple use of watch so
> don't
> > see the problems. I agree, if it doesn't understand something then stop
> > messing around and drop out.
> >
> > Patch 0001 was already fixed upstream commit 6fcb6900 has a similar fix
> > The other three patches have been applied upstream.
>
> Thanks!
>
> It looks like the handling for \e[m still has a bug in it, as it has a
> condition for that both before and after the loop.  The one after the
> loop looks wrong; I think the one before the loop is the only one that
> makes sense.
>
> - Josh Triplett
>

Bug#830313: watch: segfaults with --color

[Josh Triplett]

On Sat, Jul 09, 2016 at 04:59:07AM +, Craig Small wrote:
> Hi Josh,
>   Thanks for looking into this, I only do some simple use of watch so don't
> see the problems. I agree, if it doesn't understand something then stop
> messing around and drop out.
> 
> Patch 0001 was already fixed upstream commit 6fcb6900 has a similar fix
> The other three patches have been applied upstream.

Thanks!

It looks like the handling for \e[m still has a bug in it, as it has a
condition for that both before and after the loop.  The one after the
loop looks wrong; I think the one before the loop is the only one that
makes sense.

- Josh Triplett

Bug#830313: watch: segfaults with --color

[Craig Small]

Hi Josh,
  Thanks for looking into this, I only do some simple use of watch so don't
see the problems. I agree, if it doesn't understand something then stop
messing around and drop out.

Patch 0001 was already fixed upstream commit 6fcb6900 has a similar fix
The other three patches have been applied upstream.

 - Craig

Bug#830313: watch: segfaults with --color

[Josh Triplett]

Package: procps
Version: 2:3.3.11-3
Severity: normal
File: /usr/bin/watch
Tags: upstream patch

A minimal test case that reproduces this:
watch -c -t '/usr/bin/printf "\x1b[m"'

The underlying bug involves a read of an uninitialized pointer in the
implementation of --color (so the method of reproducing the segfault may
vary).

The implementation of --color calls the following function whenever it
encounters an escape character ('\033'):

static void process_ansi(FILE * fp)
{
int i, c;
char buf[MAX_ANSIBUF];
char *numstart, *endptr;

c = getc(fp);
if (c != '[') {
ungetc(c, fp);
return;
}
for (i = 0; i < MAX_ANSIBUF; i++) {
c = getc(fp);
/* COLOUR SEQUENCE ENDS in 'm' */
if (c == 'm') {
buf[i] = '\0';
break;
}
if (c < '0' && c > '9' && c != ';') {
while (--i >= 0)
ungetc(buf[i], fp);
return;
}
buf[i] = (char)c;
}
/*
 * buf now contains a semicolon-separated list of decimal integers,
 * each indicating an attribute to apply.
 * For example, buf might contain "0;1;31", derived from the color
 * escape sequence "[0;1;31m". There can be 1 or more
 * attributes to apply, but typically there are between 1 and 3.
 */

if (*endptr == '\0') set_ansi_attribute(0); /* [m treated as [0m */

for (endptr = numstart = buf; *endptr != '\0'; numstart = endptr + 1)
set_ansi_attribute(strtol(numstart, , 10));
}

This function has several problems:

- The if near the end dereferences *endptr without initializing it.
  This causes the segfault.

- The condition (c < '0' && c > '9' && c != ';') will never match, so
  this will happily read MAX_ANSIBUF characters unless it encounters a
  'm'.  (This will also produce interesting results when c == EOF.)
  This should use ((c < '0' || c > '9') && c != ';').

- Even with that condition fixed, this does not ungetc the character it
  just read, nor does it ungetc the '[' character or the '\x1b' escape
  character.

- This assumes, unportably, that it can ungetc more than one character;
  "man ungetc" specifically says "only one pushback is guaranteed", and
  in fact glibc only supports one character.  (In any case, the rest of
  the escape sequence should ideally be skipped, not printed; that will
  also fix the previous issue of not ungetting some of the characters.)

- This assumes all numbers in a color control sequence correspond to
  colors or attributes, which will breaks badly if it encounters a
  ISO-8613-3 escape sequence (such as for truecolor RGB).  For instance,
  the sequence "\x1b[38;2;10;20;30m" sets the foreground color to
  rgb(10,20,30), but watch will interpret all five numbers in the
  sequence as colors or attributes themselves.  watch should stop
  processing the entire escape sequence if it encounters something it
  doesn't understand.  (I'd suggest doing this by having
  set_ansi_attribute return a bool, with false meaning "skip the rest".)

If not for ncurses, I would suggest using the behavior of "less -R":
matching ANSI escape sequences, discarding non-color sequences, and
passing through color sequences assuming they don't move the cursor.
However, that won't work with ncurses.

I've attached a series of git patches that fix the problems noted above.

- Josh Triplett
>From c41c19d92da12b32a73222c7a6b286570e3eecb7 Mon Sep 17 00:00:00 2001
From: Josh Triplett 
Date: Fri, 8 Jul 2016 00:19:37 -0700
Subject: [PATCH 1/4] watch: Fix segfault in --color handling

When process_ansi processed a color sequence, it would always read from
an uninitialized pointer, which would sometimes cause a segfault.  Fix
to read from the correct pointer.

Signed-off-by: Josh Triplett 
---
 watch.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/watch.c b/watch.c
index de46730..a75f3c0 100644
--- a/watch.c
+++ b/watch.c
@@ -228,7 +228,7 @@ static void process_ansi(FILE * fp)
 	 * attributes to apply, but typically there are between 1 and 3.
 	 */
 
-	if (*endptr == '\0') set_ansi_attribute(0); /* [m treated as [0m */
+	if (*buf == '\0') set_ansi_attribute(0); /* [m treated as [0m */
 
 	for (endptr = numstart = buf; *endptr != '\0'; numstart = endptr + 1)
 		set_ansi_attribute(strtol(numstart, , 10));
-- 
2.8.1

>From a4013bf67e2f01a59d777619445b0507a9611219 Mon Sep 17 00:00:00 2001
From: Josh Triplett 
Date: Fri, 8 Jul 2016 00:21:55 -0700
Subject: [PATCH 2/4] watch: Don't process additional numbers in unknown ANSI
 color escapes

process_ansi assumed all numbers in a color control sequence correspond
to colors or attributes, which breaks badly if it encounters a
ISO-8613-3 escape