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@xxxxxx>
> ---
> 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
