Re: [PATCH 54/67] color: add overflow checks for parsing colors
From: Eric Sunshine <hidden>
Date: 2016-06-15 23:06:36
On Tue, Sep 15, 2015 at 12:07 PM, Jeff King [off-list ref] wrote:
quoted hunk ↗ jump to hunk
Our color parsing is designed to never exceed COLOR_MAXLEN bytes. But the relationship between that hand-computed number and the parsing code is not at all obvious, and we merely hope that it has been computed correctly for all cases. Let's mark the expected "end" pointer for the destination buffer and make sure that we do not exceed it. Signed-off-by: Jeff King <redacted> ---@@ -224,12 +227,18 @@ int color_parse_mem(const char *value, int value_len, char *dst) goto bad; } +#define OUT(x) do { \ + if (dst == end) \ + die("BUG: color parsing ran out of space"); \ + *dst++ = (x); \ +} while(0)
Hmm, can we have an #undef OUT before the #define OUT(...), or choose a less conflict-likely name? In particular, I'm thinking about preprocessor namespace pollution arising from sources out of our control, such as was the case with 414382f (ewah/bitmap: silence warning about MASK macro redefinition, 2015-06-03).
quoted hunk ↗ jump to hunk
+ if (attr || !color_empty(&fg) || !color_empty(&bg)) { int sep = 0; int i; - *dst++ = '\033'; - *dst++ = '['; + OUT('\033'); + OUT('['); for (i = 0; attr; i++) { unsigned bit = (1 << i);@@ -237,24 +246,24 @@ int color_parse_mem(const char *value, int value_len, char *dst) continue; attr &= ~bit; if (sep++) - *dst++ = ';'; - dst += sprintf(dst, "%d", i); + OUT(';'); + dst += xsnprintf(dst, end - dst, "%d", i); } if (!color_empty(&fg)) { if (sep++) - *dst++ = ';'; + OUT(';'); /* foreground colors are all in the 3x range */ - dst = color_output(dst, &fg, '3'); + dst = color_output(dst, end - dst, &fg, '3'); } if (!color_empty(&bg)) { if (sep++) - *dst++ = ';'; + OUT(';'); /* background colors are all in the 4x range */ - dst = color_output(dst, &bg, '4'); + dst = color_output(dst, end - dst, &bg, '4'); } - *dst++ = 'm'; + OUT('m'); } - *dst = 0; + OUT(0); return 0; bad: return error(_("invalid color value: %.*s"), value_len, value); --2.6.0.rc2.408.ga2926b9