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

From: Eric Sunshine <sunshine@sunshineco.com>
Date: Nov 4, 2015, 19:51 UTC
Message-ID: <CAPig+cS7NktyhveQUiBFkRtJH1D-Aif861e9qshL2R=ZNg2+Lw@mail.gmail.com>
In-Reply-To: <1446611086-6102-1-git-send-email-apahlevan@ieee.org>

On Tuesday, November 3, 2015, <atousa.p@gmail.com> wrote:

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

As an aid for reviewers, please include the version number of the patch, such as [PATCH vN] where "N" is the revision. format-patch's -v option can help automate this.

Show 7 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.
>
> Signed-off-by: Atousa Pahlevan Duprat <apahlevan@ieee.org>
> ---

It is helpful to reviewers if you can describe here, below the "---" line, how this version of the patch differs from the previous one. More below...

Show 25 quoted lines

> diff --git a/Makefile b/Makefile
> @@ -136,11 +136,15 @@ all::
>  # to provide your own OpenSSL library, for example from MacPorts.
>  #
>  # Define BLK_SHA1 environment variable to make use of the bundled
> -# optimized C SHA1 routine.
> +# optimized C SHA1 routine.  This implies NO_APPLE_COMMON_CRYPTO.
>  #
>  # 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).
> @@ -986,6 +990,10 @@ ifeq (no,$(USE_PARENS_AROUND_GETTEXT_N))
>  endif
>  endif
>
> +ifdef BLK_SHA1
> +       NO_APPLE_COMMON_CRYPTO=1
> +endif

I think Junio already mentioned[1] that this change (and its corresponding documentation update above) likely is undesirable, but I just wanted to mention that you would typically want to split such a change out to a separate patch since it's unrelated to the overall intention of the current patch. More below...

[1]: http://article.gmane.org/gmane.comp.version-control.git/280859

Show 48 quoted lines

>  ifeq ($(uname_S),Darwin)
>         ifndef NO_FINK
>                 ifeq ($(shell test -d /sw/lib && echo y),y)
> @@ -1346,6 +1354,8 @@ else
>  ifdef APPLE_COMMON_CRYPTO
>         COMPAT_CFLAGS += -DCOMMON_DIGEST_FOR_OPENSSL
>         SHA1_HEADER = <CommonCrypto/CommonDigest.h>
> +       # Apple CommonCrypto requires chunking
> +       SHA1_MAX_BLOCK_SIZE = 1024L*1024L*1024L
>  else
>         SHA1_HEADER = <openssl/sha.h>
>         EXTLIBS += $(LIB_4_CRYPTO)
> @@ -1353,6 +1363,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
> diff --git a/block-sha1/sha1.h b/block-sha1/sha1.h
> @@ -18,5 +18,5 @@ void blk_SHA1_Final(unsigned char hashout[20], blk_SHA_CTX *ctx);
>
>  #define git_SHA_CTX    blk_SHA_CTX
>  #define git_SHA1_Init  blk_SHA1_Init
> -#define git_SHA1_Update        blk_SHA1_Update
> +#define platform_SHA1_Update   blk_SHA1_Update
>  #define git_SHA1_Final blk_SHA1_Final
> diff --git a/cache.h b/cache.h
> index 79066e5..e345e38 100644
> --- a/cache.h
> +++ b/cache.h
> @@ -10,12 +10,21 @@
>  #include "trace.h"
>  #include "string-list.h"
>
> +/* platform's underlying implementation of SHA1 */
>  #include SHA1_HEADER
>  #ifndef git_SHA_CTX
> -#define git_SHA_CTX    SHA_CTX
> -#define git_SHA1_Init  SHA1_Init
> -#define git_SHA1_Update        SHA1_Update
> -#define git_SHA1_Final SHA1_Final
> +#define git_SHA_CTX            SHA_CTX
> +#define git_SHA1_Init          SHA1_Init
> +#define platform_SHA1_Update   SHA1_Update
> +#define git_SHA1_Final         SHA1_Final
> +#endif

It's a bit ugly that this special-cases only "update" with the "platform_" abstraction. It _might_ be preferable to generalize this for all the SHA1 operations. If so, that's something you'd want to do as a separate preparatory patch specifically aimed at adding this new abstraction layer.

(In fact, even if you decide only to special-case "update", that still might deserve a separate preparatory patch since it's a conceptually distinct change from the overall focus of this patch, and would make this patch less noisy, thus easier to review.)

Show 7 quoted lines

> +/* choose chunked implementation or not */
> +#ifdef SHA1_MAX_BLOCK_SIZE
> +int git_SHA1_Update_Chunked(SHA_CTX *c, const void *data, size_t len);
> +#define git_SHA1_Update       git_SHA1_Update_Chunked
> +#else
> +#define git_SHA1_Update       platform_SHA1_Update
>  #endif