Quantcast

[BUG] A part of an edge from an octopus merge gets colored, even with --color=never

classic Classic list List threaded Threaded
8 messages Options
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

[BUG] A part of an edge from an octopus merge gets colored, even with --color=never

Noam Postavsky
With a certain topology involving an octopus merge, git log --graph
--oneline --all --color=never produces output which includes some ANSI
escape code coloring. Attached is a script to reproduce the problem
(creates a git repository in subdir log-format-test), along with
sample graph and valgrind output (indicates some unitialialized memory
access).

I've observed the problem with Windows git versions 2.7.0, 2.5.3.
I've NOT observed it with 1.9.5,

On GNU/Linux the symptom only appears when running with valgrind, I
tried versions
2.8.0, and 2.8.2.402.gedec370 (the last is where the valgrind output comes from)

graph.log (196 bytes) Download Attachment
test-multiway-merge.sh (902 bytes) Download Attachment
valgrind.log (7K) Download Attachment
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: [BUG] A part of an edge from an octopus merge gets colored, even with --color=never

Johannes Sixt-3
Am 15.05.2016 um 15:05 schrieb Noam Postavsky:

> With a certain topology involving an octopus merge, git log --graph
> --oneline --all --color=never produces output which includes some ANSI
> escape code coloring. Attached is a script to reproduce the problem
> (creates a git repository in subdir log-format-test), along with
> sample graph and valgrind output (indicates some unitialialized memory
> access).
>
> I've observed the problem with Windows git versions 2.7.0, 2.5.3.
> I've NOT observed it with 1.9.5,
>
> On GNU/Linux the symptom only appears when running with valgrind, I
> tried versions
> 2.8.0, and 2.8.2.402.gedec370 (the last is where the valgrind output comes from)
>

Sorry, I can't reproduce your observation. I ran the script you provided
with HOME=$PWD and a minimal .gitconfig that only sets user.email. But
valgrind is happy with both 2.8.0 and v2.8.2-402-gedec370 on my Linux box.

-- Hannes

--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to [hidden email]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: [BUG] A part of an edge from an octopus merge gets colored, even with --color=never

Jeff King
On Tue, May 17, 2016 at 09:07:33PM +0200, Johannes Sixt wrote:

> Am 15.05.2016 um 15:05 schrieb Noam Postavsky:
> > With a certain topology involving an octopus merge, git log --graph
> > --oneline --all --color=never produces output which includes some ANSI
> > escape code coloring. Attached is a script to reproduce the problem
> > (creates a git repository in subdir log-format-test), along with
> > sample graph and valgrind output (indicates some unitialialized memory
> > access).
> >
> > I've observed the problem with Windows git versions 2.7.0, 2.5.3.
> > I've NOT observed it with 1.9.5,
> >
> > On GNU/Linux the symptom only appears when running with valgrind, I
> > tried versions
> > 2.8.0, and 2.8.2.402.gedec370 (the last is where the valgrind output comes from)
> >
>
> Sorry, I can't reproduce your observation. I ran the script you provided
> with HOME=$PWD and a minimal .gitconfig that only sets user.email. But
> valgrind is happy with both 2.8.0 and v2.8.2-402-gedec370 on my Linux box.

Interesting. It replicates out of the box for me. It looks like the
column pointer we are passing is bogus. If I instrument git like this:

