Re: [PATCH 54/67] color: add overflow checks for parsing colors
- From
Eric Sunshine <sunshine@sunshineco.com>
- Date
- Sep 18, 2015, 18:54 UTC
- Message-ID
- <CAPig+cQUwsDfL-epJYy+4xoG2-GfDfhb3mH=7WAzYbsea-sXDA@mail.gmail.com>
- In-Reply-To
- <20150915160703.GB29753@sigill.intra.peff.net>
On Tue, Sep 15, 2015 at 12:07 PM, Jeff King <peff@peff.net> wrote:
Show 20 quoted lines
> 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 <peff@peff.net>
> ---
> @@ -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).
Show 47 quoted lines
> +
> 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