| Message ID | 20230801025454.1137802-3-e@80x24.org (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | bda9c12073e786e2ffa2c3ec479c7fe098d49999 |
| Headers | show |
| Series | avoid functions deprecated in OpenSSL 3+ | expand |
Eric Wong <e@80x24.org> writes: > 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)); > } > +#endif This 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)); } #endif and 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.
Junio C Hamano <gitster@pobox.com> wrote: > Eric Wong <e@80x24.org> writes: > > > 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)); > > } > > +#endif > > This 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)); > } > #endif > > and drop the requirement that they must define SHA1_NEEDS_CLONE_HELPER > if they want to define their own platform_SHA1_Clone()? I just copied the existing SHA256 stuff and mostly did a s/SHA256/SHA1/ in patch 2/2. I'm not sure why SHA256_NEEDS_CLONE_HELPER was needed, either, but I decided to keep the SHA1 and SHA256 code as similar as possible for consistency. We could probably drop both *_NEEDS_CLONE_HELPER macros, but that's a separate patch.
Eric Wong <e@80x24.org> writes: > I just copied the existing SHA256 stuff and mostly did a > s/SHA256/SHA1/ in patch 2/2. I'm not sure why > SHA256_NEEDS_CLONE_HELPER was needed, either, but I decided > to keep the SHA1 and SHA256 code as similar as possible for > consistency. > > We could probably drop both *_NEEDS_CLONE_HELPER macros, > but that's a separate patch. Fair enough. Thanks.
diff --git a/Makefile b/Makefile index a499c5d7f2..ace3e5a506 100644 --- a/Makefile +++ b/Makefile @@ -3216,6 +3216,9 @@ $(SP_OBJ): %.sp: %.c %.o sparse: $(SP_OBJ) EXCEPT_HDRS := $(GENERATED_H) unicode-width.h compat/% xdiff/% +ifndef OPENSSL_SHA1 + EXCEPT_HDRS += sha1/openssl.h +endif ifndef OPENSSL_SHA256 EXCEPT_HDRS += sha256/openssl.h endif diff --git a/hash-ll.h b/hash-ll.h index 087b421bd5..10d84cc208 100644 --- a/hash-ll.h +++ b/hash-ll.h @@ -4,7 +4,11 @@ #if defined(SHA1_APPLE) #include <CommonCrypto/CommonDigest.h> #elif defined(SHA1_OPENSSL) -#include <openssl/sha.h> +# include <openssl/sha.h> +# if defined(OPENSSL_API_LEVEL) && OPENSSL_API_LEVEL >= 3 +# define SHA1_NEEDS_CLONE_HELPER +# include "sha1/openssl.h" +# endif #elif defined(SHA1_DC) #include "sha1dc_git.h" #else /* SHA1_BLK */ @@ -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 platform_SHA256_CTX #define platform_SHA256_CTX SHA256_CTX #define platform_SHA256_Init SHA256_Init @@ -67,10 +75,12 @@ #define git_SHA1_Update git_SHA1_Update_Chunked #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)); } +#endif #ifndef SHA256_NEEDS_CLONE_HELPER static inline void git_SHA256_Clone(git_SHA256_CTX *dst, const git_SHA256_CTX *src) diff --git a/sha1/openssl.h b/sha1/openssl.h new file mode 100644 index 0000000000..006c1f4ba5 --- /dev/null +++ b/sha1/openssl.h @@ -0,0 +1,49 @@ +/* wrappers for the EVP API of OpenSSL 3+ */ +#ifndef SHA1_OPENSSL_H +#define SHA1_OPENSSL_H +#include <openssl/evp.h> + +struct openssl_SHA1_CTX { + EVP_MD_CTX *ectx; +}; + +typedef struct openssl_SHA1_CTX openssl_SHA1_CTX; + +static inline void openssl_SHA1_Init(struct openssl_SHA1_CTX *ctx) +{ + const EVP_MD *type = EVP_sha1(); + + ctx->ectx = EVP_MD_CTX_new(); + if (!ctx->ectx) + die("EVP_MD_CTX_new: out of memory"); + + EVP_DigestInit_ex(ctx->ectx, type, NULL); +} + +static inline void openssl_SHA1_Update(struct openssl_SHA1_CTX *ctx, + const void *data, + size_t len) +{ + EVP_DigestUpdate(ctx->ectx, data, len); +} + +static inline void openssl_SHA1_Final(unsigned char *digest, + struct openssl_SHA1_CTX *ctx) +{ + EVP_DigestFinal_ex(ctx->ectx, digest, NULL); + EVP_MD_CTX_free(ctx->ectx); +} + +static inline void openssl_SHA1_Clone(struct openssl_SHA1_CTX *dst, + const struct openssl_SHA1_CTX *src) +{ + EVP_MD_CTX_copy_ex(dst->ectx, src->ectx); +} + +#define platform_SHA_CTX openssl_SHA1_CTX +#define platform_SHA1_Init openssl_SHA1_Init +#define platform_SHA1_Clone openssl_SHA1_Clone +#define platform_SHA1_Update openssl_SHA1_Update +#define platform_SHA1_Final openssl_SHA1_Final + +#endif /* SHA1_OPENSSL_H */
OpenSSL 3+ deprecates the SHA1_Init, SHA1_Update, and SHA1_Final functions, leading to errors when building with `DEVELOPER=1'. Use the newer EVP_* API with OpenSSL 3+ (only) despite being more error-prone and less efficient due to heap allocations. Signed-off-by: Eric Wong <e@80x24.org> --- Makefile | 3 +++ hash-ll.h | 12 +++++++++++- sha1/openssl.h | 49 +++++++++++++++++++++++++++++++++++++++++++++++++ 3 files changed, 63 insertions(+), 1 deletion(-) create mode 100644 sha1/openssl.h