diff --git a/graph.c b/graph.c
index 1350bdd..62a5810 100644
--- a/graph.c
+++ b/graph.c
@@ -76,6 +76,7 @@ static const char *column_get_color_code(unsigned short color)
 static void strbuf_write_column(struct strbuf *sb, const struct column *c,
  char col_char)
 {
+ warning("c=%p, c->color = %d, max=%d", c, c->color, column_colors_max);
  if (c->color < column_colors_max)
  strbuf_addstr(sb, column_get_color_code(c->color));
  strbuf_addch(sb, col_char);
@@ -390,6 +391,9 @@ static void graph_insert_into_new_columns(struct git_graph *graph,
  */
  graph->new_columns[graph->num_new_columns].commit = commit;
  graph->new_columns[graph->num_new_columns].color = graph_find_commit_color(graph, commit);
+ warning("assigned %p, %d",
+ &graph->new_columns[graph->num_new_columns],
+ graph->new_columns[graph->num_new_columns].color);
  graph->mapping[*mapping_index] = graph->num_new_columns;
  *mapping_index += 2;
  graph->num_new_columns++;

Then I get this output:

    warning: assigned 0x20d21d0, 12
    * 163b784 (c) c
    warning: assigned 0x20d8f90, 12
    warning: assigned 0x20d8fa0, 12
    warning: assigned 0x20d8fb0, 12
    warning: c=0x20d21d0, c->color = 12, max=12
    warning: c=0x20d8fc0, c->color = 0, max=12
    warning: c=0x20d8fc0, c->color = 0, max=12
    | *-.   a9a6975 (HEAD -> m) merge a b

Note that we set up f90, fa0, and fb0, but then pass fc0 into
strbuf_write_column (and it has bogus color values). It looks like we're
reading one past the end of our array, but I haven't figured out where
or why.

-Peff
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to [hidden email]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: [BUG] A part of an edge from an octopus merge gets colored, even with --color=never

Jeff King
On Tue, May 17, 2016 at 03:45:34PM -0400, Jeff King wrote:

> Note that we set up f90, fa0, and fb0, but then pass fc0 into
> strbuf_write_column (and it has bogus color values). It looks like we're
> reading one past the end of our array, but I haven't figured out where
> or why.

Looking at the valgrind output reveals that. Here's an assert() that
catches it reliably for me:

diff --git a/graph.c b/graph.c
index 1350bdd..964bbd1 100644
--- a/graph.c
+++ b/graph.c
@@ -794,9 +794,11 @@ static int graph_draw_octopus_merge(struct git_graph *graph,
  ((graph->num_parents - dashless_commits) * 2) - 1;
  for (i = 0; i < num_dashes; i++) {
  col_num = (i / 2) + dashless_commits + graph->commit_index;
+ assert(col_num < graph->num_new_columns);
  strbuf_write_column(sb, &graph->new_columns[col_num], '-');
  }
  col_num = (i / 2) + dashless_commits + graph->commit_index;
+ assert(col_num < graph->num_new_columns);
  strbuf_write_column(sb, &graph->new_columns[col_num], '.');
  return num_dashes + 1;
 }

(It's actually the first one which triggers). I'm not familiar enough
with the code to know whether the col_num computation is bogus, or
whether we needed to earlier increase the size of the "new_columns"
field.

-Peff
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to [hidden email]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: [BUG] A part of an edge from an octopus merge gets colored, even with --color=never

Jeff King
On Tue, May 17, 2016 at 03:51:37PM -0400, Jeff King wrote:

> On Tue, May 17, 2016 at 03:45:34PM -0400, Jeff King wrote:
>
> > Note that we set up f90, fa0, and fb0, but then pass fc0 into
> > strbuf_write_column (and it has bogus color values). It looks like we're
> > reading one past the end of our array, but I haven't figured out where
> > or why.
>
> Looking at the valgrind output reveals that. Here's an assert() that
> catches it reliably for me:
>
> diff --git a/graph.c b/graph.c
> index 1350bdd..964bbd1 100644
> --- a/graph.c
> +++ b/graph.c
> @@ -794,9 +794,11 @@ static int graph_draw_octopus_merge(struct git_graph *graph,
>   ((graph->num_parents - dashless_commits) * 2) - 1;
>   for (i = 0; i < num_dashes; i++) {
>   col_num = (i / 2) + dashless_commits + graph->commit_index;
> + assert(col_num < graph->num_new_columns);
>   strbuf_write_column(sb, &graph->new_columns[col_num], '-');
>   }
>   col_num = (i / 2) + dashless_commits + graph->commit_index;
> + assert(col_num < graph->num_new_columns);
>   strbuf_write_column(sb, &graph->new_columns[col_num], '.');
>   return num_dashes + 1;
>  }
>
> (It's actually the first one which triggers). I'm not familiar enough
> with the code to know whether the col_num computation is bogus, or
> whether we needed to earlier increase the size of the "new_columns"
> field.

And unsurprisingly, reverting 339c17bc7690b5436ac61c996cede3d52c85b50d
seems to fix this (author cc'd). It's the extra "commit_index" addition
that causes the problem. But I'm still not sure what the correct
solution is.

-Peff
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to [hidden email]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: [BUG] A part of an edge from an octopus merge gets colored, even with --color=never

Johannes Sixt-3
In reply to this post by Jeff King
Am 17.05.2016 um 21:45 schrieb Jeff King:

> On Tue, May 17, 2016 at 09:07:33PM +0200, Johannes Sixt wrote:
>
>> Am 15.05.2016 um 15:05 schrieb Noam Postavsky:
>>> With a certain topology involving an octopus merge, git log --graph
>>> --oneline --all --color=never produces output which includes some ANSI
>>> escape code coloring. Attached is a script to reproduce the problem
>>> (creates a git repository in subdir log-format-test), along with
>>> sample graph and valgrind output (indicates some unitialialized memory
>>> access).
>>>
>>> I've observed the problem with Windows git versions 2.7.0, 2.5.3.
>>> I've NOT observed it with 1.9.5,
>>>
>>> On GNU/Linux the symptom only appears when running with valgrind, I
>>> tried versions
>>> 2.8.0, and 2.8.2.402.gedec370 (the last is where the valgrind output comes from)
>>>
>>
>> Sorry, I can't reproduce your observation. I ran the script you provided
>> with HOME=$PWD and a minimal .gitconfig that only sets user.email. But
>> valgrind is happy with both 2.8.0 and v2.8.2-402-gedec370 on my Linux box.
>
> Interesting. It replicates out of the box for me.

"Out of the box" are the magic words. I usually compile with -O0, which
doesn't trigger the valgrind report.

When I compile with a 3.x based gcc on Windows, I see these warnings:

     CC color.o
color.c: In function 'color_parse_mem':
color.c:203: warning: 'c.value' may be used uninitialized in this function
color.c:203: warning: 'c.blue' may be used uninitialized in this function
color.c:203: warning: 'c.green' may be used uninitialized in this function
color.c:203: warning: 'c.red' may be used uninitialized in this function

(which triggered my curiosity in this bug report). But they seem to be
unrelated and are most likely false positives.

-- Hannes

--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to [hidden email]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: [BUG] A part of an edge from an octopus merge gets colored, even with --color=never

Jeff King
On Tue, May 17, 2016 at 10:02:22PM +0200, Johannes Sixt wrote:

> > Interesting. It replicates out of the box for me.
>
> "Out of the box" are the magic words. I usually compile with -O0, which
> doesn't trigger the valgrind report.

Heh, I meant that Noam's test worked out of the box. I also build with
-O0. I was able to replicate with different optimization levels.

I think the interesting thing here is actually the libc (and therefore
possibly your valgrind version). I tried compiling with ASAN and get a
color value of "48830". But no ASAN warning!

I think what is happening is that we over-allocate the new_columns array
based on a power of 2, but only initialize up to num_new_columns. So the
off-by-one accesses heap memory that is allocated but which we have
never written to.

> When I compile with a 3.x based gcc on Windows, I see these warnings:
>
>     CC color.o
> color.c: In function 'color_parse_mem':
> color.c:203: warning: 'c.value' may be used uninitialized in this function
> color.c:203: warning: 'c.blue' may be used uninitialized in this function
> color.c:203: warning: 'c.green' may be used uninitialized in this function
> color.c:203: warning: 'c.red' may be used uninitialized in this function
>
> (which triggered my curiosity in this bug report). But they seem to be
> unrelated and are most likely false positives.

Yeah, I think that's unrelated. I'd be highly distrustful of
-Wuninitialized in gcc 3.x. We had to mark quite a few false positives
back then, that were later corrected in the 4.x series.

-Peff
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to [hidden email]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: [BUG] A part of an edge from an octopus merge gets colored, even with --color=never

Noam Postavsky
In reply to this post by Jeff King
On Tue, May 17, 2016 at 3:55 PM, Jeff King <[hidden email]> wrote:
>> (It's actually the first one which triggers). I'm not familiar enough
>> with the code to know whether the col_num computation is bogus, or
>> whether we needed to earlier increase the size of the "new_columns"
>> field.
>
> And unsurprisingly, reverting 339c17bc7690b5436ac61c996cede3d52c85b50d
> seems to fix this (author cc'd). It's the extra "commit_index" addition
> that causes the problem. But I'm still not sure what the correct
> solution is.

Looking at the coloured output, for some octopus merges where the
first parent edge immediately merges into the next column to the left,
col_num should be decremented by 1 (otherwise the colour of the "-."
doesn't match the rest of that edge).

| | *-.
| | |\ \
| |/ / /

For the other case where the first parent edge stays straight, the
current col_num computation is correct.

| *-.
| |\ \
| | | *

I'm not sure how to distinguish these cases in the code though. Is it
enough to just compare against graph->num_new_columns?
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to [hidden email]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Loading...