Re: [PATCH] read-cache: avoid git_path() race in freshen_shared_index()
Christian Couder <christian.couder@gmail.com> writes:
> When performing an interactive rebase in split-index mode,
> the commit message that one should rework when squashing commits
> can contain some garbage instead of the usual concatenation of
> both of the commit messages.
OK, that is an understandable explanation of what problem you are trying to fix.
Show 8 quoted lines
>
> When bisecting it appears that 94c9b5af70 (Merge branch
> 'cc/split-index-config', 2017-03-17) is the first bad commit.
>
> But when rebasing cc/split-index-config on top of the commit it
> was merged with, the first bad commit is then c3a0082502
> (read-cache: use freshen_shared_index() in read_index_from(),
> 2017-03-06).
This part however doesn't help understanding the issue. "When X but when Y" sounds as if you found a botched merge, but that does not seem to be the case. The resulting tree after rebasing (with conflict resolution) is the same as the recorded merge result. It could be saying that "git bisect" is buggy and does not pinpoint the broken commit, but this is not a commit to fix "bisect".
That leaves the reader confused.
> This shows that we should be careful not to use git_path() in
> freshen_shared_index(). It is using a shared buffer that can
> too easily lead to races.
The impression I get from the symptom is that after git_path() is called here, before check_and_freshen_file() uses that result, it (or functions it calls) uses git_path(), and the number of times it does so has changed since cc/split-index-config was written on the mainline, and the rotating 4-element buffer get_pathname() gives is now exhausted, leading to the failure you observed. By the way, that does not sound a race to me.
In any case, that explains why bisect says the merge is the first bad one, and cures the confused reader ;-) The use of git_path() on the topic was still safe; it was a timebomb waiting to go off. The mainline started using more calls and the merge result was unsafe.
If you meant to summarise the whole two paragraphs above that I needed to think it through with "This shows that", I'd have to say that you are expecting too much from your readers. Please be a bit more gentle to them.
Thanks.
Show 21 quoted lines
> Signed-off-by: Christian Couder <chriscool@tuxfamily.org>
> ---
> read-cache.c | 3 ++-
> 1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/read-cache.c b/read-cache.c
> index e447751823..2f10242c24 100644
> --- a/read-cache.c
> +++ b/read-cache.c
> @@ -1682,9 +1682,10 @@ int do_read_index(struct index_state *istate, const char *path, int must_exist)
> */
> static void freshen_shared_index(char *base_sha1_hex, int warn)
> {
> - const char *shared_index = git_path("sharedindex.%s", base_sha1_hex);
> + char *shared_index = git_pathdup("sharedindex.%s", base_sha1_hex);
> if (!check_and_freshen_file(shared_index, 1) && warn)
> warning("could not freshen shared index '%s'", shared_index);
> + free(shared_index);
> }
>
> int read_index_from(struct index_state *istate, const char *path)
Christian Couder
