Re: [PATCH] fetch-pack: disregard invalid pack lockfiles

[Taylor Blau <me@ttaylorr.com>]

On Mon, Nov 30, 2020 at 08:27:15PM +0100, René Scharfe wrote:
> index_pack_lockfile() can return NULL if it doesn't like the contents it
> reads from the file descriptor passed to it.  unlink(2) is declared to
> not accept NULL pointers (at least with glibc).  Undefined Behavior
> Sanitizer together with Address Sanitizer detects a case where a NULL
> lockfile name is passed to unlink(2) by transport_unlock_pack() in t1060
> (make SANITIZE=address,undefined; cd t; ./t1060-object-corruption.sh).

Which test in t1060? I tried to reproduce this myself, but couldn't seem
to coax out a failure. (Initially I thought that my ccache wasn't
letting me recompile with the SANITIZE options, but running 'ccache
clear' and then trying again left the test still passing).

> Reinstate the NULL check to avoid undefined behavior, but put it right
> at the source, so that the number of items in the string_list reflects
> the number of valid lockfiles.
>
> Signed-off-by: René Scharfe <l.s.r@web.de>
> ---
>  fetch-pack.c | 5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)
>
> diff --git a/fetch-pack.c b/fetch-pack.c
> index b10c432315..4625926cf0 100644
> --- a/fetch-pack.c
> +++ b/fetch-pack.c
> @@ -915,8 +915,9 @@ static int get_pack(struct fetch_pack_args *args,
>  	if (start_command(&cmd))
>  		die(_("fetch-pack: unable to fork off %s"), cmd_name);
>  	if (do_keep && pack_lockfiles) {
> -		string_list_append_nodup(pack_lockfiles,
> -					 index_pack_lockfile(cmd.out));
> +		char *pack_lockfile = index_pack_lockfile(cmd.out);
> +		if (pack_lockfile)
> +			string_list_append_nodup(pack_lockfiles, pack_lockfile);

Makes sense.

Thanks,
Taylor