Re: [PATCH] Limit the size of the data block passed to SHA1_Update()

From: Junio C Hamano <gitster@pobox.com>
Date: Oct 30, 2015, 22:22 UTC
Message-ID: <xmqqa8r0j8tu.fsf@gitster.mtv.corp.google.com>
In-Reply-To: <1446243122-21464-1-git-send-email-apahlevan@ieee.org>

Atousa Pahlevan Duprat <atousa.p@gmail.com> writes:

Show 5 quoted lines

> Some implementations of SHA_Updates have inherent limits
> on the max chunk size. SHA1_MAX_BLOCK_SIZE can be defined
> to set the max chunk size supported, if required.  This is
> enabled for OSX CommonCrypto library and set to 1GiB.
> ---

Missing sign-off.

Show 71 quoted lines

>  Makefile                     |  9 +++++++++
>  cache.h                      |  7 ++++++-
>  compat/apple-common-crypto.h |  4 ++++
>  compat/sha1_chunked.c        | 20 ++++++++++++++++++++
>  4 files changed, 39 insertions(+), 1 deletion(-)
>  create mode 100644 compat/sha1_chunked.c
>
> diff --git a/Makefile b/Makefile
> index 04c2231..5955542 100644
> --- a/Makefile
> +++ b/Makefile
> @@ -141,6 +141,10 @@ all::
>  # Define PPC_SHA1 environment variable when running make to make use of
>  # a bundled SHA1 routine optimized for PowerPC.
>  #
> +# Define SHA1_MAX_BLOCK_SIZE if your SSH1_Update() implementation can
> +# hash only a limited amount of data in one call (e.g. APPLE_COMMON_CRYPTO
> +# may want 'SHA1_MAX_BLOCK_SIZE=1024L*1024L*1024L' defined).
> +#
>  # Define NEEDS_CRYPTO_WITH_SSL if you need -lcrypto when using -lssl (Darwin).
>  #
>  # Define NEEDS_SSL_WITH_CRYPTO if you need -lssl when using -lcrypto (Darwin).
> @@ -1346,6 +1350,7 @@ else
>  ifdef APPLE_COMMON_CRYPTO
>  	COMPAT_CFLAGS += -DCOMMON_DIGEST_FOR_OPENSSL
>  	SHA1_HEADER = <CommonCrypto/CommonDigest.h>
> +	SHA1_MAX_BLOCK_SIZE = 1024L*1024L*1024L
>  else
>  	SHA1_HEADER = <openssl/sha.h>
>  	EXTLIBS += $(LIB_4_CRYPTO)
> @@ -1353,6 +1358,10 @@ endif
>  endif
>  endif
>  
> +ifdef SHA1_MAX_BLOCK_SIZE
> +	LIB_OBJS += compat/sha1_chunked.o
> +	BASIC_CFLAGS += -DSHA1_MAX_BLOCK_SIZE="$(SHA1_MAX_BLOCK_SIZE)"
> +endif
>  ifdef NO_PERL_MAKEMAKER
>  	export NO_PERL_MAKEMAKER
>  endif
> diff --git a/cache.h b/cache.h
> index 79066e5..ec84b16 100644
> --- a/cache.h
> +++ b/cache.h
> @@ -14,7 +14,12 @@
>  #ifndef git_SHA_CTX
>  #define git_SHA_CTX	SHA_CTX
>  #define git_SHA1_Init	SHA1_Init
> -#define git_SHA1_Update	SHA1_Update
> +#ifdef SHA1_MAX_BLOCK_SIZE
> +extern int SHA1_Update_Chunked(SHA_CTX *, const void *, size_t);
> +#define git_SHA1_Update SHA1_Update_Chunked
> +#else
> +#define git_SHA1_Update SHA1_Update
> +#endif
>  #define git_SHA1_Final	SHA1_Final
>  #endif
>  
> diff --git a/compat/apple-common-crypto.h b/compat/apple-common-crypto.h
> index c8b9b0e..83668fd 100644
> --- a/compat/apple-common-crypto.h
> +++ b/compat/apple-common-crypto.h
> @@ -16,6 +16,10 @@
>  #undef TYPE_BOOL
>  #endif
>  
> +#ifndef SHA1_MAX_BLOCK_SIZE
> +#error "Using Apple Common Crypto library requires setting SHA1_MAX_BLOCK_SIZE"
> +#endif
> +

It crossed my mind if this might be better to just define it to some reasonable value instead of erroring out, but because we do give a default value in the Makefile, it would be a sign that the user is doing something _quite_ unusual if the symbol is not defined here, so I agree with your decision to error it out here.

Show 17 quoted lines

>  #ifdef APPLE_LION_OR_NEWER
>  #define git_CC_error_check(pattern, err) \
>  	do { \
> diff --git a/compat/sha1_chunked.c b/compat/sha1_chunked.c
> new file mode 100644
> index 0000000..4a8e4f7
> --- /dev/null
> +++ b/compat/sha1_chunked.c
> @@ -0,0 +1,20 @@
> +#include "cache.h"
> +
> +#ifdef SHA1_MAX_BLOCK_SIZE
> +int git_SHA1_Update(SHA_CTX *c, const void *data, size_t len)
> +{
> +	size_t nr;
> +	size_t total = 0;
> +	char *cdata = (char*)data;

Please have a single blank line between the decls at the beginning of a function and its first statement. I am not sure about the cast here, though. Doesn't the function SHA1_Update() you are going to call in the body of the loop take "const void *" as its second parameter? That's how openssl/sha1.h and block-sha1/sha1.h declare this function.

> +	while(len > 0) {
> +		nr = len;
> +		if(nr > SHA1_MAX_BLOCK_SIZE)

Please have a SP around () for control statements, like while, switch and if.

Show 9 quoted lines

> +			nr = SHA1_MAX_BLOCK_SIZE;
> +		SHA1_Update(c, cdata, nr);
> +		total += nr;
> +		cdata += nr;
> +		len -= nr;
> +	}
> +	return total;
> +}
> +#endif

Thanks.