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

[Jeff King <peff@peff.net>]

On Mon, Nov 30, 2020 at 08:27:15PM +0100, René Scharfe wrote:

> 9da69a6539 (fetch-pack: support more than one pack lockfile, 2020-06-10)
> started to use a string_list for pack lockfile names instead of a single
> string pointer.  It removed a NULL check from transport_unlock_pack() as
> well, which is the function that eventually deletes these lockfiles and
> releases their name strings.
> 
> 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).
> 
> 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.

It took me a minute to understand how 9da69a6539 made this worse, since
in the hunk you're touching here, the original "if NULL, do nothing"
check was checking the pointer-to-pointer to see if the caller was
interested in the lockfile name. But your "but put it right at the
source" pointed me in the right direction. The hunk from 9da69a6539 that
matters is this one:

  -       if (pack_lockfile) {
  -               printf("lock %s\n", pack_lockfile);
  +       if (pack_lockfiles.nr) {
  +               int i;
  +
  +               printf("lock %s\n", pack_lockfiles.items[0].string);
                  fflush(stdout);
  +               for (i = 1; i < pack_lockfiles.nr; i++)
  +                       warning(_("Lockfile created but not reported: %s"),
  +                               pack_lockfiles.items

(not complaining about anything, just verbosely reviewing).

> 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);
>  		close(cmd.out);
>  	}

So this is an obviously correct fix, but I have to wonder whether we
ought to be (even before 9da69a6539) complaining about pack-objects not
correctly reporting the pack name to us. I think in practice this
happens when it dies early without reporting anything to us, in which
case we'd notice its non-zero exit anyway. So it's probably not a big
deal, but would amount to an assertion (and if we did want to do it, it
should come on top of your fix, and not hold it up).

-Peff