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

From: Duy Nguyen <pclouds@gmail.com>
Date: Feb 4, 2019, 10:34 UTC
Message-ID: <CACsJy8CvVPr+OJ04aLGDeAS-o5__x1+3cKQNuq2+qba5gw5W+g@mail.gmail.com>
In-Reply-To: <20190204000650.686175-1-sandals@crustytoothpaste.net>

On Mon, Feb 4, 2019 at 7:06 AM brian m. carlson <sandals@crustytoothpaste.net> wrote:

Show 11 quoted lines

>
> 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.

Show 33 quoted lines

> Signed-off-by: brian m. carlson <sandals@crustytoothpaste.net>
> ---
>  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;
>         }
>

-- 
Duy