# git fsck failure on OS X with files >= 4 GiB 27 messages from 2015-10-28 to 2015-11-04. Participants: Rafael Espíndola, Filipe Cabecinhas, Atousa Duprat, Junio C Hamano, Atousa Pahlevan Duprat, Randall S. Becker, Eric Sunshine, atousa.p@gmail.com, Torsten Bögershausen. Thread: https://gitlist.dev/t/40669 ## Rafael Espíndola, 2015-10-28 23:10 Subject: git fsck failure on OS X with files >= 4 GiB Message-ID: URL: https://gitlist.dev/e/CAG3jReJn2Pz6-bXLw6baOZaE1BHYiC%2B1-zN0eagigfG3umWpJA%40mail.gmail.com ``` I first noticed this with "2.4.9 (Apple Git-60)", but it reproduces with git built from 37023ba381b6d251d7140a997b39b566dbc63c42. Create two files with just 0s: -rw-r--r-- 1 espindola staff 4294967296 28 Oct 11:09 exactly-4gib -rw-r--r-- 1 espindola staff 4294967295 28 Oct 11:09 one-less-than-4gib and run git init git add one-less-than-4gib git commit -m bar git fsck git add exactly-4gib git commit -m bar git fsck The first fsck will run with no problems, but the second one fails: error: packed cfdaf54c9ccfd8f5e4cee562f7d5f92df13d3106 from .git/objects/pack/pack-ff08480fd7f767b6bd0aeb559f0f5dea2245b0b3.pack is corrupt Using the very same revision on freebsd doesn't cause any errors. Cheers, Rafael ``` ## Filipe Cabecinhas, 2015-10-29 06:46 Subject: Re: git fsck failure on OS X with files >= 4 GiB Message-ID: URL: https://gitlist.dev/e/CAEDE853qg_J_asKf4SsE7mB8L7E0Tb3ReJnNWDfAfXfPw%2BtjhQ%40mail.gmail.com In-Reply-To: ``` I did some debugging, and it seems CC_SHA1_Update (used by write_sha1_file_prepare if APPLE_COMMON_CRYPTO is defined in the Makefile) takes a uint32_t as a "length" parameter, which explains why it stops working at 4GiB (UINT_MAX+1). In the OS X 10.11 SDK header CommonCrypto/CommonDigest.h, we have: typedef uint32_t CC_LONG; /* 32 bit unsigned integer */ //... extern int CC_SHA1_Update(CC_SHA1_CTX *c, const void *data, CC_LONG len) A possible fix would be to either call SHA1_Update with a maximum of UINT_MAX, looping if necessary. Or have a compatibility SHA1_Update for OS X which can handle data longer than UINT_MAX. I'm not sure what the git maintainers would prefer. Regards, Filipe On Wed, Oct 28, 2015 at 4:10 PM, Rafael Espíndola wrote: > > I first noticed this with "2.4.9 (Apple Git-60)", but it reproduces > with git built from 37023ba381b6d251d7140a997b39b566dbc63c42. > > Create two files with just 0s: > > -rw-r--r-- 1 espindola staff 4294967296 28 Oct 11:09 exactly-4gib > -rw-r--r-- 1 espindola staff 4294967295 28 Oct 11:09 one-less-than-4gib > > > and run > > git init > git add one-less-than-4gib > git commit -m bar > git fsck > git add exactly-4gib > git commit -m bar > git fsck > > The first fsck will run with no problems, but the second one fails: > > error: packed cfdaf54c9ccfd8f5e4cee562f7d5f92df13d3106 from > .git/objects/pack/pack-ff08480fd7f767b6bd0aeb559f0f5dea2245b0b3.pack > is corrupt > > Using the very same revision on freebsd doesn't cause any errors. > > Cheers, > Rafael ``` ## Rafael Espíndola, 2015-10-29 10:46 Subject: Re: git fsck failure on OS X with files >= 4 GiB Message-ID: URL: https://gitlist.dev/e/CAG3jRe%2B23sy1k9QNdpdn3GF3nbzPMmYO%3DTM%3DSufEq83OtwNxbA%40mail.gmail.com In-Reply-To: ``` Awesome, building with NO_OPENSSL = 1 NO_GETTEXT = 1 produces a working git :-) Cheers, Rafael On 28 October 2015 at 23:37, Filipe Cabecinhas wrote: > I did some debugging, and it seems CC_SHA1_Update (used by > write_sha1_file_prepare if APPLE_COMMON_CRYPTO is defined in the Makefile) > takes a uint32_t as a "length" parameter, which explains why it stops > working at 4GiB (UINT_MAX+1). > > In the OS X 10.11 SDK header CommonCrypto/CommonDigest.h, we have: > > typedef uint32_t CC_LONG; /* 32 bit unsigned integer */ > //... > extern int CC_SHA1_Update(CC_SHA1_CTX *c, const void *data, CC_LONG len) > > A possible fix would be to either call SHA1_Update with a maximum of > UINT_MAX, looping if necessary. Or have a compatibility SHA1_Update for OS X > which can handle data longer than UINT_MAX. > > I'm not sure what the git maintainers would prefer. > > Regards, > > Filipe > > On Wed, Oct 28, 2015 at 4:10 PM, Rafael Espíndola > wrote: >> >> I first noticed this with "2.4.9 (Apple Git-60)", but it reproduces >> with git built from 37023ba381b6d251d7140a997b39b566dbc63c42. >> >> Create two files with just 0s: >> >> -rw-r--r-- 1 espindola staff 4294967296 28 Oct 11:09 exactly-4gib >> -rw-r--r-- 1 espindola staff 4294967295 28 Oct 11:09 one-less-than-4gib >> >> >> and run >> >> git init >> git add one-less-than-4gib >> git commit -m bar >> git fsck >> git add exactly-4gib >> git commit -m bar >> git fsck >> >> The first fsck will run with no problems, but the second one fails: >> >> error: packed cfdaf54c9ccfd8f5e4cee562f7d5f92df13d3106 from >> .git/objects/pack/pack-ff08480fd7f767b6bd0aeb559f0f5dea2245b0b3.pack >> is corrupt >> >> Using the very same revision on freebsd doesn't cause any errors. >> >> Cheers, >> Rafael > > ``` ## Filipe Cabecinhas, 2015-10-29 15:15 Subject: Re: git fsck failure on OS X with files >= 4 GiB Message-ID: URL: https://gitlist.dev/e/CAEDE853n2HR-SK9_sGn8n3j0xoTb3eQa86UvdW0DBrc%2BZ1sDrg%40mail.gmail.com In-Reply-To: ``` Defining BLK_SHA1 = YesPlease (when calling make) should just change the SHA functions, instead of completely removing OpenSSL or CommonCrypto. Regards, Filipe On Thu, Oct 29, 2015 at 3:46 AM, Rafael Espíndola wrote: > Awesome, building with > > NO_OPENSSL = 1 > NO_GETTEXT = 1 > > produces a working git :-) > > Cheers, > Rafael > > > On 28 October 2015 at 23:37, Filipe Cabecinhas wrote: >> I did some debugging, and it seems CC_SHA1_Update (used by >> write_sha1_file_prepare if APPLE_COMMON_CRYPTO is defined in the Makefile) >> takes a uint32_t as a "length" parameter, which explains why it stops >> working at 4GiB (UINT_MAX+1). >> >> In the OS X 10.11 SDK header CommonCrypto/CommonDigest.h, we have: >> >> typedef uint32_t CC_LONG; /* 32 bit unsigned integer */ >> //... >> extern int CC_SHA1_Update(CC_SHA1_CTX *c, const void *data, CC_LONG len) >> >> A possible fix would be to either call SHA1_Update with a maximum of >> UINT_MAX, looping if necessary. Or have a compatibility SHA1_Update for OS X >> which can handle data longer than UINT_MAX. >> >> I'm not sure what the git maintainers would prefer. >> >> Regards, >> >> Filipe >> >> On Wed, Oct 28, 2015 at 4:10 PM, Rafael Espíndola >> wrote: >>> >>> I first noticed this with "2.4.9 (Apple Git-60)", but it reproduces >>> with git built from 37023ba381b6d251d7140a997b39b566dbc63c42. >>> >>> Create two files with just 0s: >>> >>> -rw-r--r-- 1 espindola staff 4294967296 28 Oct 11:09 exactly-4gib >>> -rw-r--r-- 1 espindola staff 4294967295 28 Oct 11:09 one-less-than-4gib >>> >>> >>> and run >>> >>> git init >>> git add one-less-than-4gib >>> git commit -m bar >>> git fsck >>> git add exactly-4gib >>> git commit -m bar >>> git fsck >>> >>> The first fsck will run with no problems, but the second one fails: >>> >>> error: packed cfdaf54c9ccfd8f5e4cee562f7d5f92df13d3106 from >>> .git/objects/pack/pack-ff08480fd7f767b6bd0aeb559f0f5dea2245b0b3.pack >>> is corrupt >>> >>> Using the very same revision on freebsd doesn't cause any errors. >>> >>> Cheers, >>> Rafael >> >> ``` ## Atousa Duprat, 2015-10-29 16:02 Subject: Re: git fsck failure on OS X with files >= 4 GiB Message-ID: URL: https://gitlist.dev/e/CA%2BizobtdwszVrYsnKU%3D_ytLuNbPGyRe_7kXqyrQO7u5Lo%2BOdPg%40mail.gmail.com In-Reply-To: ``` Here is a solution that avoids problems with OS-specific implementations of SHA_Update() by limiting the size of each update request to 1GiB and calling the function repeatedly in a loop. Atousa -- [PATCH] Limit the size of the data block passed to SHA1_Update() This avoids issues where OS-specific implementations use a 32-bit integer to specify block size. Limit currently set to 1GiB. --- cache.h | 20 +++++++++++++++++++- 1 file changed, 19 insertions(+), 1 deletion(-) diff --git a/cache.h b/cache.h index 79066e5..c305985 100644 --- a/cache.h +++ b/cache.h @@ -14,10 +14,28 @@ #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 #endif +#define SHA1_MAX_BLOCK_SIZE (1024*1024*1024) + +static inline int git_SHA1_Update(SHA_CTX *c, const void *data, size_t len) +{ + size_t nr; + size_t total = 0; + char *cdata = (char*)data; + while(len > 0) { + nr = len; + if(nr > SHA1_MAX_BLOCK_SIZE) + nr = SHA1_MAX_BLOCK_SIZE; + SHA1_Update(c, cdata, nr); + total += nr; + cdata += nr; + len -= nr; + } + return total; +} + #include typedef struct git_zstream { z_stream z; -- 2.4.9 (Apple Git-60) On Thu, Oct 29, 2015 at 8:15 AM, Filipe Cabecinhas wrote: > Defining BLK_SHA1 = YesPlease (when calling make) should just change > the SHA functions, instead of completely removing OpenSSL or > CommonCrypto. > > Regards, > Filipe > > > On Thu, Oct 29, 2015 at 3:46 AM, Rafael Espíndola > wrote: >> Awesome, building with >> >> NO_OPENSSL = 1 >> NO_GETTEXT = 1 >> >> produces a working git :-) >> >> Cheers, >> Rafael >> >> >> On 28 October 2015 at 23:37, Filipe Cabecinhas wrote: >>> I did some debugging, and it seems CC_SHA1_Update (used by >>> write_sha1_file_prepare if APPLE_COMMON_CRYPTO is defined in the Makefile) >>> takes a uint32_t as a "length" parameter, which explains why it stops >>> working at 4GiB (UINT_MAX+1). >>> >>> In the OS X 10.11 SDK header CommonCrypto/CommonDigest.h, we have: >>> >>> typedef uint32_t CC_LONG; /* 32 bit unsigned integer */ >>> //... >>> extern int CC_SHA1_Update(CC_SHA1_CTX *c, const void *data, CC_LONG len) >>> >>> A possible fix would be to either call SHA1_Update with a maximum of >>> UINT_MAX, looping if necessary. Or have a compatibility SHA1_Update for OS X >>> which can handle data longer than UINT_MAX. >>> >>> I'm not sure what the git maintainers would prefer. >>> >>> Regards, >>> >>> Filipe >>> >>> On Wed, Oct 28, 2015 at 4:10 PM, Rafael Espíndola >>> wrote: >>>> >>>> I first noticed this with "2.4.9 (Apple Git-60)", but it reproduces >>>> with git built from 37023ba381b6d251d7140a997b39b566dbc63c42. >>>> >>>> Create two files with just 0s: >>>> >>>> -rw-r--r-- 1 espindola staff 4294967296 28 Oct 11:09 exactly-4gib >>>> -rw-r--r-- 1 espindola staff 4294967295 28 Oct 11:09 one-less-than-4gib >>>> >>>> >>>> and run >>>> >>>> git init >>>> git add one-less-than-4gib >>>> git commit -m bar >>>> git fsck >>>> git add exactly-4gib >>>> git commit -m bar >>>> git fsck >>>> >>>> The first fsck will run with no problems, but the second one fails: >>>> >>>> error: packed cfdaf54c9ccfd8f5e4cee562f7d5f92df13d3106 from >>>> .git/objects/pack/pack-ff08480fd7f767b6bd0aeb559f0f5dea2245b0b3.pack >>>> is corrupt >>>> >>>> Using the very same revision on freebsd doesn't cause any errors. >>>> >>>> Cheers, >>>> Rafael >>> >>> > -- > To unsubscribe from this list: send the line "unsubscribe git" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html -- Atousa Pahlevan, PhD M.Math. University of Waterloo, Canada Ph.D. Department of Computer Science, University of Victoria, Canada Voice: 415-341-6206 Email: apahlevan@ieee.org Website: www.apahlevan.org ``` ## Junio C Hamano, 2015-10-29 17:19 Subject: Re: git fsck failure on OS X with files >= 4 GiB Message-ID: URL: https://gitlist.dev/e/xmqqlhalsict.fsf%40gitster.mtv.corp.google.com In-Reply-To: ``` Atousa Duprat writes: > [PATCH] Limit the size of the data block passed to SHA1_Update() > > This avoids issues where OS-specific implementations use > a 32-bit integer to specify block size. Limit currently > set to 1GiB. > --- > cache.h | 20 +++++++++++++++++++- > 1 file changed, 19 insertions(+), 1 deletion(-) > > diff --git a/cache.h b/cache.h > index 79066e5..c305985 100644 > --- a/cache.h > +++ b/cache.h > @@ -14,10 +14,28 @@ > #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 > #endif > > +#define SHA1_MAX_BLOCK_SIZE (1024*1024*1024) > + > +static inline int git_SHA1_Update(SHA_CTX *c, const void *data, size_t len) > +{ > + size_t nr; > + size_t total = 0; > + char *cdata = (char*)data; > + while(len > 0) { > + nr = len; > + if(nr > SHA1_MAX_BLOCK_SIZE) > + nr = SHA1_MAX_BLOCK_SIZE; > + SHA1_Update(c, cdata, nr); > + total += nr; > + cdata += nr; > + len -= nr; > + } > + return total; > +} > + I think the idea illustrated above is a good start, but there are a few issues: * SHA1_Update() is used in fairly many places; it is unclear if it is a good idea to inline. * There is no need to punish implementations with working SHA1_Update by another level of wrapping. * What would you do when you find an implementation for which 1G is still too big? Perhaps something like this in the header #ifdef SHA1_MAX_BLOCK_SIZE extern int SHA1_Update_Chunked(SHA_CTX *, const void *, size_t); #define git_SHA1_Update SHA1_Update_Chunked #endif with compat/sha1_chunked.c that has #ifdef SHA1_MAX_BLOCK_SIZE int SHA1_Update_Chunked(SHA_CTX *c, const void *data, size_t len) { ... your looping implementation ... } #endif in it, that is only triggered via a Makefile macro, e.g. might be a good workaround. diff --git a/Makefile b/Makefile index 8466333..83348b8 100644 --- a/Makefile +++ b/Makefile @@ -139,6 +139,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). @@ -1002,6 +1006,7 @@ ifeq ($(uname_S),Darwin) ifndef NO_APPLE_COMMON_CRYPTO APPLE_COMMON_CRYPTO = YesPlease COMPAT_CFLAGS += -DAPPLE_COMMON_CRYPTO + SHA1_MAX_BLOCK_SIZE=1024L*1024L*1024L endif NO_REGEX = YesPlease PTHREAD_LIBS = @@ -1350,6 +1355,11 @@ endif endif endif +ifdef SHA1_MAX_BLOCK_SIZE +LIB_OBJS += compat/sha1_chunked.o +BASIC_CFLAGS += SHA1_MAX_BLOCK_SIZE="$(SHA1_MAX_BLOCK_SIZE)" +endif + ifdef NO_PERL_MAKEMAKER export NO_PERL_MAKEMAKER endif ``` ## Atousa Duprat, 2015-10-30 02:15 Subject: Re: git fsck failure on OS X with files >= 4 GiB Message-ID: URL: https://gitlist.dev/e/CA%2BizobsBmYHHepYka795K2VnVLYBmN2tFqEyzSweMoS9gvuRVw%40mail.gmail.com In-Reply-To: ``` Thank you for the feedback. I have revised the proposed patch as suggested, allowing the use of SHA1_MAX_BLOCK_SIZE to enable the chunked implementation. When building for OSX with the CommonCrypto library we error out if SHA1_MAX_BLOCK_SIZE is not defined, which will avoid compiling a version of the tool that won't compute hashes properly on large files. It should be easy to enable this on other platforms if needed. Atousa On Thu, Oct 29, 2015 at 10:19 AM, Junio C Hamano wrote: > Atousa Duprat writes: > >> [PATCH] Limit the size of the data block passed to SHA1_Update() >> >> This avoids issues where OS-specific implementations use >> a 32-bit integer to specify block size. Limit currently >> set to 1GiB. >> --- >> cache.h | 20 +++++++++++++++++++- >> 1 file changed, 19 insertions(+), 1 deletion(-) >> >> diff --git a/cache.h b/cache.h >> index 79066e5..c305985 100644 >> --- a/cache.h >> +++ b/cache.h >> @@ -14,10 +14,28 @@ >> #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 >> #endif >> >> +#define SHA1_MAX_BLOCK_SIZE (1024*1024*1024) >> + >> +static inline int git_SHA1_Update(SHA_CTX *c, const void *data, size_t len) >> +{ >> + size_t nr; >> + size_t total = 0; >> + char *cdata = (char*)data; >> + while(len > 0) { >> + nr = len; >> + if(nr > SHA1_MAX_BLOCK_SIZE) >> + nr = SHA1_MAX_BLOCK_SIZE; >> + SHA1_Update(c, cdata, nr); >> + total += nr; >> + cdata += nr; >> + len -= nr; >> + } >> + return total; >> +} >> + > > I think the idea illustrated above is a good start, but there are > a few issues: > > * SHA1_Update() is used in fairly many places; it is unclear if it > is a good idea to inline. > > * There is no need to punish implementations with working > SHA1_Update by another level of wrapping. > > * What would you do when you find an implementation for which 1G is > still too big? > > Perhaps something like this in the header > > #ifdef SHA1_MAX_BLOCK_SIZE > extern int SHA1_Update_Chunked(SHA_CTX *, const void *, size_t); > #define git_SHA1_Update SHA1_Update_Chunked > #endif > > with compat/sha1_chunked.c that has > > #ifdef SHA1_MAX_BLOCK_SIZE > int SHA1_Update_Chunked(SHA_CTX *c, const void *data, size_t len) > { > ... your looping implementation ... > } > #endif > > in it, that is only triggered via a Makefile macro, e.g. > might be a good workaround. > > diff --git a/Makefile b/Makefile > index 8466333..83348b8 100644 > --- a/Makefile > +++ b/Makefile > @@ -139,6 +139,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). > @@ -1002,6 +1006,7 @@ ifeq ($(uname_S),Darwin) > ifndef NO_APPLE_COMMON_CRYPTO > APPLE_COMMON_CRYPTO = YesPlease > COMPAT_CFLAGS += -DAPPLE_COMMON_CRYPTO > + SHA1_MAX_BLOCK_SIZE=1024L*1024L*1024L > endif > NO_REGEX = YesPlease > PTHREAD_LIBS = > @@ -1350,6 +1355,11 @@ endif > endif > endif > > +ifdef SHA1_MAX_BLOCK_SIZE > +LIB_OBJS += compat/sha1_chunked.o > +BASIC_CFLAGS += SHA1_MAX_BLOCK_SIZE="$(SHA1_MAX_BLOCK_SIZE)" > +endif > + > ifdef NO_PERL_MAKEMAKER > export NO_PERL_MAKEMAKER > endif -- Atousa Pahlevan, PhD M.Math. University of Waterloo, Canada Ph.D. Department of Computer Science, University of Victoria, Canada Voice: 415-341-6206 Email: apahlevan@ieee.org Website: www.apahlevan.org ``` ## Atousa Pahlevan Duprat, 2015-10-30 22:12 Subject: [PATCH] Limit the size of the data block passed to SHA1_Update() Message-ID: <1446243122-21464-1-git-send-email-apahlevan@ieee.org> URL: https://gitlist.dev/e/1446243122-21464-1-git-send-email-apahlevan%40ieee.org In-Reply-To: ``` 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. --- 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 = + SHA1_MAX_BLOCK_SIZE = 1024L*1024L*1024L else SHA1_HEADER = 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 + #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; + while(len > 0) { + nr = len; + if(nr > SHA1_MAX_BLOCK_SIZE) + nr = SHA1_MAX_BLOCK_SIZE; + SHA1_Update(c, cdata, nr); + total += nr; + cdata += nr; + len -= nr; + } + return total; +} +#endif -- 2.4.9 (Apple Git-60) ``` ## Atousa Pahlevan Duprat, 2015-10-30 22:18 Subject: [PATCH] Limit the size of the data block passed to SHA1_Update() Message-ID: <1446243500-21580-1-git-send-email-apahlevan@ieee.org> URL: https://gitlist.dev/e/1446243500-21580-1-git-send-email-apahlevan%40ieee.org In-Reply-To: ``` 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. --- 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 = + SHA1_MAX_BLOCK_SIZE = 1024L*1024L*1024L else SHA1_HEADER = 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 + #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; + while(len > 0) { + nr = len; + if(nr > SHA1_MAX_BLOCK_SIZE) + nr = SHA1_MAX_BLOCK_SIZE; + SHA1_Update(c, cdata, nr); + total += nr; + cdata += nr; + len -= nr; + } + return total; +} +#endif -- 2.4.9 (Apple Git-60) ``` ## Junio C Hamano, 2015-10-30 22:22 Subject: Re: [PATCH] Limit the size of the data block passed to SHA1_Update() Message-ID: URL: https://gitlist.dev/e/xmqqa8r0j8tu.fsf%40gitster.mtv.corp.google.com In-Reply-To: <1446243122-21464-1-git-send-email-apahlevan@ieee.org> ``` Atousa Pahlevan Duprat writes: > 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. > 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 = > + SHA1_MAX_BLOCK_SIZE = 1024L*1024L*1024L > else > SHA1_HEADER = > 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. > #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. > + nr = SHA1_MAX_BLOCK_SIZE; > + SHA1_Update(c, cdata, nr); > + total += nr; > + cdata += nr; > + len -= nr; > + } > + return total; > +} > +#endif Thanks. ``` ## Randall S. Becker, 2015-10-30 22:26 Subject: RE: [PATCH] Limit the size of the data block passed to SHA1_Update() Message-ID: <015b01d11362$0b8c1e70$22a45b50$@com> URL: https://gitlist.dev/e/015b01d11362%240b8c1e70%2422a45b50%24%40com In-Reply-To: <1446243500-21580-1-git-send-email-apahlevan@ieee.org> ``` On October-30-15 6:18 PM, Atousa Pahlevan Duprat wrote: >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. >--- > Didn't we have this same issue with NonStop port about a year ago and decided to provision this through the configure script? Cheers, Randall ``` ## Junio C Hamano, 2015-10-31 17:35 Subject: Re: [PATCH] Limit the size of the data block passed to SHA1_Update() Message-ID: URL: https://gitlist.dev/e/xmqqoaffhrfi.fsf%40gitster.mtv.corp.google.com In-Reply-To: <015b01d11362$0b8c1e70$22a45b50$@com> ``` "Randall S. Becker" writes: > Didn't we have this same issue with NonStop port about a year ago and > decided to provision this through the configure script? I do not recall, but a support in configure.ac would be a nice addition. It does not have to be a requirement for the first cut solution, though. The customization based on Makefile variables like in Atousa's change would serve as a foundation to add configure support as a follow-up change, so let's not make the scope of this topic too big before we have a solid foundation to build on. ``` ## Eric Sunshine, 2015-11-01 01:32 Subject: Re: [PATCH] Limit the size of the data block passed to SHA1_Update() Message-ID: URL: https://gitlist.dev/e/CAPig%2BcRRjCDhdT-DvGtZqns1mMxygnxi%3DZnRKzg%2BH_do7oRpqQ%40mail.gmail.com In-Reply-To: <1446243122-21464-1-git-send-email-apahlevan@ieee.org> ``` On Fri, Oct 30, 2015 at 6:12 PM, Atousa Pahlevan Duprat wrote: > 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. > --- > 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 You can drop the quotes around the argument to #error. ``` ## atousa.p@gmail.com, 2015-11-01 06:32 Subject: [PATCH] Limit the size of the data block passed to SHA1_Update() Message-ID: <1446359536-25829-1-git-send-email-apahlevan@ieee.org> URL: https://gitlist.dev/e/1446359536-25829-1-git-send-email-apahlevan%40ieee.org In-Reply-To: ``` From: Atousa Pahlevan Duprat 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 --- Makefile | 9 +++++++++ cache.h | 7 ++++++- compat/apple-common-crypto.h | 4 ++++ compat/sha1_chunked.c | 21 +++++++++++++++++++++ 4 files changed, 40 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 = + SHA1_MAX_BLOCK_SIZE = 1024L*1024L*1024L else SHA1_HEADER = 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..d3fb264 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 + #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..bf62b1b --- /dev/null +++ b/compat/sha1_chunked.c @@ -0,0 +1,21 @@ +#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; + + while (len > 0) { + nr = len; + if (nr > SHA1_MAX_BLOCK_SIZE) + nr = SHA1_MAX_BLOCK_SIZE; + SHA1_Update(c, cdata, nr); + total += nr; + cdata += nr; + len -= nr; + } + return total; +} +#endif -- 2.4.9 (Apple Git-60) ``` ## Atousa Duprat, 2015-11-01 06:37 Subject: Re: [PATCH] Limit the size of the data block passed to SHA1_Update() Message-ID: URL: https://gitlist.dev/e/CA%2Bizobt11C%3D0hSkW6tivjRDPfbVij9-KOuhikisXrG6PWgMAjA%40mail.gmail.com In-Reply-To: <015b01d11362$0b8c1e70$22a45b50$@com> ``` > Didn't we have this same issue with NonStop port about a year ago and > decided to provision this through the configure script? I'll be happy to look at it. Can you point me to the right email thread or location in the configure.ac file? Atousa ``` ## Atousa Duprat, 2015-11-01 06:41 Subject: Re: [PATCH] Limit the size of the data block passed to SHA1_Update() Message-ID: URL: https://gitlist.dev/e/CA%2BizobuGFGhho9X6xGdXMskay3UH6-FuzriNA7%3DmkveFjo0SBA%40mail.gmail.com In-Reply-To: ``` >> +int git_SHA1_Update(SHA_CTX *c, const void *data, size_t len) >> +{ >> + size_t nr; >> + size_t total = 0; >> + char *cdata = (char*)data; > 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. Indeed, the declaration needs a const void *; but I need to advance by a specific number of bytes in each iteration of the loop. Hence the cast. Atousa ``` ## Eric Sunshine, 2015-11-01 08:30 Subject: Re: [PATCH] Limit the size of the data block passed to SHA1_Update() Message-ID: URL: https://gitlist.dev/e/CAPig%2BcSLoC-O1YneUVRjBpZzHLmASmRQx_%2B8b2dsk34ReU-ovw%40mail.gmail.com In-Reply-To: <1446359536-25829-1-git-send-email-apahlevan@ieee.org> ``` On Sun, Nov 1, 2015 at 1:32 AM, wrote: > 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 > --- > diff --git a/compat/sha1_chunked.c b/compat/sha1_chunked.c > new file mode 100644 > index 0000000..bf62b1b > --- /dev/null > +++ b/compat/sha1_chunked.c > @@ -0,0 +1,21 @@ > +#include "cache.h" > + > +#ifdef SHA1_MAX_BLOCK_SIZE This file is only compiled when SHA1_MAX_BLOCK_SIZE is defined, so does this #ifdef serve a purpose? > +int git_SHA1_Update(SHA_CTX *c, const void *data, size_t len) > +{ > + size_t nr; > + size_t total = 0; > + char *cdata = (char*)data; Nit: This could be 'const char *'. > + while (len > 0) { > + nr = len; > + if (nr > SHA1_MAX_BLOCK_SIZE) > + nr = SHA1_MAX_BLOCK_SIZE; > + SHA1_Update(c, cdata, nr); > + total += nr; > + cdata += nr; > + len -= nr; > + } > + return total; > +} > +#endif ``` ## Junio C Hamano, 2015-11-01 18:31 Subject: Re: [PATCH] Limit the size of the data block passed to SHA1_Update() Message-ID: URL: https://gitlist.dev/e/xmqqlhahh8q7.fsf%40gitster.mtv.corp.google.com In-Reply-To: ``` Atousa Duprat writes: > Indeed, the declaration needs a const void *; but I need to advance by > a specific number of bytes in each iteration of the loop. Hence the > cast. Ah, of course you are right. Silly me. Thanks. ``` ## Junio C Hamano, 2015-11-01 18:37 Subject: Re: [PATCH] Limit the size of the data block passed to SHA1_Update() Message-ID: URL: https://gitlist.dev/e/xmqqh9l5h8g3.fsf%40gitster.mtv.corp.google.com In-Reply-To: <1446359536-25829-1-git-send-email-apahlevan@ieee.org> ``` atousa.p@gmail.com writes: > 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 Hmm, I admit that this mess is my creation, but unfortunately it does not allow us to say: make SHA1_MAX_BLOCK_SIZE='1024L*1024L*1024L' when using other SHA-1 implementations (e.g. blk_SHA1_Update()). Ideas for cleaning it up, anybody? ``` ## Atousa Duprat, 2015-11-02 20:52 Subject: Re: [PATCH] Limit the size of the data block passed to SHA1_Update() Message-ID: URL: https://gitlist.dev/e/CA%2BizobvbDYLvShT8TdDhe9UiYHVWw%2BLe%2BYy4yOnvCYOWE0bhQQ%40mail.gmail.com In-Reply-To: ``` In the Makefile there is the following: ifdef BLK_SHA1 SHA1_HEADER = "block-sha1/sha1.h" LIB_OBJS += block-sha1/sha1.o else ifdef PPC_SHA1 SHA1_HEADER = "ppc/sha1.h" LIB_OBJS += ppc/sha1.o ppc/sha1ppc.o else ifdef APPLE_COMMON_CRYPTO COMPAT_CFLAGS += -DCOMMON_DIGEST_FOR_OPENSSL SHA1_HEADER = SHA1_MAX_BLOCK_SIZE = 1024L*1024L*1024L else SHA1_HEADER = EXTLIBS += $(LIB_4_CRYPTO) endif which seems to imply that BLK_SHA1 and APPLE_COMMON_CRYPTO are mutually exclusive? On Sun, Nov 1, 2015 at 10:37 AM, Junio C Hamano wrote: > atousa.p@gmail.com writes: > >> 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 > > Hmm, I admit that this mess is my creation, but unfortunately it > does not allow us to say: > > make SHA1_MAX_BLOCK_SIZE='1024L*1024L*1024L' > > when using other SHA-1 implementations (e.g. blk_SHA1_Update()). > > Ideas for cleaning it up, anybody? > -- Atousa Pahlevan, PhD M.Math. University of Waterloo, Canada Ph.D. Department of Computer Science, University of Victoria, Canada Voice: 415-341-6206 Email: apahlevan@ieee.org Website: www.apahlevan.org ``` ## Junio C Hamano, 2015-11-02 21:21 Subject: Re: [PATCH] Limit the size of the data block passed to SHA1_Update() Message-ID: URL: https://gitlist.dev/e/xmqqpozsdrnl.fsf%40gitster.mtv.corp.google.com In-Reply-To: ``` Atousa Duprat writes: > On Sun, Nov 1, 2015 at 10:37 AM, Junio C Hamano wrote: >> >> Hmm, I admit that this mess is my creation, but unfortunately it >> does not allow us to say: >> >> make SHA1_MAX_BLOCK_SIZE='1024L*1024L*1024L' >> >> when using other SHA-1 implementations (e.g. blk_SHA1_Update()). >> >> Ideas for cleaning it up, anybody? >> > In the Makefile there is the following: > > ifdef BLK_SHA1 > SHA1_HEADER = "block-sha1/sha1.h" > LIB_OBJS += block-sha1/sha1.o > else > ifdef PPC_SHA1 > SHA1_HEADER = "ppc/sha1.h" > LIB_OBJS += ppc/sha1.o ppc/sha1ppc.o > else > ifdef APPLE_COMMON_CRYPTO > COMPAT_CFLAGS += -DCOMMON_DIGEST_FOR_OPENSSL > SHA1_HEADER = > SHA1_MAX_BLOCK_SIZE = 1024L*1024L*1024L > else > SHA1_HEADER = > EXTLIBS += $(LIB_4_CRYPTO) > endif > > which seems to imply that BLK_SHA1 and APPLE_COMMON_CRYPTO are > mutually exclusive? Yes, you are correct that these two cannot be used at the same time. In general (not limited to BLK_SHA1 and APPLE_COMMON_CRYPTO) you can pick only _one_ underlying SHA-1 implementation to use with the system. If you use APPLE_COMMON_CRYPTO, you may have to use the Chunked thing, because of APPLE_COMMON_CRYPTO implementation's limitation. But the above two facts taken together does not have to imply that you are forbidden from choosing to use Chunked thing if you are using BLK_SHA1 or OpenSSL's SHA-1 implementation. If only for making sure that the Chunked wrapper passes compilation test, for trying it out to see how well it works, or just for satisfying curiosity, it would be nice if we allowed such a combination. The original arrangement of macro was: * The user code uses git_SHA1_Update() * cache.h renames git_SHA1_Update() to refer to the underlying SHA1_Update() function, either from OpenSSL or AppleCommonCrypto, or block-sha1/sha1.h renames git_SHA1_Update() to refer to our implementation blk_SHA1_Update(). What we want with Chunked is: * The user code uses git_SHA1_Update(); we must not change this, as there are many existing calls. * We want git_SHA1_Update() to call the Chunked thing when SHA1_MAX_BLOCK_SIZE is set. * The Chunked thing must delegate the actual hashing to underlying SHA1_Update(), either from OpenSSL or AppleCommonCrypto. If we are using BLK_SHA1, we want the Chunked thing to instead call blk_SHA1_Update(). I do not seem to be able to find a way to do this with the current two-level indirection. If we added another level, we can. * In cache.h, define platform_SHA1_Update() to refer to SHA1_Update() from the platform (unless block-sha1/ is used). git_SHA1_Update() in the user code may directly call it, or it may go to the Chunked thing. #ifndef git_SHA1_CTX #define platform_SHA1_Update SHA1_Update #endif #ifdef SHA1_MAX_BLOCK_SIZE #define git_SHA1_Update git_SHA1_Update_Chunked #else #define git_SHA1_Update platform_SHA1_Update #endif * In block-sha1/sha1.h, redirect platform_SHA1_Update() to blk_SHA1_Update(). #define platform_SHA1_Update blk_SHA1_Update * In compat/sha1_chunked.c, implement the Chunked thing in terms of the platform_SHA1_Update(): git_SHA1_Update_Chunked(...) { ... while (...) { platform_SHA1_Update(...); } } I am not sure if the above is worth it, but I suspect the damage is localized enough that this may be OK. ``` ## atousa.p@gmail.com, 2015-11-03 06:58 Subject: [PATCH 1/2] Limit the size of the data block passed to SHA1_Update() Message-ID: <1446533930-463-1-git-send-email-apahlevan@ieee.org> URL: https://gitlist.dev/e/1446533930-463-1-git-send-email-apahlevan%40ieee.org In-Reply-To: ``` From: Atousa Pahlevan Duprat 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 --- Makefile | 16 +++++++++++++++- block-sha1/sha1.h | 2 +- cache.h | 17 +++++++++++++---- compat/apple-common-crypto.h | 4 ++++ compat/sha1_chunked.c | 19 +++++++++++++++++++ 5 files changed, 52 insertions(+), 6 deletions(-) create mode 100644 compat/sha1_chunked.c diff --git a/Makefile b/Makefile index 04c2231..1b098cc 100644 --- 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 + 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 = + # Apple CommonCrypto requires chunking + SHA1_MAX_BLOCK_SIZE = 1024L*1024L*1024L else SHA1_HEADER = 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 ifdef NO_PERL_MAKEMAKER export NO_PERL_MAKEMAKER endif diff --git a/block-sha1/sha1.h b/block-sha1/sha1.h index b864df6..d085412 100644 --- 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..a501652 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 + +// choose whether 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 #include diff --git a/compat/apple-common-crypto.h b/compat/apple-common-crypto.h index c8b9b0e..d3fb264 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 + #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..61f67de --- /dev/null +++ b/compat/sha1_chunked.c @@ -0,0 +1,19 @@ +#include "cache.h" + +int git_SHA1_Update_Chunked(SHA_CTX *c, const void *data, size_t len) +{ + size_t nr; + size_t total = 0; + const char *cdata = (const char*)data; + + while (len > 0) { + nr = len; + if (nr > SHA1_MAX_BLOCK_SIZE) + nr = SHA1_MAX_BLOCK_SIZE; + platform_SHA1_Update(c, cdata, nr); + total += nr; + cdata += nr; + len -= nr; + } + return total; +} -- 2.4.9 (Apple Git-60) ``` ## Torsten Bögershausen, 2015-11-03 11:51 Subject: Re: [PATCH 1/2] Limit the size of the data block passed to SHA1_Update() Message-ID: <56389FBC.7050909@web.de> URL: https://gitlist.dev/e/56389FBC.7050909%40web.de In-Reply-To: <1446533930-463-1-git-send-email-apahlevan@ieee.org> ``` On 11/03/2015 07:58 AM, atousa.p@gmail.com wrote: > From: Atousa Pahlevan Duprat Minor comments inline > diff --git a/block-sha1/sha1.h b/block-sha1/sha1.h > index b864df6..d085412 100644 > --- 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..a501652 100644 > --- a/cache.h > +++ b/cache.h > @@ -10,12 +10,21 @@ > #include "trace.h" > #include "string-list.h" > > +// platform's underlying implementation of SHA1 Please use /* */ for comments > #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 > + > +// choose whether 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 > > #include > diff --git a/compat/apple-common-crypto.h b/compat/apple-common-crypto.h > index c8b9b0e..d3fb264 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 > + > #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..61f67de > --- /dev/null > +++ b/compat/sha1_chunked.c > @@ -0,0 +1,19 @@ > +#include "cache.h" > + > +int git_SHA1_Update_Chunked(SHA_CTX *c, const void *data, size_t len) > +{ > + size_t nr; > + size_t total = 0; > + const char *cdata = (const char*)data; > + > + while (len > 0) { size_t is unsigned, isn't it ? Better to use "while (len) {" > + nr = len; > + if (nr > SHA1_MAX_BLOCK_SIZE) > + nr = SHA1_MAX_BLOCK_SIZE; > + platform_SHA1_Update(c, cdata, nr); > + total += nr; > + cdata += nr; > + len -= nr; > + } > + return total; > +} ``` ## atousa.p@gmail.com, 2015-11-04 04:24 Subject: [PATCH] Limit the size of the data block passed to SHA1_Update() Message-ID: <1446611086-6102-1-git-send-email-apahlevan@ieee.org> URL: https://gitlist.dev/e/1446611086-6102-1-git-send-email-apahlevan%40ieee.org In-Reply-To: <56389FBC.7050909@web.de> ``` From: Atousa Pahlevan Duprat 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 --- Makefile | 16 +++++++++++++++- block-sha1/sha1.h | 2 +- cache.h | 17 +++++++++++++---- compat/apple-common-crypto.h | 4 ++++ compat/sha1_chunked.c | 19 +++++++++++++++++++ 5 files changed, 52 insertions(+), 6 deletions(-) create mode 100644 compat/sha1_chunked.c diff --git a/Makefile b/Makefile index 04c2231..1b098cc 100644 --- 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 + 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 = + # Apple CommonCrypto requires chunking + SHA1_MAX_BLOCK_SIZE = 1024L*1024L*1024L else SHA1_HEADER = 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 ifdef NO_PERL_MAKEMAKER export NO_PERL_MAKEMAKER endif diff --git a/block-sha1/sha1.h b/block-sha1/sha1.h index b864df6..d085412 100644 --- 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 + +/* 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 #include diff --git a/compat/apple-common-crypto.h b/compat/apple-common-crypto.h index c8b9b0e..d3fb264 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 + #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..6d0062b --- /dev/null +++ b/compat/sha1_chunked.c @@ -0,0 +1,19 @@ +#include "cache.h" + +int git_SHA1_Update_Chunked(SHA_CTX *c, const void *data, size_t len) +{ + size_t nr; + size_t total = 0; + const char *cdata = (const char*)data; + + while (len) { + nr = len; + if (nr > SHA1_MAX_BLOCK_SIZE) + nr = SHA1_MAX_BLOCK_SIZE; + platform_SHA1_Update(c, cdata, nr); + total += nr; + cdata += nr; + len -= nr; + } + return total; +} -- 2.4.9 (Apple Git-60) ``` ## Atousa Duprat, 2015-11-04 04:27 Subject: Re: [PATCH 1/2] Limit the size of the data block passed to SHA1_Update() Message-ID: URL: https://gitlist.dev/e/CA%2Bizobtfk_k9im1gy7MCOt4p%2B99PD4A2iP93haRGDzerugr8Yw%40mail.gmail.com In-Reply-To: <56389FBC.7050909@web.de> ``` Thank you for the feedback. The patch is updated as suggested. On Tue, Nov 3, 2015 at 3:51 AM, Torsten Bögershausen wrote: > On 11/03/2015 07:58 AM, atousa.p@gmail.com wrote: >> >> From: Atousa Pahlevan Duprat > > Minor comments inline >> >> diff --git a/block-sha1/sha1.h b/block-sha1/sha1.h >> index b864df6..d085412 100644 >> --- 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..a501652 100644 >> --- a/cache.h >> +++ b/cache.h >> @@ -10,12 +10,21 @@ >> #include "trace.h" >> #include "string-list.h" >> +// platform's underlying implementation of SHA1 > > Please use /* */ for comments > >> #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 >> + >> +// choose whether 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 >> #include >> diff --git a/compat/apple-common-crypto.h b/compat/apple-common-crypto.h >> index c8b9b0e..d3fb264 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 >> + >> #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..61f67de >> --- /dev/null >> +++ b/compat/sha1_chunked.c >> @@ -0,0 +1,19 @@ >> +#include "cache.h" >> + >> +int git_SHA1_Update_Chunked(SHA_CTX *c, const void *data, size_t len) >> +{ >> + size_t nr; >> + size_t total = 0; >> + const char *cdata = (const char*)data; >> + >> + while (len > 0) { > > size_t is unsigned, isn't it ? > Better to use "while (len) {" > >> + nr = len; >> + if (nr > SHA1_MAX_BLOCK_SIZE) >> + nr = SHA1_MAX_BLOCK_SIZE; >> + platform_SHA1_Update(c, cdata, nr); >> + total += nr; >> + cdata += nr; >> + len -= nr; >> + } >> + return total; >> +} > > -- Atousa Pahlevan, PhD M.Math. University of Waterloo, Canada Ph.D. Department of Computer Science, University of Victoria, Canada Voice: 415-341-6206 Email: apahlevan@ieee.org Website: www.apahlevan.org ``` ## Junio C Hamano, 2015-11-04 17:09 Subject: Re: [PATCH] Limit the size of the data block passed to SHA1_Update() Message-ID: URL: https://gitlist.dev/e/xmqqsi4lbsk0.fsf%40gitster.mtv.corp.google.com In-Reply-To: ``` Junio C Hamano writes: >> ifdef BLK_SHA1 >> SHA1_HEADER = "block-sha1/sha1.h" >> LIB_OBJS += block-sha1/sha1.o >> else >> ifdef PPC_SHA1 >> SHA1_HEADER = "ppc/sha1.h" >> LIB_OBJS += ppc/sha1.o ppc/sha1ppc.o >> else >> ifdef APPLE_COMMON_CRYPTO >> COMPAT_CFLAGS += -DCOMMON_DIGEST_FOR_OPENSSL >> SHA1_HEADER = >> SHA1_MAX_BLOCK_SIZE = 1024L*1024L*1024L >> else >> SHA1_HEADER = >> EXTLIBS += $(LIB_4_CRYPTO) >> endif >> >> which seems to imply that BLK_SHA1 and APPLE_COMMON_CRYPTO are >> mutually exclusive? > > Yes, you are correct that these two cannot be used at the same time. > In general (not limited to BLK_SHA1 and APPLE_COMMON_CRYPTO) you can > pick only _one_ underlying SHA-1 implementation to use with the > system. Our "seems to imply" above is a faulty reading. The four lines I wrote are not incorrect per-se, but this exchange was misleading. When BLK_SHA1 is defined, the above fragment from the Makefile is only saying "CommonCrypto may or may not be used for any other purposes, but for SHA-1 hashing, I'll use our own block-sha1/ implementation." It does not say anything about how the system favours CommonCrypto over OpenSSL with APPLE_COMMON_CRYPTO. And it is legit to define both APPLE_COMMON_CRYPTO and BLK_SHA1; the resulting build would still use SSL-related functions what other people may use from OpenSSL from CommonCrypto. Filipe Cabecinhas pointed out that such a configuration works (and solves the issue that triggered this thread) very early in the thread. I haven't looked carefully at the latest version of your patch, but I just wanted to make sure that I didn't mislead you to add an unnecessary "we check if both APPLE_COMMON_CRYPTO and BLK_SHA1 are defined and error out because they are incompatible" check. Sorry for an earlier message that may have been confusing. Thanks. ``` ## Eric Sunshine, 2015-11-04 19:51 Subject: Re: [PATCH] Limit the size of the data block passed to SHA1_Update() Message-ID: URL: https://gitlist.dev/e/CAPig%2BcS7NktyhveQUiBFkRtJH1D-Aif861e9qshL2R%3DZNg2%2BLw%40mail.gmail.com In-Reply-To: <1446611086-6102-1-git-send-email-apahlevan@ieee.org> ``` On Tuesday, November 3, 2015, 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 > --- 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 = > + # Apple CommonCrypto requires chunking > + SHA1_MAX_BLOCK_SIZE = 1024L*1024L*1024L > else > SHA1_HEADER = > 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 ```