{"thread":{"id":"62244","subject":"[PATCH (v2.47 regression)] hash.h: set NEEDS_CLONE_HELPER_UNSAFE in fallback mode","startedAt":"2024-10-02T23:26:25Z","lastAt":"2024-10-03T18:20:22Z","messageCount":6,"participants":["Jeff King","brian m. carlson","Taylor Blau","Junio C Hamano"],"isPatch":true,"patchVersion":2,"patchTotal":null},"messages":[{"id":"504001","messageId":"20241002232618.GA3442753@coredump.intra.peff.net","threadId":"62244","inReplyTo":null,"subject":"[PATCH (v2.47 regression)] hash.h: set NEEDS_CLONE_HELPER_UNSAFE in fallback mode","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2024-10-02T23:26:18Z","receivedAt":"2024-10-02T23:26:25Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"Commit 253ed9ecff (hash.h: scaffolding for _unsafe hashing variants,\n2024-09-26) introduced the concept of having two hash algorithms: a safe\nand an unsafe one. When the Makefile knobs do not explicitly request an\nunsafe one, we fall back to using the safe algorithm.\n\nHowever, the fallback to do so forgot one case: we should inherit the\nNEEDS_CLONE_HELPER flag from the safe variant. Failing to do so means\nthat we'll end up defining two clone functions (the algorithm specific\none, and the generic one that just calls memcpy). You'll see an error\nlike this:\n\n $ make OPENSSL_SHA1=1\n [...]\n sha1/openssl.h:46:29: error: redefinition of ‘openssl_SHA1_Clone’\n 46 | #define platform_SHA1_Clone openssl_SHA1_Clone\n | ^~~~~~~~~~~~~~~~~~\n hash.h:83:40: note: in expansion of macro ‘platform_SHA1_Clone’\n 83 | # define platform_SHA1_Clone_unsafe platform_SHA1_Clone\n | ^~~~~~~~~~~~~~~~~~~\n hash.h:101:33: note: in expansion of macro ‘platform_SHA1_Clone_unsafe’\n 101 | # define git_SHA1_Clone_unsafe platform_SHA1_Clone_unsafe\n | ^~~~~~~~~~~~~~~~~~~~~~~~~~\n hash.h:133:20: note: in expansion of macro ‘git_SHA1_Clone_unsafe’\n 133 | static inline void git_SHA1_Clone_unsafe(git_SHA_CTX_unsafe *dst,\n | ^~~~~~~~~~~~~~~~~~~~~\n sha1/openssl.h:37:20: note: previous definition of ‘openssl_SHA1_Clone’ with type ‘void(struct openssl_SHA1_CTX *, const struct openssl_SHA1_CTX *)’\n 37 | static inline void openssl_SHA1_Clone(struct openssl_SHA1_CTX *dst,\n | ^~~~~~~~~~~~~~~~~~\n\nThis only matters when compiling with openssl as the \"safe\" variant,\nsince it's the only algorithm that requires a clone helper (and even\nthen, only if you are using openssl 3.0+). And you should never do that,\nbecause it's not safe. But still, the invocation above used to work and\nshould continue to do so until we decide to require a\ncollision-detecting variant for the safe algorithm entirely.\n\nSigned-off-by: Jeff King \n---\nThis is a regression in v2.47.0-rc0. As mentioned above, I kind of doubt\nanybody will hit it in practice (I only did because I was trying to do\nsome timing tests between the fast and dc variants). And it is almost\ntempting to leave it as a wake-up call for anybody who is still not\nusing a collision-detecting sha1. ;)\n\n hash.h | 3 +++\n 1 file changed, 3 insertions(+)\n\ndiff --git a/hash.h b/hash.h\nindex f97f858307..756166ce5e 100644\n--- a/hash.h\n+++ b/hash.h\n@@ -82,6 +82,9 @@\n # ifdef platform_SHA1_Clone\n # define platform_SHA1_Clone_unsafe platform_SHA1_Clone\n # endif\n+# ifdef SHA1_NEEDS_CLONE_HELPER\n+# define SHA1_NEEDS_CLONE_HELPER_UNSAFE\n+# endif\n #endif\n \n #define git_SHA_CTX\t\tplatform_SHA_CTX\n-- \n2.47.0.rc1.376.gb58337ca88\n"},{"id":"504004","messageId":"Zv3huwCNuqi5C8Wm@tapette.crustytoothpaste.net","threadId":"62244","inReplyTo":"20241002232618.GA3442753@coredump.intra.peff.net","subject":"Re: [PATCH (v2.47 regression)] hash.h: set NEEDS_CLONE_HELPER_UNSAFE in fallback mode","fromName":"brian m. carlson","fromEmail":"sandals@crustytoothpaste.net","sentAt":"2024-10-03T00:13:47Z","receivedAt":"2024-10-03T00:13:50Z","isPatch":true,"sender":{"key":"sandals@crustytoothpaste.net","avatar":"https://avatars.githubusercontent.com/u/497054?v=4"},"body":"On 2024-10-02 at 23:26:18, Jeff King wrote:\n> This is a regression in v2.47.0-rc0. As mentioned above, I kind of doubt\n> anybody will hit it in practice (I only did because I was trying to do\n> some timing tests between the fast and dc variants). And it is almost\n> tempting to leave it as a wake-up call for anybody who is still not\n> using a collision-detecting sha1. ;)\n\nI think this is a fine fix for 2.47. I have a branch on my remote\n(sha1-dc-only), which I'll send out after it passes CI (probably later\nthis week), that removes support for the everything but SHA-1-DC (except\nfor the unsafe code).\n\nI don't think there's a reasonable configuration where people can use\nGit with other SHA-1 code except in extremely limited circumstances we\nshouldn't have to maintain code for. The code is open source, so people\nwho really must have maximum performance with all of the vulnerabilities\ncan patch it back in themselves.\n-- \nbrian m. carlson (they/them or he/him)\nToronto, Ontario, CA\n"},{"id":"504006","messageId":"20241003002140.GB3486271@coredump.intra.peff.net","threadId":"62244","inReplyTo":"Zv3huwCNuqi5C8Wm@tapette.crustytoothpaste.net","subject":"Re: [PATCH (v2.47 regression)] hash.h: set NEEDS_CLONE_HELPER_UNSAFE in fallback mode","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2024-10-03T00:21:40Z","receivedAt":"2024-10-03T00:21:42Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Oct 03, 2024 at 12:13:47AM +0000, brian m. carlson wrote:\n\n> On 2024-10-02 at 23:26:18, Jeff King wrote:\n> > This is a regression in v2.47.0-rc0. As mentioned above, I kind of doubt\n> > anybody will hit it in practice (I only did because I was trying to do\n> > some timing tests between the fast and dc variants). And it is almost\n> > tempting to leave it as a wake-up call for anybody who is still not\n> > using a collision-detecting sha1. ;)\n> \n> I think this is a fine fix for 2.47. I have a branch on my remote\n> (sha1-dc-only), which I'll send out after it passes CI (probably later\n> this week), that removes support for the everything but SHA-1-DC (except\n> for the unsafe code).\n> \n> I don't think there's a reasonable configuration where people can use\n> Git with other SHA-1 code except in extremely limited circumstances we\n> shouldn't have to maintain code for. The code is open source, so people\n> who really must have maximum performance with all of the vulnerabilities\n> can patch it back in themselves.\n\nYeah, I feel the same way. I only happened to try this because it was\nthe easiest way to speed-compare different implementations using\n\"test-tool sha1\". ;)\n\nPossibly that helper could grow an option to use the unsafe variant,\nthough even that is probably not a high priority.\n\n-Peff\n"},{"id":"504008","messageId":"Zv3r9FBd+Nog3wb7@nand.local","threadId":"62244","inReplyTo":"20241002232618.GA3442753@coredump.intra.peff.net","subject":"Re: [PATCH (v2.47 regression)] hash.h: set NEEDS_CLONE_HELPER_UNSAFE in fallback mode","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2024-10-03T00:57:24Z","receivedAt":"2024-10-03T00:57:32Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Wed, Oct 02, 2024 at 07:26:18PM -0400, Jeff King wrote:\n> diff --git a/hash.h b/hash.h\n> index f97f858307..756166ce5e 100644\n> --- a/hash.h\n> +++ b/hash.h\n> @@ -82,6 +82,9 @@\n> # ifdef platform_SHA1_Clone\n> # define platform_SHA1_Clone_unsafe platform_SHA1_Clone\n> # endif\n> +# ifdef SHA1_NEEDS_CLONE_HELPER\n> +# define SHA1_NEEDS_CLONE_HELPER_UNSAFE\n> +# endif\n> #endif\n\nGah. I could have sworn I wrote something like this myself, but I must\nhave dropped it or hallucinated writing it in the first place.\n\nThanks for finding and fixing.\n\nThanks,\nTaylor\n"},{"id":"504009","messageId":"Zv3syfwZykRtJYQX@nand.local","threadId":"62244","inReplyTo":"20241003002140.GB3486271@coredump.intra.peff.net","subject":"Re: [PATCH (v2.47 regression)] hash.h: set NEEDS_CLONE_HELPER_UNSAFE in fallback mode","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2024-10-03T01:00:57Z","receivedAt":"2024-10-03T01:01:00Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Wed, Oct 02, 2024 at 08:21:40PM -0400, Jeff King wrote:\n> On Thu, Oct 03, 2024 at 12:13:47AM +0000, brian m. carlson wrote:\n>\n> > On 2024-10-02 at 23:26:18, Jeff King wrote:\n> > > This is a regression in v2.47.0-rc0. As mentioned above, I kind of doubt\n> > > anybody will hit it in practice (I only did because I was trying to do\n> > > some timing tests between the fast and dc variants). And it is almost\n> > > tempting to leave it as a wake-up call for anybody who is still not\n> > > using a collision-detecting sha1. ;)\n> >\n> > I think this is a fine fix for 2.47. I have a branch on my remote\n> > (sha1-dc-only), which I'll send out after it passes CI (probably later\n> > this week), that removes support for the everything but SHA-1-DC (except\n> > for the unsafe code).\n> >\n> > I don't think there's a reasonable configuration where people can use\n> > Git with other SHA-1 code except in extremely limited circumstances we\n> > shouldn't have to maintain code for. The code is open source, so people\n> > who really must have maximum performance with all of the vulnerabilities\n> > can patch it back in themselves.\n>\n> Yeah, I feel the same way. I only happened to try this because it was\n> the easiest way to speed-compare different implementations using\n> \"test-tool sha1\". ;)\n\nI imagine that you both mean that non-collision detecting variants are\nunsuitable for the \"safe\" SHA-1 implementation, and that the \"unsafe\"\nvariant can still be driven with BLK_SHA1, OpenSSL, etc.\n\nAnd reading the patch at the tip of brian's 'sha1-dc-only' branch, that\nlooks to be the case. So I'm in agreement with the both of you ;-).\n\n> Possibly that helper could grow an option to use the unsafe variant,\n> though even that is probably not a high priority.\n\nYeah, that would be nice. Though I agree it's not a huge priority.\n\nThanks,\nTaylor\n"},{"id":"504037","messageId":"xmqq8qv5hwv0.fsf@gitster.g","threadId":"62244","inReplyTo":"Zv3r9FBd+Nog3wb7@nand.local","subject":"Re: [PATCH (v2.47 regression)] hash.h: set NEEDS_CLONE_HELPER_UNSAFE in fallback mode","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-10-03T18:20:19Z","receivedAt":"2024-10-03T18:20:22Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Taylor Blau writes:\n\n> On Wed, Oct 02, 2024 at 07:26:18PM -0400, Jeff King wrote:\n>> diff --git a/hash.h b/hash.h\n>> index f97f858307..756166ce5e 100644\n>> --- a/hash.h\n>> +++ b/hash.h\n>> @@ -82,6 +82,9 @@\n>> # ifdef platform_SHA1_Clone\n>> # define platform_SHA1_Clone_unsafe platform_SHA1_Clone\n>> # endif\n>> +# ifdef SHA1_NEEDS_CLONE_HELPER\n>> +# define SHA1_NEEDS_CLONE_HELPER_UNSAFE\n>> +# endif\n>> #endif\n>\n> Gah. I could have sworn I wrote something like this myself, but I must\n> have dropped it or hallucinated writing it in the first place.\n>\n> Thanks for finding and fixing.\n\nYeah, thanks, all.\n"}]}