Re: [PATCH] fetch-pack: check result of index_pack_lockfile
- From
Johannes Schindelin <johannes.schindelin@gmx.de>
- Date
- Dec 4, 2020, 23:16 UTC
- Message-ID
- <nycvar.QRO.7.76.6.2012050014280.25979@tvgsbejvaqbjf.bet>
- In-Reply-To
- <20201204221457.2873935-1-samuel.thibault@ens-lyon.org>
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 > >