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@xxxxxxxxx>
> > > 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
