Re: [PATCH] color: protect against out-of-bounds array access/assignment

From: Jeff King <peff@peff.net>
Date: Aug 2, 2018, 19:30 UTC
Message-ID: <20180802193038.GB28962@sigill.intra.peff.net>
In-Reply-To: <xmqq8t5o1shl.fsf@gitster-ct.c.googlers.com>

On Thu, Aug 02, 2018 at 12:24:54PM -0700, Junio C Hamano wrote:

Show 19 quoted lines

> Eric Sunshine <sunshine@sunshineco.com> writes:
> 
> > On Thu, Aug 2, 2018 at 1:37 PM Junio C Hamano <gitster@pobox.com> wrote:
> >> Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
> >> > ACK!
> >>
> >> Did you write a buggy caller that would have been caught or helped
> >> with this change?  You did not write the callee that is made more
> >> defensive with this patch, so I am being curious as to where that
> >> Ack is coming from (I wouldn't have felt curious if this were
> >> a reviewed-by instead).
> >
> > The code being made more defensive with this patch was authored by Dscho[1].
> >
> > [1]: 295d949cfa (color: introduce support for colorizing stderr, 2018-04-21)
> 
> Ah, OK.  The original by Peff done long time ago didn't check three
> fds separately, but just did a single check_auto_color() implicitly
> only for the standard output.

Right. Technically Eric's check could go into the "if (...AUTO)" conditional, since that was what was touched in 295d949cfa. But I doubt it matters for performance (and if it did, we should probably be coalescing all of these conditionals into a single cached int flag).

> Come to think of it, would want_color_fd(0, var) ever make sense?

No, it's just there because of 0-indexing. The BUG() check could actually be "if (fd < 1 || ...)" to cover that (it "works", but it is nonsensical).

Or we could even use "fd - 1" as the index. But it is probably not worth the effort.

-Peff