From: Eric Sunshine Date: Fri, 03 Aug 2018 06:43:17 GMT Subject: Re: [PATCH] color: protect against out-of-bounds array access/assignment Message-ID: In-Reply-To: <20180803060749.GA237521@aiede.svl.corp.google.com> On Fri, Aug 3, 2018 at 2:26 AM Jonathan Nieder wrote: > Eric Sunshine wrote: > > + if (fd < 0 || fd >= ARRAY_SIZE(want_auto)) > > + BUG("file descriptor out of range: %d", fd); > > The indentation looks wrong here. Yep, that's weird. I can't figure out how that got indented with four spaces rather than a TAB. I just re-typed the entire change in my editor as I believe I typed it earlier, and the editor correctly auto-indented it with a TAB. Odd. Thanks for catching it. > Combining that with the other suggestions from this thread, I end up > with this v2: This looks good to me. Thanks. And, if needed: Signed-off-by: Eric Sunshine (patch left unsnipped) > -- >8 -- > From: Eric Sunshine > Subject: color: protect against out-of-bounds reads and writes > > want_color_fd() is designed to work only with standard output and > error file descriptors and stores information about each descriptor in > an array. However, it doesn't verify that the passed-in descriptor > lives within that set, which, with a buggy caller, could lead to > access or assignment outside the array bounds. > > Signed-off-by: Eric Sunshine > Acked-by: Johannes Schindelin > Signed-off-by: Jonathan Nieder > --- > color.c | 3 +++ > 1 file changed, 3 insertions(+) > > diff --git a/color.c b/color.c > index b1c24c69de..ebb222ec33 100644 > --- a/color.c > +++ b/color.c > @@ -343,6 +343,9 @@ int want_color_fd(int fd, int var) > > static int want_auto[3] = { -1, -1, -1 }; > > + if (fd < 1 || fd >= ARRAY_SIZE(want_auto)) > + BUG("file descriptor out of range: %d", fd); > + > if (var < 0) > var = git_use_color_default; > > -- > 2.18.0.597.ga71716f1ad