{"thread":{"id":"61855","subject":"[2.46 regression] git ls-remote crash with import remote-helper","startedAt":"2024-07-27T19:50:12Z","lastAt":"2024-08-02T15:26:48Z","messageCount":6,"participants":["Mike Hommey","Junio C Hamano","Patrick Steinhardt"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"499489","messageId":"20240727191917.p64ul4jybpm2a7hm@glandium.org","threadId":"61855","inReplyTo":null,"subject":"[2.46 regression] git ls-remote crash with import remote-helper","fromName":"Mike Hommey","fromEmail":"mh@glandium.org","sentAt":"2024-07-27T19:19:17Z","receivedAt":"2024-07-27T19:50:12Z","isPatch":false,"sender":{"key":"mh@glandium.org","avatar":"https://avatars.githubusercontent.com/u/1038527?v=4"},"body":"Hi,\n\nRunning `git ls-remote $helper::$url` crashes when run outside a git\nrepo and the helper uses the import feature.\n\nHere is a minimal reproducer:\n```\n$ cat > git-remote-foo < writes:\n\n> Running `git ls-remote $helper::$url` crashes when run outside a git\n> repo and the helper uses the import feature.\n>\n> Here is a minimal reproducer:\n> ```\n> $ cat > git-remote-foo < #!/bin/sh\n> echo import\n> echo refspec '*:*'\n> EOF\n> $ chmod +x git-remote-foo\n> $ PATH=$PWD:$PATH git ls-remote foo::bar\n> ```\n>\n> The crash happens in parse_refspec in refspec.c, on a deref of the_hash_algo,\n> because the_hash_also is not set anymore at that point since c8aed5e8da.\n\nThanks for a report, Mike.\n\nPatrick, we have expected reports like this when we did c8aed5e8\n(repository: stop setting SHA1 as the default object hash,\n2024-05-07), so it is not very surprising. In general, I think any\ncommand that is designed to be usable outside a repository should\ncontinue to fall back and use SHA-1, at least for now. A command\nlike ls-remote _might_ want to do even better by waiting until it\nhas a chance to inspect what the other side said before setting the\nhash-algo, or even better is to make it work without having any\nconcrete value in the hash-algo. After all, when SHA-256\nrepositories become common out in the world, you should be able to\nsay ls-remote against them from your SHA-1 repository and the fact\nthat the hash-algo is read from local repository and set to SHA-1\nshould *not* negatively affect our ability to receive the ls-remote\nresponse from SHA-256 repositories. But that are all for longer\nterm future. At least assuming SHA-1 like we have always done\nshould be better than the current situation.\n\nThanks.\n\n\n"},{"id":"499523","messageId":"Zqe4Mec80hKaPWfH@tanuki","threadId":"61855","inReplyTo":"xmqqle1mrx22.fsf@gitster.g","subject":"Re: [2.46 regression] git ls-remote crash with import remote-helper","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-07-29T15:41:37Z","receivedAt":"2024-07-29T15:41:51Z","isPatch":false,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Sat, Jul 27, 2024 at 08:54:13PM -0700, Junio C Hamano wrote:\n> Mike Hommey writes:\n> \n> > Running `git ls-remote $helper::$url` crashes when run outside a git\n> > repo and the helper uses the import feature.\n> >\n> > Here is a minimal reproducer:\n> > ```\n> > $ cat > git-remote-foo < > #!/bin/sh\n> > echo import\n> > echo refspec '*:*'\n> > EOF\n> > $ chmod +x git-remote-foo\n> > $ PATH=$PWD:$PATH git ls-remote foo::bar\n> > ```\n> >\n> > The crash happens in parse_refspec in refspec.c, on a deref of the_hash_algo,\n> > because the_hash_also is not set anymore at that point since c8aed5e8da.\n> \n> Thanks for a report, Mike.\n\nIndeed, thanks for the report, I was able to reproduce the segfault\neasily with that reproducer.\n\n> Patrick, we have expected reports like this when we did c8aed5e8\n> (repository: stop setting SHA1 as the default object hash,\n> 2024-05-07), so it is not very surprising. In general, I think any\n> command that is designed to be usable outside a repository should\n> continue to fall back and use SHA-1, at least for now. A command\n> like ls-remote _might_ want to do even better by waiting until it\n> has a chance to inspect what the other side said before setting the\n> hash-algo, or even better is to make it work without having any\n> concrete value in the hash-algo. After all, when SHA-256\n> repositories become common out in the world, you should be able to\n> say ls-remote against them from your SHA-1 repository and the fact\n> that the hash-algo is read from local repository and set to SHA-1\n> should *not* negatively affect our ability to receive the ls-remote\n> response from SHA-256 repositories. But that are all for longer\n> term future. At least assuming SHA-1 like we have always done\n> should be better than the current situation.\n\nI definitely agree that as a short-term solution it's probably the best\nway to handle this.\n\nThe below patch is what I came up with to paper over the issue. This\nrestores the old behaviour, which is somewhat broken because we end up\nmis-parsing refspecs. But I guess \"somewhat broken\" is arguably better\nthan \"completely broken\".\n\nLet me know whether you want me to send this as a proper standalone\npatch.\n\nPatrick\n\n--- >8 ---\n\ncommit c52112d3946b2fd8d030580cd7acb809fa54012a\nAuthor: Patrick Steinhardt \nDate: Mon Jul 29 17:21:00 2024 +0200\n\n builtin/ls-remote: fall back to SHA1 outside of a repo\n \n In c8aed5e8da (repository: stop setting SHA1 as the default object hash,\n 2024-05-07), we have stopped setting the default hash algorithm for\n `the_repository`. Consequently, code that relies on `the_hash_algo` will\n now crash when it hasn't explicitly been initialized, which may be the\n case when running outside of a Git repository.\n \n It was reported that git-ls-remote(1) may crash in such a way when using\n a remote helper that advertises refspecs. This is because the refspec\n announced by the helper will get parsed during capability negotiation.\n At that point we haven't yet figured out what object format the remote\n uses though, so when run outside of a repository then we will fail.\n \n The course of action is somewhat dubious in the first place. Ideally, we\n should only parse object IDs once we have asked the remote helper for\n the object format. And if the helper didn't announce the \"object-format\"\n capability, then we should always assume SHA256. But instead, we used to\n take either SHA1 if there was no repository, or we used the hash of the\n local repository, which is wrong.\n \n Arguably though, crashing hard may not be in the best interest of our\n users, either. So while the old behaviour was buggy, let's restore it\n for now as a short-term fix. We should eventually revisit, potentially\n by deferring the point in time when we parse the refspec until after we\n have figured out the remote's object hash.\n \n Reported-by: Mike Hommey \n Signed-off-by: Patrick Steinhardt \n\ndiff --git a/builtin/ls-remote.c b/builtin/ls-remote.c\nindex debf2d4f88..6da63a67f5 100644\n--- a/builtin/ls-remote.c\n+++ b/builtin/ls-remote.c\n@@ -91,6 +91,21 @@ int cmd_ls_remote(int argc, const char **argv, const char *prefix)\n \t\t\t PARSE_OPT_STOP_AT_NON_OPTION);\n \tdest = argv[0];\n \n+\t/*\n+\t * TODO: This is buggy, but required for transport helpers. When a\n+\t * transport helper advertises a \"refspec\", then we'd add that to a\n+\t * list of refspecs via `refspec_append()`, which transitively depends\n+\t * on `the_hash_algo`. Thus, when the hash algorithm isn't properly set\n+\t * up, this would lead to a segfault.\n+\t *\n+\t * We really should fix this in the transport helper logic such that we\n+\t * lazily parse refspec capabilities _after_ we have learned about the\n+\t * remote's object format. Otherwise, we may end up misparsing refspecs\n+\t * depending on what object hash the remote uses.\n+\t */\n+\tif (!the_repository->hash_algo)\n+\t\trepo_set_hash_algo(the_repository, GIT_HASH_SHA1);\n+\n \tpacket_trace_identity(\"ls-remote\");\n \n \tif (argc > 1) {\ndiff --git a/t/t5512-ls-remote.sh b/t/t5512-ls-remote.sh\nindex 42e77eb5a9..bc442ec221 100755\n--- a/t/t5512-ls-remote.sh\n+++ b/t/t5512-ls-remote.sh\n@@ -402,4 +402,17 @@ test_expect_success 'v0 clients can handle multiple symrefs' '\n \ttest_cmp expect actual\n '\n \n+test_expect_success 'helper with refspec capability fails gracefully' '\n+\tmkdir test-bin &&\n+\twrite_script test-bin/git-remote-foo <<-EOF &&\n+\techo import\n+\techo refspec ${SQ}*:*${SQ}\n+\tEOF\n+\t(\n+\t\tPATH=\"$PWD/test-bin:$PATH\" &&\n+\t\texport PATH &&\n+\t\ttest_must_fail nongit git ls-remote foo::bar\n+\t)\n+'\n+\n test_done\n\n"},{"id":"499525","messageId":"xmqqplqwi0c4.fsf@gitster.g","threadId":"61855","inReplyTo":"Zqe4Mec80hKaPWfH@tanuki","subject":"Re: [2.46 regression] git ls-remote crash with import remote-helper","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-07-29T17:18:03Z","receivedAt":"2024-07-29T17:18:06Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Patrick Steinhardt writes:\n\n>> ... After all, when SHA-256\n>> repositories become common out in the world, you should be able to\n>> say ls-remote against them from your SHA-1 repository and the fact\n>> that the hash-algo is read from local repository and set to SHA-1\n>> should *not* negatively affect our ability to receive the ls-remote\n>> response from SHA-256 repositories. But that are all for longer\n>> term future. At least assuming SHA-1 like we have always done\n>> should be better than the current situation.\n>\n> I definitely agree that as a short-term solution it's probably the best\n> way to handle this.\n>\n> The below patch is what I came up with to paper over the issue. This\n> restores the old behaviour, which is somewhat broken because we end up\n> mis-parsing refspecs. But I guess \"somewhat broken\" is arguably better\n> than \"completely broken\".\n\nIt probably matters much more that this \"will restore the old\nbehaviour\" than it is \"somewhat but not completely broken\". Going\nback to the old behaviour is not making anything worse.\n\nI do not consider this (or any other \"a command that optionally can\nwork outside a repository no longer works\") issue a ultra-high\npriority. If your \"ls-remote $URL\" does not work in a directory you\nwanted to run it, you can temporarily create an empty repository\nthere and run the command to obtain the result you wanted to get (in\nother words, there is an easy workaround).\n\nI was scanning the command[] table in git.c for entries marked with\nRUN_SETUP_GENTLY but other than ls-remote nothing that is commonly\nused stood out.\n\nSo let's make this one of the early \"oops, here is a fix for .1 when\nenough of them have accumulated\" patch.\n\nThanks.\n\n> Patrick\n>\n> --- >8 ---\n>\n> commit c52112d3946b2fd8d030580cd7acb809fa54012a\n> Author: Patrick Steinhardt \n> Date: Mon Jul 29 17:21:00 2024 +0200\n>\n> builtin/ls-remote: fall back to SHA1 outside of a repo\n> \n> In c8aed5e8da (repository: stop setting SHA1 as the default object hash,\n> 2024-05-07), we have stopped setting the default hash algorithm for\n> `the_repository`. Consequently, code that relies on `the_hash_algo` will\n> now crash when it hasn't explicitly been initialized, which may be the\n> case when running outside of a Git repository.\n> \n> It was reported that git-ls-remote(1) may crash in such a way when using\n> a remote helper that advertises refspecs. This is because the refspec\n> announced by the helper will get parsed during capability negotiation.\n> At that point we haven't yet figured out what object format the remote\n> uses though, so when run outside of a repository then we will fail.\n> \n> The course of action is somewhat dubious in the first place. Ideally, we\n> should only parse object IDs once we have asked the remote helper for\n> the object format. And if the helper didn't announce the \"object-format\"\n> capability, then we should always assume SHA256. But instead, we used to\n> take either SHA1 if there was no repository, or we used the hash of the\n> local repository, which is wrong.\n> \n> Arguably though, crashing hard may not be in the best interest of our\n> users, either. So while the old behaviour was buggy, let's restore it\n> for now as a short-term fix. We should eventually revisit, potentially\n> by deferring the point in time when we parse the refspec until after we\n> have figured out the remote's object hash.\n> \n> Reported-by: Mike Hommey \n> Signed-off-by: Patrick Steinhardt \n>\n> diff --git a/builtin/ls-remote.c b/builtin/ls-remote.c\n> index debf2d4f88..6da63a67f5 100644\n> --- a/builtin/ls-remote.c\n> +++ b/builtin/ls-remote.c\n> @@ -91,6 +91,21 @@ int cmd_ls_remote(int argc, const char **argv, const char *prefix)\n> \t\t\t PARSE_OPT_STOP_AT_NON_OPTION);\n> \tdest = argv[0];\n> \n> +\t/*\n> +\t * TODO: This is buggy, but required for transport helpers. When a\n> +\t * transport helper advertises a \"refspec\", then we'd add that to a\n> +\t * list of refspecs via `refspec_append()`, which transitively depends\n> +\t * on `the_hash_algo`. Thus, when the hash algorithm isn't properly set\n> +\t * up, this would lead to a segfault.\n> +\t *\n> +\t * We really should fix this in the transport helper logic such that we\n> +\t * lazily parse refspec capabilities _after_ we have learned about the\n> +\t * remote's object format. Otherwise, we may end up misparsing refspecs\n> +\t * depending on what object hash the remote uses.\n> +\t */\n> +\tif (!the_repository->hash_algo)\n> +\t\trepo_set_hash_algo(the_repository, GIT_HASH_SHA1);\n> +\n> \tpacket_trace_identity(\"ls-remote\");\n> \n> \tif (argc > 1) {\n> diff --git a/t/t5512-ls-remote.sh b/t/t5512-ls-remote.sh\n> index 42e77eb5a9..bc442ec221 100755\n> --- a/t/t5512-ls-remote.sh\n> +++ b/t/t5512-ls-remote.sh\n> @@ -402,4 +402,17 @@ test_expect_success 'v0 clients can handle multiple symrefs' '\n> \ttest_cmp expect actual\n> '\n> \n> +test_expect_success 'helper with refspec capability fails gracefully' '\n> +\tmkdir test-bin &&\n> +\twrite_script test-bin/git-remote-foo <<-EOF &&\n> +\techo import\n> +\techo refspec ${SQ}*:*${SQ}\n> +\tEOF\n> +\t(\n> +\t\tPATH=\"$PWD/test-bin:$PATH\" &&\n> +\t\texport PATH &&\n> +\t\ttest_must_fail nongit git ls-remote foo::bar\n> +\t)\n> +'\n> +\n> test_done\n"},{"id":"499929","messageId":"c52112d3946b2fd8d030580cd7acb809fa54012a.1722573777.git.ps@pks.im","threadId":"61855","inReplyTo":"20240727191917.p64ul4jybpm2a7hm@glandium.org","subject":"[PATCH] builtin/ls-remote: fall back to SHA1 outside of a repo","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-08-02T04:44:11Z","receivedAt":"2024-08-02T04:44:17Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"In c8aed5e8da (repository: stop setting SHA1 as the default object hash,\n2024-05-07), we have stopped setting the default hash algorithm for\n`the_repository`. Consequently, code that relies on `the_hash_algo` will\nnow crash when it hasn't explicitly been initialized, which may be the\ncase when running outside of a Git repository.\n\nIt was reported that git-ls-remote(1) may crash in such a way when using\na remote helper that advertises refspecs. This is because the refspec\nannounced by the helper will get parsed during capability negotiation.\nAt that point we haven't yet figured out what object format the remote\nuses though, so when run outside of a repository then we will fail.\n\nThe course of action is somewhat dubious in the first place. Ideally, we\nshould only parse object IDs once we have asked the remote helper for\nthe object format. And if the helper didn't announce the \"object-format\"\ncapability, then we should always assume SHA256. But instead, we used to\ntake either SHA1 if there was no repository, or we used the hash of the\nlocal repository, which is wrong.\n\nArguably though, crashing hard may not be in the best interest of our\nusers, either. So while the old behaviour was buggy, let's restore it\nfor now as a short-term fix. We should eventually revisit, potentially\nby deferring the point in time when we parse the refspec until after we\nhave figured out the remote's object hash.\n\nReported-by: Mike Hommey \nSigned-off-by: Patrick Steinhardt \n---\n\nI didn't spot this in the \"What's cooking\" report. I guess that's my own\nfault for not sending it as a proper patch, so let me fix that now :)\n\nPatrick\n\n builtin/ls-remote.c | 15 +++++++++++++++\n t/t5512-ls-remote.sh | 13 +++++++++++++\n 2 files changed, 28 insertions(+)\n\ndiff --git a/builtin/ls-remote.c b/builtin/ls-remote.c\nindex debf2d4f88..6da63a67f5 100644\n--- a/builtin/ls-remote.c\n+++ b/builtin/ls-remote.c\n@@ -91,6 +91,21 @@ int cmd_ls_remote(int argc, const char **argv, const char *prefix)\n \t\t\t PARSE_OPT_STOP_AT_NON_OPTION);\n \tdest = argv[0];\n \n+\t/*\n+\t * TODO: This is buggy, but required for transport helpers. When a\n+\t * transport helper advertises a \"refspec\", then we'd add that to a\n+\t * list of refspecs via `refspec_append()`, which transitively depends\n+\t * on `the_hash_algo`. Thus, when the hash algorithm isn't properly set\n+\t * up, this would lead to a segfault.\n+\t *\n+\t * We really should fix this in the transport helper logic such that we\n+\t * lazily parse refspec capabilities _after_ we have learned about the\n+\t * remote's object format. Otherwise, we may end up misparsing refspecs\n+\t * depending on what object hash the remote uses.\n+\t */\n+\tif (!the_repository->hash_algo)\n+\t\trepo_set_hash_algo(the_repository, GIT_HASH_SHA1);\n+\n \tpacket_trace_identity(\"ls-remote\");\n \n \tif (argc > 1) {\ndiff --git a/t/t5512-ls-remote.sh b/t/t5512-ls-remote.sh\nindex 42e77eb5a9..bc442ec221 100755\n--- a/t/t5512-ls-remote.sh\n+++ b/t/t5512-ls-remote.sh\n@@ -402,4 +402,17 @@ test_expect_success 'v0 clients can handle multiple symrefs' '\n \ttest_cmp expect actual\n '\n \n+test_expect_success 'helper with refspec capability fails gracefully' '\n+\tmkdir test-bin &&\n+\twrite_script test-bin/git-remote-foo <<-EOF &&\n+\techo import\n+\techo refspec ${SQ}*:*${SQ}\n+\tEOF\n+\t(\n+\t\tPATH=\"$PWD/test-bin:$PATH\" &&\n+\t\texport PATH &&\n+\t\ttest_must_fail nongit git ls-remote foo::bar\n+\t)\n+'\n+\n test_done\n-- \n2.46.0.dirty\n\n"},{"id":"499952","messageId":"xmqqle1feyiy.fsf@gitster.g","threadId":"61855","inReplyTo":"c52112d3946b2fd8d030580cd7acb809fa54012a.1722573777.git.ps@pks.im","subject":"Re: [PATCH] builtin/ls-remote: fall back to SHA1 outside of a repo","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-08-02T15:26:45Z","receivedAt":"2024-08-02T15:26:48Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Patrick Steinhardt writes:\n\n> I didn't spot this in the \"What's cooking\" report. I guess that's my own\n> fault for not sending it as a proper patch, so let me fix that now :)\n\nYeah, I had a fix already when I gave my response to the initial\nproblem report, but felt that it was too premature to commit to the\napproach before listening to others (you included) for potentially\nbetter alternative approaches, and then forgot about it.\n\nThe fix here is in line with my thoughts, after seeing how other\nparts of the transport work. Thanks for tying the loose ends.\n"}]}