Web lists-archives.com

Re: [PATCH] fetch-pack: clear alternate shallow when complete

On Mon, Feb 4, 2019 at 7:06 AM brian m. carlson
<sandals@xxxxxxxxxxxxxxxxxxxx> wrote:
> When we write an alternate shallow file in update_shallow, we write it
> into the lock file. The string stored in alternate_shallow_file is
> copied from the lock file path, but it is freed the moment that the lock
> file is closed, since we call strbuf_release to free that path.
> This used to work, since we did not invoke git index-pack more than
> once, but now that we do, we reuse the freed memory. Ensure we reset the
> value to NULL to avoid using freed memory. git index-pack will read the
> repository's shallow file, which will have been updated with the correct
> information.

It may be worth mentioning bd0b42aed3 (fetch-pack: do not take shallow
lock unnecessarily - 2019-01-10). I believe this is the same problem
and a full solution was suggested but not implemented in that commit.

The problem with dangling alternate_shallow_file is also from that
commit. When line_received is false at the end of
receive_shallow_info(), we should clear alternate_shallow_file. I'm
still debating myself whether we should clear alternate_shallow_file
in receive_shallow_info() in addition to your changes (which is good
hygiene anyway) to keep the setup steps of do_fetch_pack() and
do_fetch_pack_v2() aligned.

pack protocol v1 does this clearing alternate_shallow_file (near the
end of do_fetch_pack()) so it will not have the same dangling pointer
problem even if we do two fetches in the same process. But from the
Tan's commit description, I thought v1 probably faces the same "lock
twice not supported". But luckily it's never triggered in practice.
When backfill_tags() is called, if I read it correctly, we either
disable shallow (TRANS_OPT_DEPTH set to zero) or we start a second
process. In either case, the lock will not be acquired twice by the
same process.

> Signed-off-by: brian m. carlson <sandals@xxxxxxxxxxxxxxxxxxxx>
> ---
>  fetch-pack.c | 3 +++
>  1 file changed, 3 insertions(+)
> diff --git a/fetch-pack.c b/fetch-pack.c
> index 577faa6229..2d76287674 100644
> --- a/fetch-pack.c
> +++ b/fetch-pack.c
> @@ -1489,6 +1489,7 @@ static void update_shallow(struct fetch_pack_args *args,
>                         rollback_lock_file(&shallow_lock);
>                 } else
>                         commit_lock_file(&shallow_lock);
> +               alternate_shallow_file = NULL;
>                 return;
>         }
> @@ -1512,6 +1513,7 @@ static void update_shallow(struct fetch_pack_args *args,
>                                                 &alternate_shallow_file,
>                                                 &extra);
>                         commit_lock_file(&shallow_lock);
> +                       alternate_shallow_file = NULL;
>                 }
>                 oid_array_clear(&extra);
>                 return;
> @@ -1551,6 +1553,7 @@ static void update_shallow(struct fetch_pack_args *args,
>                 commit_lock_file(&shallow_lock);
>                 oid_array_clear(&extra);
>                 oid_array_clear(&ref);
> +               alternate_shallow_file = NULL;
>                 return;
>         }