[PATCH iproute2 v2 0/4] Fix ip segfault when using --color switch

[PATCH iproute2 v2 0/4] Fix ip segfault when using --color switch

[Petr Vorel]

Hi Stephen,

I cleanup code not to use magic offsets. I kept it in separate commits,
as it's clearer what happened.

Petr Vorel (4):
  color: Fix ip segfault when using --color switch
  color: Fix another ip segfault when using --color switch
  color: Cleanup code to remove "magic" offset + 7
  color: Rename enum

 include/color.h      |  3 +--
 include/json_print.h |  2 +-
 lib/color.c          | 17 ++++++++++-------
 3 files changed, 12 insertions(+), 10 deletions(-)

-- 
2.14.2

[PATCH iproute2 v2 1/4] color: Fix ip segfault when using --color switch

[Petr Vorel]

Commit d0e72011 ("ip: ipaddress.c: add support for json output")
introduced passing -1 as enum color_attr. This is not only wrong as no
color_attr has value -1, but also causes another segfault in color_fprintf()
on this setup as there is no item with index -1 in array of enum attr_colors[].
Using COLOR_CLEAR is valid option.

Reproduce with:
$ COLORFGBG='0;15' ip -c a

NOTE: COLORFGBG is environmental variable used for defining whether user
has light or dark background.
COLORFGBG="0;15" is used to ask for color set suitable for light background,
COLORFGBG="15;0" is used to ask for color set suitable for dark background.

Signed-off-by: Petr Vorel <petr.vorel@gmail.com>
---
 include/json_print.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/include/json_print.h b/include/json_print.h
index b6ce1f9f..596af35a 100644
--- a/include/json_print.h
+++ b/include/json_print.h
@@ -53,7 +53,7 @@ void close_json_array(enum output_type type, const char *delim);
 					     const char *fmt,		\
 					     type value)		\
 	{								\
-		print_color_##type_name(t, -1, key, fmt, value);	\
+		print_color_##type_name(t, COLOR_CLEAR, key, fmt, value);	\
 	}
 _PRINT_FUNC(int, int);
 _PRINT_FUNC(bool, bool);
-- 
2.14.2

[PATCH iproute2 v2 2/4] color: Fix another ip segfault when using --color switch

[Petr Vorel]

Commit 959f1428 ("color: add new COLOR_NONE and disable_color function")
introducing color enum COLOR_NONE, which is not only duplicite of
COLOR_CLEAR, but also caused segfault, when running ip with --color
switch, as 'attr + 8' in color_fprintf() access array item out of
bounds. Thus removing it and restoring "magic" offset + 7.

Reproduce with:
$ ip -c a

Signed-off-by: Petr Vorel <petr.vorel@gmail.com>
---
 include/color.h | 1 -
 lib/color.c     | 4 ++--
 2 files changed, 2 insertions(+), 3 deletions(-)

