* [PATCH] fetch-pack: check result of index_pack_lockfile
@ 2020-12-04 22:14 Samuel Thibault
2020-12-04 22:44 ` Junio C Hamano
2020-12-04 23:16 ` Johannes Schindelin
0 siblings, 2 replies; 4+ messages in thread
From: Samuel Thibault @ 2020-12-04 22:14 UTC (permalink / raw)
To: git; +Cc: Samuel Thibault
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(-)
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
^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: [PATCH] fetch-pack: check result of index_pack_lockfile
2020-12-04 22:14 [PATCH] fetch-pack: check result of index_pack_lockfile Samuel Thibault
@ 2020-12-04 22:44 ` Junio C Hamano
2020-12-04 23:14 ` Samuel Thibault
2020-12-04 23:16 ` Johannes Schindelin
1 sibling, 1 reply; 4+ messages in thread
From: Junio C Hamano @ 2020-12-04 22:44 UTC (permalink / raw)
To: Samuel Thibault; +Cc: git
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>
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH] fetch-pack: check result of index_pack_lockfile
2020-12-04 22:44 ` Junio C Hamano
@ 2020-12-04 23:14 ` Samuel Thibault
0 siblings, 0 replies; 4+ messages in thread
From: Samuel Thibault @ 2020-12-04 23:14 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git
Junio C Hamano, le ven. 04 déc. 2020 14:44:39 -0800, a ecrit:
> 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
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH] fetch-pack: check result of index_pack_lockfile
2020-12-04 22:14 [PATCH] fetch-pack: check result of index_pack_lockfile Samuel Thibault
2020-12-04 22:44 ` Junio C Hamano
@ 2020-12-04 23:16 ` Johannes Schindelin
1 sibling, 0 replies; 4+ messages in thread
From: Johannes Schindelin @ 2020-12-04 23:16 UTC (permalink / raw)
To: Samuel Thibault; +Cc: git
Hi,
On Fri, 4 Dec 2020, Samuel Thibault wrote:
> 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.
> 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
> close(cmd.out);
> }
>
> --
> 2.29.2
>
>
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2020-12-04 23:22 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-12-04 22:14 [PATCH] fetch-pack: check result of index_pack_lockfile Samuel Thibault
2020-12-04 22:44 ` Junio C Hamano
2020-12-04 23:14 ` Samuel Thibault
2020-12-04 23:16 ` Johannes Schindelin
Code repositories for project(s) associated with this public inbox
https://80x24.org/mirrors/git.git
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).