{"thread":{"id":"54732","subject":"[PATCH] fetch-pack: disregard invalid pack lockfiles","startedAt":"2020-11-30T19:29:56Z","lastAt":"2020-12-01T04:54:07Z","messageCount":5,"participants":["René Scharfe","Taylor Blau","Jeff King"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"411007","messageId":"c54233ce-ff72-ca29-68c2-1416169b8e42@web.de","threadId":"54732","inReplyTo":null,"subject":"[PATCH] fetch-pack: disregard invalid pack lockfiles","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2020-11-30T19:27:15Z","receivedAt":"2020-11-30T19:29:56Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"9da69a6539 (fetch-pack: support more than one pack lockfile, 2020-06-10)\nstarted to use a string_list for pack lockfile names instead of a single\nstring pointer. It removed a NULL check from transport_unlock_pack() as\nwell, which is the function that eventually deletes these lockfiles and\nreleases their name strings.\n\nindex_pack_lockfile() can return NULL if it doesn't like the contents it\nreads from the file descriptor passed to it. unlink(2) is declared to\nnot accept NULL pointers (at least with glibc). Undefined Behavior\nSanitizer together with Address Sanitizer detects a case where a NULL\nlockfile name is passed to unlink(2) by transport_unlock_pack() in t1060\n(make SANITIZE=address,undefined; cd t; ./t1060-object-corruption.sh).\n\nReinstate the NULL check to avoid undefined behavior, but put it right\nat the source, so that the number of items in the string_list reflects\nthe number of valid lockfiles.\n\nSigned-off-by: René Scharfe \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..4625926cf0 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 *pack_lockfile = index_pack_lockfile(cmd.out);\n+\t\tif (pack_lockfile)\n+\t\t\tstring_list_append_nodup(pack_lockfiles, pack_lockfile);\n \t\tclose(cmd.out);\n \t}\n\n--\n2.29.2\n"},{"id":"411008","messageId":"X8VNszeQKJPfZ+Ht@nand.local","threadId":"54732","inReplyTo":"c54233ce-ff72-ca29-68c2-1416169b8e42@web.de","subject":"Re: [PATCH] fetch-pack: disregard invalid pack lockfiles","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2020-11-30T19:53:23Z","receivedAt":"2020-11-30T19:54:08Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Mon, Nov 30, 2020 at 08:27:15PM +0100, René Scharfe wrote:\n> index_pack_lockfile() can return NULL if it doesn't like the contents it\n> reads from the file descriptor passed to it. unlink(2) is declared to\n> not accept NULL pointers (at least with glibc). Undefined Behavior\n> Sanitizer together with Address Sanitizer detects a case where a NULL\n> lockfile name is passed to unlink(2) by transport_unlock_pack() in t1060\n> (make SANITIZE=address,undefined; cd t; ./t1060-object-corruption.sh).\n\nWhich test in t1060? I tried to reproduce this myself, but couldn't seem\nto coax out a failure. (Initially I thought that my ccache wasn't\nletting me recompile with the SANITIZE options, but running 'ccache\nclear' and then trying again left the test still passing).\n\n> Reinstate the NULL check to avoid undefined behavior, but put it right\n> at the source, so that the number of items in the string_list reflects\n> the number of valid lockfiles.\n>\n> Signed-off-by: René Scharfe \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..4625926cf0 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 *pack_lockfile = index_pack_lockfile(cmd.out);\n> +\t\tif (pack_lockfile)\n> +\t\t\tstring_list_append_nodup(pack_lockfiles, pack_lockfile);\n\nMakes sense.\n\nThanks,\nTaylor\n"},{"id":"411009","messageId":"67cf2f10-43a4-1200-0c60-dada466eadb9@web.de","threadId":"54732","inReplyTo":"X8VNszeQKJPfZ+Ht@nand.local","subject":"Re: [PATCH] fetch-pack: disregard invalid pack lockfiles","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2020-11-30T20:15:47Z","receivedAt":"2020-11-30T20:17:55Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Am 30.11.20 um 20:53 schrieb Taylor Blau:\n> On Mon, Nov 30, 2020 at 08:27:15PM +0100, René Scharfe wrote:\n>> index_pack_lockfile() can return NULL if it doesn't like the contents it\n>> reads from the file descriptor passed to it. unlink(2) is declared to\n>> not accept NULL pointers (at least with glibc). Undefined Behavior\n>> Sanitizer together with Address Sanitizer detects a case where a NULL\n>> lockfile name is passed to unlink(2) by transport_unlock_pack() in t1060\n>> (make SANITIZE=address,undefined; cd t; ./t1060-object-corruption.sh).\n>\n> Which test in t1060? I tried to reproduce this myself, but couldn't seem\n> to coax out a failure. (Initially I thought that my ccache wasn't\n> letting me recompile with the SANITIZE options, but running 'ccache\n> clear' and then trying again left the test still passing).\n\n15 - fetch into corrupted repo with index-pack\n\n $ cat trash\\ directory.t1060-object-corruption/bit-error-cp/stderr\n error: inflate: data stream error (invalid distance too far back)\n error: unable to unpack d95f3ad14dee633a758d2e331151e950dd13e4ed header\n fatal: cannot read existing object info d95f3ad14dee633a758d2e331151e950dd13e4ed\n fatal: index-pack failed\n wrapper.c:568:52: runtime error: null pointer passed as argument 1, which is declared to never be null\n /usr/include/unistd.h:825:48: note: nonnull attribute specified here\n SUMMARY: UndefinedBehaviorSanitizer: undefined-behavior wrapper.c:568:52 in\n Aborted\n\nCompiled with:\n Debian clang version 11.0.0-5+b1\n Target: x86_64-pc-linux-gnu\n Thread model: posix\n InstalledDir: /usr/bin\n\nRené\n"},{"id":"411012","messageId":"X8VUf/nbRiMvQFpu@nand.local","threadId":"54732","inReplyTo":"67cf2f10-43a4-1200-0c60-dada466eadb9@web.de","subject":"Re: [PATCH] fetch-pack: disregard invalid pack lockfiles","fromName":"Taylor Blau","fromEmail":"ttaylorr@github.com","sentAt":"2020-11-30T20:22:46Z","receivedAt":"2020-11-30T20:23:49Z","isPatch":true,"sender":{"key":"ttaylorr@github.com","avatar":"https://gravatar.com/avatar/d5f3476f26b6f99cbb6b467e7ed7482f5762c8157bc73f569196e428bdcbea25?d=mp&s=160"},"body":"On Mon, Nov 30, 2020 at 09:15:47PM +0100, René Scharfe wrote:\n> Am 30.11.20 um 20:53 schrieb Taylor Blau:\n> > On Mon, Nov 30, 2020 at 08:27:15PM +0100, René Scharfe wrote:\n> >> index_pack_lockfile() can return NULL if it doesn't like the contents it\n> >> reads from the file descriptor passed to it. unlink(2) is declared to\n> >> not accept NULL pointers (at least with glibc). Undefined Behavior\n> >> Sanitizer together with Address Sanitizer detects a case where a NULL\n> >> lockfile name is passed to unlink(2) by transport_unlock_pack() in t1060\n> >> (make SANITIZE=address,undefined; cd t; ./t1060-object-corruption.sh).\n> >\n> > Which test in t1060? I tried to reproduce this myself, but couldn't seem\n> > to coax out a failure. (Initially I thought that my ccache wasn't\n> > letting me recompile with the SANITIZE options, but running 'ccache\n> > clear' and then trying again left the test still passing).\n>\n> 15 - fetch into corrupted repo with index-pack\n>\n> $ cat trash\\ directory.t1060-object-corruption/bit-error-cp/stderr\n> error: inflate: data stream error (invalid distance too far back)\n> error: unable to unpack d95f3ad14dee633a758d2e331151e950dd13e4ed header\n> fatal: cannot read existing object info d95f3ad14dee633a758d2e331151e950dd13e4ed\n> fatal: index-pack failed\n> wrapper.c:568:52: runtime error: null pointer passed as argument 1, which is declared to never be null\n> /usr/include/unistd.h:825:48: note: nonnull attribute specified here\n> SUMMARY: UndefinedBehaviorSanitizer: undefined-behavior wrapper.c:568:52 in\n> Aborted\n>\n> Compiled with:\n> Debian clang version 11.0.0-5+b1\n> Target: x86_64-pc-linux-gnu\n> Thread model: posix\n> InstalledDir: /usr/bin\n\nI see. I was compiling with: gcc 10.2.0, so setting CC=clang does\nreproduce the error for me.\n\n Reviewed-by: Taylor Blau \n\nThanks,\nTaylor\n"},{"id":"411036","messageId":"X8XMQiAmDqJXy0d3@coredump.intra.peff.net","threadId":"54732","inReplyTo":"c54233ce-ff72-ca29-68c2-1416169b8e42@web.de","subject":"Re: [PATCH] fetch-pack: disregard invalid pack lockfiles","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2020-12-01T04:53:22Z","receivedAt":"2020-12-01T04:54:07Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Nov 30, 2020 at 08:27:15PM +0100, René Scharfe wrote:\n\n> 9da69a6539 (fetch-pack: support more than one pack lockfile, 2020-06-10)\n> started to use a string_list for pack lockfile names instead of a single\n> string pointer. It removed a NULL check from transport_unlock_pack() as\n> well, which is the function that eventually deletes these lockfiles and\n> releases their name strings.\n> \n> index_pack_lockfile() can return NULL if it doesn't like the contents it\n> reads from the file descriptor passed to it. unlink(2) is declared to\n> not accept NULL pointers (at least with glibc). Undefined Behavior\n> Sanitizer together with Address Sanitizer detects a case where a NULL\n> lockfile name is passed to unlink(2) by transport_unlock_pack() in t1060\n> (make SANITIZE=address,undefined; cd t; ./t1060-object-corruption.sh).\n> \n> Reinstate the NULL check to avoid undefined behavior, but put it right\n> at the source, so that the number of items in the string_list reflects\n> the number of valid lockfiles.\n\nIt took me a minute to understand how 9da69a6539 made this worse, since\nin the hunk you're touching here, the original \"if NULL, do nothing\"\ncheck was checking the pointer-to-pointer to see if the caller was\ninterested in the lockfile name. But your \"but put it right at the\nsource\" pointed me in the right direction. The hunk from 9da69a6539 that\nmatters is this one:\n\n - if (pack_lockfile) {\n - printf(\"lock %s\\n\", pack_lockfile);\n + if (pack_lockfiles.nr) {\n + int i;\n +\n + printf(\"lock %s\\n\", pack_lockfiles.items[0].string);\n fflush(stdout);\n + for (i = 1; i < pack_lockfiles.nr; i++)\n + warning(_(\"Lockfile created but not reported: %s\"),\n + pack_lockfiles.items\n\n(not complaining about anything, just verbosely reviewing).\n\n> diff --git a/fetch-pack.c b/fetch-pack.c\n> index b10c432315..4625926cf0 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 *pack_lockfile = index_pack_lockfile(cmd.out);\n> +\t\tif (pack_lockfile)\n> +\t\t\tstring_list_append_nodup(pack_lockfiles, pack_lockfile);\n> \t\tclose(cmd.out);\n> \t}\n\nSo this is an obviously correct fix, but I have to wonder whether we\nought to be (even before 9da69a6539) complaining about pack-objects not\ncorrectly reporting the pack name to us. I think in practice this\nhappens when it dies early without reporting anything to us, in which\ncase we'd notice its non-zero exit anyway. So it's probably not a big\ndeal, but would amount to an assertion (and if we did want to do it, it\nshould come on top of your fix, and not hold it up).\n\n-Peff\n"}]}