[PATCH] fetch-pack: disregard invalid pack lockfiles

[PATCH] fetch-pack: disregard invalid pack lockfiles

[René Scharfe]

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.

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

--
2.29.2

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

[Taylor Blau]

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

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

[René Scharfe]

Am 30.11.20 um 20:53 schrieb Taylor Blau:
> 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).

15 - fetch into corrupted repo with index-pack

   $ cat trash\ directory.t1060-object-corruption/bit-error-cp/stderr
   error: inflate: data stream error (invalid distance too far back)
   error: unable to unpack d95f3ad14dee633a758d2e331151e950dd13e4ed header
   fatal: cannot read existing object info d95f3ad14dee633a758d2e331151e950dd13e4ed
   fatal: index-pack failed
   wrapper.c:568:52: runtime error: null pointer passed as argument 1, which is declared to never be null
   /usr/include/unistd.h:825:48: note: nonnull attribute specified here
   SUMMARY: UndefinedBehaviorSanitizer: undefined-behavior wrapper.c:568:52 in
   Aborted

Compiled with:
   Debian clang version 11.0.0-5+b1
   Target: x86_64-pc-linux-gnu
   Thread model: posix
   InstalledDir: /usr/bin

René

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

[Taylor Blau]

On Mon, Nov 30, 2020 at 09:15:47PM +0100, René Scharfe wrote:
> Am 30.11.20 um 20:53 schrieb Taylor Blau:
> > 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).
>
> 15 - fetch into corrupted repo with index-pack
>
>    $ cat trash\ directory.t1060-object-corruption/bit-error-cp/stderr
>    error: inflate: data stream error (invalid distance too far back)
>    error: unable to unpack d95f3ad14dee633a758d2e331151e950dd13e4ed header
>    fatal: cannot read existing object info d95f3ad14dee633a758d2e331151e950dd13e4ed
>    fatal: index-pack failed
>    wrapper.c:568:52: runtime error: null pointer passed as argument 1, which is declared to never be null
>    /usr/include/unistd.h:825:48: note: nonnull attribute specified here
>    SUMMARY: UndefinedBehaviorSanitizer: undefined-behavior wrapper.c:568:52 in
>    Aborted
>
> Compiled with:
>    Debian clang version 11.0.0-5+b1
>    Target: x86_64-pc-linux-gnu
>    Thread model: posix
>    InstalledDir: /usr/bin

I see. I was compiling with: gcc 10.2.0, so setting CC=clang does
reproduce the error for me.

  Reviewed-by: Taylor Blau <me@ttaylorr.com>

Thanks,
Taylor

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

[Jeff King]

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