Re: [PATCH] rebase -i: Fix possibly wrong onto hash in todo

[Taylor Blau <me@ttaylorr.com>]

On Tue, Aug 11, 2020 at 09:10:17PM +0300, Antti Keränen wrote:
> Hi Taylor,
>
> On Tue, Aug 11, 2020 at 11:28:32AM -0400, Taylor Blau wrote:
> > Hi Antti,
> >
> > Thanks for working on this. I have a few thoughts below, but I think
> > that this is on the right track.
> >
> > On Tue, Aug 10, 2020 at 04:13:15PM +0300, Antti Keränen wrote:
> > > 'todo_list_write_to_file' may overwrite the static buffer, originating
> > > from 'find_unique_abbrev', that was used to store the short commit hash
> > > 'c' for "# Rebase a..b onto c" message in the todo editor.
> >
> > It would be great to know the commit that regressed, or if this has
> > always been the case. I'm not sure if you'll have a ton of luck
> > bisecting, since you indicate that this overwrite *may* occur (that
> > makes me think that it doesn't always happen, so your efforts to bisect
> > the change may be noisy).
>
> This was introduced when interactive rebase was refactored. The first
> commit is 94bcad797966b6a3490bc6806d3ee3eed54da9d9. Would you like to
> have this information in the commit message also?

If it's been broken since the start, I think having a reference to
94bcad7979 isn't interesting, but "this behavior has been broken since
its introduction" is.

> The reason I stated it may occur is that the buffer find_unique_abbrev
> returns is valid for 4 more calls. So the rebase must have 4 commits in
> it before this happens.

:-). Interesting detail, too, and probably worth noting in the commit
message.

>
> > > Fix by duplicating the string before usage, so subsequent calls to
> > > 'find_unique_abbrev' or other functions calling 'hash_to_hex_algop_r'
> > > can't overwrite the buffer.
> > >
> > > Found-by: Jussi Keränen <jussike@gmail.com>
> > > Signed-off-by: Antti Keränen <detegr@rbx.email>
> >
> > > ---
> > >  sequencer.c                   |  7 ++++---
> > >  t/t3404-rebase-interactive.sh | 13 +++++++++++++
> > >  2 files changed, 17 insertions(+), 3 deletions(-)
> > >
> > > diff --git a/sequencer.c b/sequencer.c
> > > index fd7701c88a..0679adb639 100644
> > > --- a/sequencer.c
> > > +++ b/sequencer.c
> > > @@ -5178,13 +5178,12 @@ int complete_action(struct repository *r, struct replay_opts *opts, unsigned fla
> > >  		    struct string_list *commands, unsigned autosquash,
> > >  		    struct todo_list *todo_list)
> > >  {
> > > -	const char *shortonto, *todo_file = rebase_path_todo();
> > > +	const char *todo_file = rebase_path_todo();
> > >  	struct todo_list new_todo = TODO_LIST_INIT;
> > >  	struct strbuf *buf = &todo_list->buf, buf2 = STRBUF_INIT;
> > >  	struct object_id oid = onto->object.oid;
> > >  	int res;
> > > -
> > > -	shortonto = find_unique_abbrev(&oid, DEFAULT_ABBREV);
> > > +	char *shortonto;
> >
> > A minor nit is that you could probably move this line up to below the
> > 'const char *' declaration (it makes sense why you have to drop the
> > const qualifier, though).
>
> Ack.

Feel free to discard this suggestion in favor of what Phillip is
proposing, though.

> > > +	shortonto = xstrdup(find_unique_abbrev(&oid, DEFAULT_ABBREV));
> > >  	res = edit_todo_list(r, todo_list, &new_todo, shortrevisions,
> > >  			     shortonto, flags);
> > > +	free(shortonto);
> >
> > OK. I think of two things here:
> >
> >   1. Why are we calling 'find_unique_abbrev()' instead of
> >      'short_commit_name()'? We already have a commit pointer, so I don't
> >      see any reason that we should be reimplementing that function (even
> >      though it is a one-liner).
> >
> >   2. If we should indeed be calling 'short_commit_name()', are there
> >      other callers that need to do the same duplication? In other words:
> >      could you say a little bit more about what makes
> >      'complete_action()' special in this regard?
>
> Good question. The code used find_unique_abbrev and as I'm new to the
> code base I did not notice that short_commit_name essentially does the
> same thing.
>
> The reason what makes this special is that this code first calls
> find_unique_abbrev which, as we know, stores its information to a static
> buffer. The pointer is stored in a variable and it is assumed it does
> not change.
> Afterwards, it calls edit_todo_list that reuses the buffer before it
> accesses the shortonto string, meaning that if we have enough commits in
> the rebase, the shortonto string is overwritten.

Thanks for the detailed analysis. Makes sense.

> Actually, now that you asked, I noticed that orig_head in
> complete_action (actually comes from get_revision_ranges in
> builtin/rebase.c) may be affected as well. Though, I'm not sure whether
> there are enough calls to find_unique_abbrev in the execution path when
> it is being used to actually cause a rewritten buffer. It wouldn't hurt
> to duplicate it too just in case.
>
> --
> Antti

Thanks,
Taylor