* [PATCH] color: protect against out-of-bounds array access/assignment
@ 2018-08-02 9:32 Eric Sunshine
2018-08-02 12:38 ` Johannes Schindelin
2018-08-03 6:07 ` Jonathan Nieder
0 siblings, 2 replies; 8+ messages in thread
From: Eric Sunshine @ 2018-08-02 9:32 UTC (permalink / raw)
To: git; +Cc: Johannes Schindelin, Eric Sunshine
want_color_fd() is designed to work only with standard input, 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/assignment outside the array bounds.
Signed-off-by: Eric Sunshine <sunshine@sunshineco.com>
---
Just something I noticed while studying this code in relation to a patch
review.
color.c | 3 +++
1 file changed, 3 insertions(+)
diff --git a/color.c b/color.c
index b1c24c69de..b0be9ce505 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 < 0 || fd >= ARRAY_SIZE(want_auto))
+ BUG("file descriptor out of range: %d", fd);
+
if (var < 0)
var = git_use_color_default;
--
2.18.0.599.g4ce2a8faa4.dirty
^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [PATCH] color: protect against out-of-bounds array access/assignment
2018-08-02 9:32 [PATCH] color: protect against out-of-bounds array access/assignment Eric Sunshine
@ 2018-08-02 12:38 ` Johannes Schindelin
2018-08-02 17:36 ` Junio C Hamano
2018-08-03 6:07 ` Jonathan Nieder
1 sibling, 1 reply; 8+ messages in thread
From: Johannes Schindelin @ 2018-08-02 12:38 UTC (permalink / raw)
To: Eric Sunshine; +Cc: git
Hi Eric,
On Thu, 2 Aug 2018, Eric Sunshine wrote:
> want_color_fd() is designed to work only with standard input, 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/assignment outside the array bounds.
ACK!
Thanks,
Dscho
>
> Signed-off-by: Eric Sunshine <sunshine@sunshineco.com>
> ---
>
> Just something I noticed while studying this code in relation to a patch
> review.
>
> color.c | 3 +++
> 1 file changed, 3 insertions(+)
>
> diff --git a/color.c b/color.c
> index b1c24c69de..b0be9ce505 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 < 0 || fd >= ARRAY_SIZE(want_auto))
> + BUG("file descriptor out of range: %d", fd);
> +
> if (var < 0)
> var = git_use_color_default;
>
> --
> 2.18.0.599.g4ce2a8faa4.dirty
>
>
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] color: protect against out-of-bounds array access/assignment
2018-08-02 12:38 ` Johannes Schindelin
@ 2018-08-02 17:36 ` Junio C Hamano
2018-08-02 17:45 ` Eric Sunshine
0 siblings, 1 reply; 8+ messages in thread
From: Junio C Hamano @ 2018-08-02 17:36 UTC (permalink / raw)
To: Johannes Schindelin; +Cc: Eric Sunshine, git
Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
> Hi Eric,
>
> On Thu, 2 Aug 2018, Eric Sunshine wrote:
>
>> want_color_fd() is designed to work only with standard input, 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/assignment outside the array bounds.
>
> ACK!
>
> Thanks,
> Dscho
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).
In any case, this looks like a good defensive measure.
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] color: protect against out-of-bounds array access/assignment
2018-08-02 17:36 ` Junio C Hamano
@ 2018-08-02 17:45 ` Eric Sunshine
2018-08-02 19:24 ` Junio C Hamano
0 siblings, 1 reply; 8+ messages in thread
From: Eric Sunshine @ 2018-08-02 17:45 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Johannes Schindelin, Git List
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)
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] color: protect against out-of-bounds array access/assignment
2018-08-02 17:45 ` Eric Sunshine
@ 2018-08-02 19:24 ` Junio C Hamano
2018-08-02 19:30 ` Jeff King
0 siblings, 1 reply; 8+ messages in thread
From: Junio C Hamano @ 2018-08-02 19:24 UTC (permalink / raw)
To: Eric Sunshine; +Cc: Johannes Schindelin, Git List
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.
Come to think of it, would want_color_fd(0, var) ever make sense?
In any case, thanks for unconfusing me.
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] color: protect against out-of-bounds array access/assignment
2018-08-02 19:24 ` Junio C Hamano
@ 2018-08-02 19:30 ` Jeff King
0 siblings, 0 replies; 8+ messages in thread
From: Jeff King @ 2018-08-02 19:30 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Eric Sunshine, Johannes Schindelin, Git List
On Thu, Aug 02, 2018 at 12:24:54PM -0700, Junio C Hamano wrote:
> 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
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] color: protect against out-of-bounds array access/assignment
2018-08-02 9:32 [PATCH] color: protect against out-of-bounds array access/assignment Eric Sunshine
2018-08-02 12:38 ` Johannes Schindelin
@ 2018-08-03 6:07 ` Jonathan Nieder
2018-08-03 6:43 ` Eric Sunshine
1 sibling, 1 reply; 8+ messages in thread
From: Jonathan Nieder @ 2018-08-03 6:07 UTC (permalink / raw)
To: Eric Sunshine; +Cc: git, Johannes Schindelin, Junio C Hamano, Jeff King
Hi,
Eric Sunshine wrote:
> want_color_fd() is designed to work only with standard input, 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/assignment outside the array bounds.
>
> Signed-off-by: Eric Sunshine <sunshine@sunshineco.com>
> ---
> color.c | 3 +++
> 1 file changed, 3 insertions(+)
>
> diff --git a/color.c b/color.c
> index b1c24c69de..b0be9ce505 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 < 0 || fd >= ARRAY_SIZE(want_auto))
> + BUG("file descriptor out of range: %d", fd);
The indentation looks wrong here.
Combining that with the other suggestions from this thread, I end up
with this v2:
-- >8 --
From: Eric Sunshine <sunshine@sunshineco.com>
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 <sunshine@sunshineco.com>
Acked-by: Johannes Schindelin <Johannes.Schindelin@gmx.de>
Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>
---
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
^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [PATCH] color: protect against out-of-bounds array access/assignment
2018-08-03 6:07 ` Jonathan Nieder
@ 2018-08-03 6:43 ` Eric Sunshine
0 siblings, 0 replies; 8+ messages in thread
From: Eric Sunshine @ 2018-08-03 6:43 UTC (permalink / raw)
To: Jonathan Nieder; +Cc: Git List, Johannes Schindelin, Junio C Hamano, Jeff King
On Fri, Aug 3, 2018 at 2:26 AM Jonathan Nieder <jrnieder@gmail.com> 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 <sunshine@sunshineco.com>
(patch left unsnipped)
> -- >8 --
> From: Eric Sunshine <sunshine@sunshineco.com>
> 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 <sunshine@sunshineco.com>
> Acked-by: Johannes Schindelin <Johannes.Schindelin@gmx.de>
> Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>
> ---
> 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
^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2018-08-03 6:43 UTC | newest]
Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-08-02 9:32 [PATCH] color: protect against out-of-bounds array access/assignment Eric Sunshine
2018-08-02 12:38 ` Johannes Schindelin
2018-08-02 17:36 ` Junio C Hamano
2018-08-02 17:45 ` Eric Sunshine
2018-08-02 19:24 ` Junio C Hamano
2018-08-02 19:30 ` Jeff King
2018-08-03 6:07 ` Jonathan Nieder
2018-08-03 6:43 ` Eric Sunshine
Code repositories for project(s) associated with this public inbox
https://80x24.org/mirrors/git.git
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).