{"thread":{"id":"54766","subject":"[PATCH] fetch-pack: check result of index_pack_lockfile","startedAt":"2020-12-04T22:22:14Z","lastAt":"2020-12-04T23:18:40Z","messageCount":4,"participants":["Samuel Thibault","Junio C Hamano","Johannes Schindelin"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"411412","messageId":"20201204221457.2873935-1-samuel.thibault@ens-lyon.org","threadId":"54766","inReplyTo":null,"subject":"[PATCH] fetch-pack: check result of index_pack_lockfile","fromName":"Samuel Thibault","fromEmail":"samuel.thibault@ens-lyon.org","sentAt":"2020-12-04T22:14:57Z","receivedAt":"2020-12-04T22:22:14Z","isPatch":true,"sender":{"key":"samuel.thibault@ens-lyon.org","avatar":"https://gravatar.com/avatar/fff265e7cec4e803a5627a6e839d72cc8f80ab86533031c5d4411da3aacea5e3?d=mp&s=160"},"body":"The fetch-pack command may fail (e.g. like in test 15 - fetch into corrupted\nrepo with index-pack), in which case index_pack_lockfile will return\nNULL. We should then avoid adding it to pack_lockfiles, since the rest of\nthe code assumes that it is non-NULL. Notably transport_unlock_pack() calls\nunlink_or_warn() with it, thus unlink() with it. On Linux that fortunately\nonly returns EFAULT, but other systems would segfault there.\n\nSigned-off-by: Samuel Thibault \n---\n fetch-pack.c | 5 +++--\n 1 file changed, 3 insertions(+), 2 deletions(-)\n\ndiff --git a/fetch-pack.c b/fetch-pack.c\nindex b10c432315..7d31232960 100644\n--- a/fetch-pack.c\n+++ b/fetch-pack.c\n@@ -915,8 +915,9 @@ static int get_pack(struct fetch_pack_args *args,\n \tif (start_command(&cmd))\n \t\tdie(_(\"fetch-pack: unable to fork off %s\"), cmd_name);\n \tif (do_keep && pack_lockfiles) {\n-\t\tstring_list_append_nodup(pack_lockfiles,\n-\t\t\t\t\t index_pack_lockfile(cmd.out));\n+\t\tchar *lockfile = index_pack_lockfile(cmd.out);\n+\t\tif (lockfile)\n+\t\t\tstring_list_append_nodup(pack_lockfiles, lockfile);\n \t\tclose(cmd.out);\n \t}\n \n-- \n2.29.2\n\n"},{"id":"411414","messageId":"xmqqwnxxcht4.fsf@gitster.c.googlers.com","threadId":"54766","inReplyTo":"20201204221457.2873935-1-samuel.thibault@ens-lyon.org","subject":"Re: [PATCH] fetch-pack: check result of index_pack_lockfile","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-12-04T22:44:39Z","receivedAt":"2020-12-04T22:45:41Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Samuel Thibault writes:\n\n> The fetch-pack command may fail (e.g. like in test 15 - fetch into corrupted\n> repo with index-pack), in which case index_pack_lockfile will\n> return ...\n\nThanks.\n\nIt sounds like the same as 6031af38 (fetch-pack: disregard invalid\npack lockfiles, 2020-11-30), which came from\n\n Message-Id: \n\n"},{"id":"411420","messageId":"20201204231433.w2iaj4q42ubc7cbs@function","threadId":"54766","inReplyTo":"xmqqwnxxcht4.fsf@gitster.c.googlers.com","subject":"Re: [PATCH] fetch-pack: check result of index_pack_lockfile","fromName":"Samuel Thibault","fromEmail":"samuel.thibault@ens-lyon.org","sentAt":"2020-12-04T23:14:33Z","receivedAt":"2020-12-04T23:15:19Z","isPatch":true,"sender":{"key":"samuel.thibault@ens-lyon.org","avatar":"https://gravatar.com/avatar/fff265e7cec4e803a5627a6e839d72cc8f80ab86533031c5d4411da3aacea5e3?d=mp&s=160"},"body":"Junio C Hamano, le ven. 04 déc. 2020 14:44:39 -0800, a ecrit:\n> Samuel Thibault writes:\n> \n> > The fetch-pack command may fail (e.g. like in test 15 - fetch into corrupted\n> > repo with index-pack), in which case index_pack_lockfile will\n> > return ...\n> \n> Thanks.\n> \n> It sounds like the same as 6031af38 (fetch-pack: disregard invalid\n> pack lockfiles, 2020-11-30), which came from\n> \n> Message-Id: \n\nOh, indeed! I hadn't realized there was a \"next\" branch.\n\nSamuel\n"},{"id":"411422","messageId":"nycvar.QRO.7.76.6.2012050014280.25979@tvgsbejvaqbjf.bet","threadId":"54766","inReplyTo":"20201204221457.2873935-1-samuel.thibault@ens-lyon.org","subject":"Re: [PATCH] fetch-pack: check result of index_pack_lockfile","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2020-12-04T23:16:49Z","receivedAt":"2020-12-04T23:18:40Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi,\n\nOn Fri, 4 Dec 2020, Samuel Thibault wrote:\n\n> The fetch-pack command may fail (e.g. like in test 15 - fetch into corrupted\n> repo with index-pack), in which case index_pack_lockfile will return\n> NULL. We should then avoid adding it to pack_lockfiles, since the rest of\n> the code assumes that it is non-NULL. Notably transport_unlock_pack() calls\n> unlink_or_warn() with it, thus unlink() with it. On Linux that fortunately\n> only returns EFAULT, but other systems would segfault there.\n\nGood explanation.\n\n> Signed-off-by: Samuel Thibault \n> ---\n> fetch-pack.c | 5 +++--\n> 1 file changed, 3 insertions(+), 2 deletions(-)\n>\n> diff --git a/fetch-pack.c b/fetch-pack.c\n> index b10c432315..7d31232960 100644\n> --- a/fetch-pack.c\n> +++ b/fetch-pack.c\n> @@ -915,8 +915,9 @@ static int get_pack(struct fetch_pack_args *args,\n> \tif (start_command(&cmd))\n> \t\tdie(_(\"fetch-pack: unable to fork off %s\"), cmd_name);\n> \tif (do_keep && pack_lockfiles) {\n> -\t\tstring_list_append_nodup(pack_lockfiles,\n> -\t\t\t\t\t index_pack_lockfile(cmd.out));\n> +\t\tchar *lockfile = index_pack_lockfile(cmd.out);\n> +\t\tif (lockfile)\n> +\t\t\tstring_list_append_nodup(pack_lockfiles, lockfile);\n\nI did stumble over this or a similar problem while glancing over the\nCoverity issues recently. Thank you for addressing it.\n\nIn this instance, however, I wonder whether we want to even continue if we\ncannot create the lock file?\n\nCiao,\nJohannes\n\n> \t\tclose(cmd.out);\n> \t}\n>\n> --\n> 2.29.2\n>\n>\n"}]}