threads / patch / 54766

patchfetch-pack: check result of index_pack_lockfile

Subject: [PATCH] fetch-pack: check result of index_pack_lockfile

## tl;dr

4 messages between Dec 4, 2020 and Dec 4, 2020. Diffs are folded; open one to read it.

replies: 3people: 3as markdown or json

Samuel Thibault· Dec 4, 2020, 22:14 UTC · lore

The fetch-pack command may fail (e.g. like in test 15 - fetch into corrupted repo with index-pack), in which case index_pack_lockfile will return NULL. We should then avoid adding it to pack_lockfiles, since the rest of the code assumes that it is non-NULL. Notably transport_unlock_pack() calls unlink_or_warn() with it, thus unlink() with it. On Linux that fortunately only returns EFAULT, but other systems would segfault there.

Signed-off-by: Samuel Thibault <samuel.thibault@ens-lyon.org>
---
 fetch-pack.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

Show changes to fetch-pack.c +3 −2

diff --git a/fetch-pack.c b/fetch-pack.c
index b10c432315..7d31232960 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 *lockfile = index_pack_lockfile(cmd.out);
+		if (lockfile)
+			string_list_append_nodup(pack_lockfiles, lockfile);
 		close(cmd.out);
 	}

-- 
2.29.2

Junio C Hamano· Dec 4, 2020, 22:44 UTC · re: Samuel Thibault · lore

Re: [PATCH] fetch-pack: check result of index_pack_lockfile

Samuel Thibault <samuel.thibault@ens-lyon.org> writes:

> The fetch-pack command may fail (e.g. like in test 15 - fetch into corrupted
> repo with index-pack), in which case index_pack_lockfile will
> return ...

Thanks.

It sounds like the same as 6031af38 (fetch-pack: disregard invalid pack lockfiles, 2020-11-30), which came from

    Message-Id: <c54233ce-ff72-ca29-68c2-1416169b8e42@web.de>

Samuel Thibault· Dec 4, 2020, 23:14 UTC · re: Junio C Hamano · lore

Re: [PATCH] fetch-pack: check result of index_pack_lockfile

Junio C Hamano, le ven. 04 déc. 2020 14:44:39 -0800, a ecrit:

Show 12 quoted lines

> Samuel Thibault <samuel.thibault@ens-lyon.org> writes:
> 
> > The fetch-pack command may fail (e.g. like in test 15 - fetch into corrupted
> > repo with index-pack), in which case index_pack_lockfile will
> > return ...
> 
> Thanks.
> 
> It sounds like the same as 6031af38 (fetch-pack: disregard invalid
> pack lockfiles, 2020-11-30), which came from
> 
>     Message-Id: <c54233ce-ff72-ca29-68c2-1416169b8e42@web.de>

Oh, indeed!  I hadn't realized there was a "next" branch.

Samuel

Johannes Schindelin· Dec 4, 2020, 23:16 UTC · re: Samuel Thibault · lore

Re: [PATCH] fetch-pack: check result of index_pack_lockfile

Hi,

On Fri, 4 Dec 2020, Samuel Thibault wrote:

Show 6 quoted lines

> The fetch-pack command may fail (e.g. like in test 15 - fetch into corrupted
> repo with index-pack), in which case index_pack_lockfile will return
> NULL. We should then avoid adding it to pack_lockfiles, since the rest of
> the code assumes that it is non-NULL. Notably transport_unlock_pack() calls
> unlink_or_warn() with it, thus unlink() with it. On Linux that fortunately
> only returns EFAULT, but other systems would segfault there.

Good explanation.

Show 18 quoted lines

> Signed-off-by: Samuel Thibault <samuel.thibault@ens-lyon.org>
> ---
>  fetch-pack.c | 5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)
>
> diff --git a/fetch-pack.c b/fetch-pack.c
> index b10c432315..7d31232960 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 *lockfile = index_pack_lockfile(cmd.out);
> +		if (lockfile)
> +			string_list_append_nodup(pack_lockfiles, lockfile);

I did stumble over this or a similar problem while glancing over the Coverity issues recently. Thank you for addressing it.

In this instance, however, I wonder whether we want to even continue if we cannot create the lock file?

Ciao, Johannes

Show 7 quoted lines

>  		close(cmd.out);
>  	}
>
> --
> 2.29.2
>
>