Re: [PATCH 2/2] avoid SHA-1 functions deprecated in OpenSSL 3+
Eric Wong <e@80x24.org> writes:
Show 19 quoted lines
> diff --git a/hash-ll.h b/hash-ll.h
> index 087b421bd5..10d84cc208 100644
> --- a/hash-ll.h
> +++ b/hash-ll.h
> @@ -45,6 +49,10 @@
> #define git_SHA1_Update platform_SHA1_Update
> #define git_SHA1_Final platform_SHA1_Final
>
> +#ifdef platform_SHA1_Clone
> +#define git_SHA1_Clone platform_SHA1_Clone
> +#endif
> +
> ...
> +#ifndef SHA1_NEEDS_CLONE_HELPER
> static inline void git_SHA1_Clone(git_SHA_CTX *dst, const git_SHA_CTX *src)
> {
> memcpy(dst, src, sizeof(*dst));
> }
> +#endifThis smelled a bit strange in that all the other platform_* stuff is "if a platform sha-1 header implements platform_SHA1_*, we will use it to define git_SHA1_* (which is the symbol we use in the code)" plus its inverse "if there is no specific platform_SHA1_*, we assume OpenSSL compatible ones and use them as platform_SHA1_* (which in turn will be used as git_SHA1-*)".
And that is why "#ifndef platform_SHA1_CTX" block gave us default values for them. And from that point of view, the first hunk (i.e. "if SHA1_CLONE is defined for the platform, we use it") is entirely sensible.
But I did not get why we guard the other hunk with a different CPP macro. If we have platform_SHA1_Clone already defined, and then NEEDS_CLONE_HELPER not defined, we end up creating an static inline platform_SHA1_CLONE here, and I was not sure if that is what we wanted to do.
The answer to the above puzzle (at least it was a puzzle to me) is that the new header "sha1/openssl.h" added by this series does have platform_SHA1_Clone defined, and the code that includes it define NEEDS_CLONE_HELPER to avoid this "static inline", so the CPP macro SHA1_NEEDS_CLONE_HELPER means "we need more than just a straight bitwise copy to clone the SHA context, which is provided elsewhere in the form of platform_SHA1_Clone".
So everything evens out. If we are with newer OpenSSL, we will include sha1/openssl.h and get both platform_SHA1_Clone and SHA1_NEEDS_CLONE_HELPER defined. If we are with older OpenSSL or non-OpenSSL, we do not get platform_SHA1_Clone (because the "#ifndef platform_SHA1_CTX" block does not have a fallback default defined) and we do not get SHA1_NEEDS_CLONE_HELPER either. We either use the memcpy() fallback only when we are not working with newer OpenSSL or whatever defines its own platform_SHA1_Clone. So the patch smelled a bit strange, but there isn't anything incorrect per-se.
But then is this making folks unnecessary work when they add non-OpenSSL support that needs more than just memcpy() to clone the context? What breaks if we turn these two hunks into
#ifdef platform_SHA1_Clone
#define git_SHA1_Clone platform_SHA1_Clone
#else
static inline void git_SHA1_Clone(git_SHA_CTX *dst, git_SHA_CTX *src)
{
memcpy(dst, src, sizeof(*dst));
}
#endifand drop the requirement that they must define SHA1_NEEDS_CLONE_HELPER if they want to define their own platform_SHA1_Clone()?
Thanks. Everything else in the patch made sense (even though I am not familiar with the EVP API) to me.
