On Tuesday, November 3, 2015, <atousa.p@xxxxxxxxx> 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. > 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@xxxxxxxx> > --- 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... > 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 > 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.) > +/* 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 -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html