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 "<ESC>[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, &endptr, 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 <j...@joshtriplett.org>
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 <j...@joshtriplett.org>
---
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, &endptr, 10));
--
2.8.1
>From a4013bf67e2f01a59d777619445b0507a9611219 Mon Sep 17 00:00:00 2001
From: Josh Triplett <j...@joshtriplett.org>
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 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.
Stop processing the entire escape sequence if watch encounters any
number it doesn't understand, as that number may change the meaning of
the rest of the sequence.
Signed-off-by: Josh Triplett <j...@joshtriplett.org>
---
watch.c | 8 ++++++--
1 file changed, 6 insertions(+), 2 deletions(-)
diff --git a/watch.c b/watch.c
index a75f3c0..118cb8b 100644
--- a/watch.c
+++ b/watch.c
@@ -136,7 +136,7 @@ static void init_ansi_colors(void)
}
-static void set_ansi_attribute(const int attrib)
+static int set_ansi_attribute(const int attrib)
{
switch (attrib) {
case -1: /* restore last settings */
@@ -189,10 +189,13 @@ static void set_ansi_attribute(const int attrib)
fg_col = attrib - 30 + 1;
} else if (attrib >= 40 && attrib <= 47) { /* set background color */
bg_col = attrib - 40 + 1;
+ } else {
+ return 0; /* Not understood */
}
}
attrset(attributes | COLOR_PAIR(bg_col * nr_of_colors + fg_col + 1));
+ return 1;
}
static void process_ansi(FILE * fp)
@@ -231,7 +234,8 @@ static void process_ansi(FILE * fp)
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, &endptr, 10));
+ if (!set_ansi_attribute(strtol(numstart, &endptr, 10)))
+ break;
}
static void __attribute__ ((__noreturn__)) do_exit(int status)
--
2.8.1
>From 4590116cca8c0200680f4a7cdefb9443c3c28eb6 Mon Sep 17 00:00:00 2001
From: Josh Triplett <j...@joshtriplett.org>
Date: Fri, 8 Jul 2016 00:29:59 -0700
Subject: [PATCH 3/4] watch: Fix ANSI escape sequence termination
process_ansi stopped processing an ANSI escape sequence if
(c < '0' && c > '9' && c != ';'), which will never happen. Fix the
range check to use || instead.
Signed-off-by: Josh Triplett <j...@joshtriplett.org>
---
watch.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/watch.c b/watch.c
index 118cb8b..56b5b29 100644
--- a/watch.c
+++ b/watch.c
@@ -216,7 +216,7 @@ static void process_ansi(FILE * fp)
buf[i] = '\0';
break;
}
- if (c < '0' && c > '9' && c != ';') {
+ if ((c < '0' || c > '9') && c != ';') {
while (--i >= 0)
ungetc(buf[i], fp);
return;
--
2.8.1
>From 597aff834ef7ff59a5b760b6461002f910798cf3 Mon Sep 17 00:00:00 2001
From: Josh Triplett <j...@joshtriplett.org>
Date: Fri, 8 Jul 2016 00:32:59 -0700
Subject: [PATCH 4/4] watch: Don't attempt to ungetc parts of unknown ANSI
escape sequences
If process_ansi encountered an unknown character when processing an ANSI
escape sequence, it would ungetc all the characters read so far, except
for the character just read, and the opening '\033['. ungetting the
middle of the escape sequence does not produce useful results, and also
relies on the unportable assumption that ungetc works on multiple
characters (which glibc does not support). Discard the characters
instead.
Signed-off-by: Josh Triplett <j...@joshtriplett.org>
---
watch.c | 2 --
1 file changed, 2 deletions(-)
diff --git a/watch.c b/watch.c
index 56b5b29..8978dd7 100644
--- a/watch.c
+++ b/watch.c
@@ -217,8 +217,6 @@ static void process_ansi(FILE * fp)
break;
}
if ((c < '0' || c > '9') && c != ';') {
- while (--i >= 0)
- ungetc(buf[i], fp);
return;
}
buf[i] = (char)c;
--
2.8.1