diff --git a/include/color.h b/include/color.h
index 1cd6f7d2..c183ef79 100644
--- a/include/color.h
+++ b/include/color.h
@@ -2,7 +2,6 @@
 #define __COLOR_H__ 1
 
 enum color_attr {
-	COLOR_NONE,
 	COLOR_IFNAME,
 	COLOR_MAC,
 	COLOR_INET,
diff --git a/lib/color.c b/lib/color.c
index 79d5e289..05afcb21 100644
--- a/lib/color.c
+++ b/lib/color.c
@@ -104,13 +104,13 @@ int color_fprintf(FILE *fp, enum color_attr attr, const char *fmt, ...)
 
 	va_start(args, fmt);
 
-	if (!color_is_enabled || attr == COLOR_NONE) {
+	if (!color_is_enabled || attr == COLOR_CLEAR) {
 		ret = vfprintf(fp, fmt, args);
 		goto end;
 	}
 
 	ret += fprintf(fp, "%s",
-		       color_codes[attr_colors[is_dark_bg ? attr + 8 : attr]]);
+		       color_codes[attr_colors[is_dark_bg ? attr + 7 : attr]]);
 	ret += vfprintf(fp, fmt, args);
 	ret += fprintf(fp, "%s", color_codes[C_CLEAR]);
 
-- 
2.14.2

[PATCH iproute2 v2 3/4] color: Cleanup code to remove "magic" offset + 7

[Petr Vorel]

Signed-off-by: Petr Vorel <petr.vorel@gmail.com>
---
 lib/color.c | 13 ++++++++-----
 1 file changed, 8 insertions(+), 5 deletions(-)

diff --git a/lib/color.c b/lib/color.c
index 05afcb21..497f5e1b 100644
--- a/lib/color.c
+++ b/lib/color.c
@@ -45,8 +45,8 @@ static const char * const color_codes[] = {
 	NULL,
 };
 
-static enum color attr_colors[] = {
-	/* light background */
+/* light background */
+static enum color attr_colors_light[] = {
 	C_CYAN,
 	C_YELLOW,
 	C_MAGENTA,
@@ -54,8 +54,10 @@ static enum color attr_colors[] = {
 	C_GREEN,
 	C_RED,
 	C_CLEAR,
+};
 
-	/* dark background */
+/* dark background */
+static enum color attr_colors_dark[] = {
 	C_BOLD_CYAN,
 	C_BOLD_YELLOW,
 	C_BOLD_MAGENTA,
@@ -109,8 +111,9 @@ int color_fprintf(FILE *fp, enum color_attr attr, const char *fmt, ...)
 		goto end;
 	}
 
-	ret += fprintf(fp, "%s",
-		       color_codes[attr_colors[is_dark_bg ? attr + 7 : attr]]);
+	ret += fprintf(fp, "%s", color_codes[is_dark_bg ?
+		attr_colors_dark[attr] : attr_colors_light[attr]]);
+
 	ret += vfprintf(fp, fmt, args);
 	ret += fprintf(fp, "%s", color_codes[C_CLEAR]);
 
-- 
2.14.2

[PATCH iproute2 v2 4/4] color: Rename enum

[Petr Vorel]

COLOR_NONE is more descriptive than COLOR_CLEAR.

Signed-off-by: Petr Vorel <petr.vorel@gmail.com>
---
 include/color.h      | 2 +-
 include/json_print.h | 2 +-
 lib/color.c          | 6 +++---
 3 files changed, 5 insertions(+), 5 deletions(-)

diff --git a/include/color.h b/include/color.h
index c183ef79..7fd685d0 100644
--- a/include/color.h
+++ b/include/color.h
@@ -8,7 +8,7 @@ enum color_attr {
 	COLOR_INET6,
 	COLOR_OPERSTATE_UP,
 	COLOR_OPERSTATE_DOWN,
-	COLOR_CLEAR
+	COLOR_NONE
 };
 
 void enable_color(void);
diff --git a/include/json_print.h b/include/json_print.h
index 596af35a..dc4d2bb3 100644
--- a/include/json_print.h
+++ b/include/json_print.h
@@ -53,7 +53,7 @@ void close_json_array(enum output_type type, const char *delim);
 					     const char *fmt,		\
 					     type value)		\
 	{								\
-		print_color_##type_name(t, COLOR_CLEAR, key, fmt, value);	\
+		print_color_##type_name(t, COLOR_NONE, key, fmt, value);	\
 	}
 _PRINT_FUNC(int, int);
 _PRINT_FUNC(bool, bool);
diff --git a/lib/color.c b/lib/color.c
index 497f5e1b..8d049a01 100644
--- a/lib/color.c
+++ b/lib/color.c
@@ -106,7 +106,7 @@ int color_fprintf(FILE *fp, enum color_attr attr, const char *fmt, ...)
 
 	va_start(args, fmt);
 
-	if (!color_is_enabled || attr == COLOR_CLEAR) {
+	if (!color_is_enabled || attr == COLOR_NONE) {
 		ret = vfprintf(fp, fmt, args);
 		goto end;
 	}
@@ -130,7 +130,7 @@ enum color_attr ifa_family_color(__u8 ifa_family)
 	case AF_INET6:
 		return COLOR_INET6;
 	default:
-		return COLOR_CLEAR;
+		return COLOR_NONE;
 	}
 }
 
@@ -142,6 +142,6 @@ enum color_attr oper_state_color(__u8 state)
 	case IF_OPER_DOWN:
 		return COLOR_OPERSTATE_DOWN;
 	default:
-		return COLOR_CLEAR;
+		return COLOR_NONE;
 	}
 }
-- 
2.14.2

Re: [PATCH iproute2 v2 0/4] Fix ip segfault when using --color switch

[Stephen Hemminger]

On Fri, 13 Oct 2017 15:57:15 +0200
Petr Vorel <petr.vorel@gmail.com> wrote:

> Hi Stephen,
> 
> I cleanup code not to use magic offsets. I kept it in separate commits,
> as it's clearer what happened.
> 
> Petr Vorel (4):
>   color: Fix ip segfault when using --color switch
>   color: Fix another ip segfault when using --color switch
>   color: Cleanup code to remove "magic" offset + 7
>   color: Rename enum
> 
>  include/color.h      |  3 +--
>  include/json_print.h |  2 +-
>  lib/color.c          | 17 ++++++++++-------
>  3 files changed, 12 insertions(+), 10 deletions(-)
> 

Applied, Thans Petr