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

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

Eric Sunshine [off-list ref] writes:

quoted

On Thu, Aug 2, 2018 at 1:37 PM Junio C Hamano [off-list ref] wrote:

quoted

Johannes Schindelin [off-list ref] writes:

quoted

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