From ac5f15e93b81455b270563d857bf87338b5df43f Mon Sep 17 00:00:00 2001 From: Philip Oakley Date: Sun, 31 Oct 2021 23:15:13 +0000 Subject: [PATCH 1/7] hash-object: demonstrate a >4GB/LLP64 problem On LLP64 systems, such as Windows, the size of `long`, `int`, etc. is only 32 bits (for backward compatibility). Git's use of `unsigned long` for file memory sizes in many places, rather than size_t, limits the handling of large files on LLP64 systems (commonly given as `>4GB`). Provide a minimum test for handling a >4GB file. The `hash-object` command, with the `--literally` and without `-w` option avoids writing the object, either loose or packed. This avoids the code paths hitting the `bigFileThreshold` config test code, the zlib code, and the pack code. Subsequent patches will walk the test's call chain, converting types to `size_t` (which is larger in LLP64 data models) where appropriate. Signed-off-by: Philip Oakley Signed-off-by: Johannes Schindelin --- t/t1007-hash-object.sh | 11 +++++++++++ 1 file changed, 11 insertions(+) diff --git a/t/t1007-hash-object.sh b/t/t1007-hash-object.sh index 64b340f227..712c822d77 100755 --- a/t/t1007-hash-object.sh +++ b/t/t1007-hash-object.sh @@ -49,6 +49,9 @@ test_expect_success 'setup' ' example sha1:ddd3f836d3e3fbb7ae289aa9ae83536f76956399 example sha256:b44fe1fe65589848253737db859bd490453510719d7424daab03daf0767b85ae + + large5GB sha1:0be2be10a4c8764f32c4bf372a98edc731a4b204 + large5GB sha256:dc18ca621300c8d3cfa505a275641ebab00de189859e022a975056882d313e64 EOF ' @@ -248,4 +251,12 @@ test_expect_success '--literally with extra-long type' ' echo example | git hash-object -t $t --literally --stdin ' +test_expect_failure EXPENSIVE,SIZE_T_IS_64BIT,!LONG_IS_64BIT \ + 'files over 4GB hash literally' ' + test-tool genzeros $((5*1024*1024*1024)) >big && + test_oid large5GB >expect && + git hash-object --stdin --literally actual && + test_cmp expect actual +' + test_done From eecb48d160369d38a72e84e3f120322e58c712ed Mon Sep 17 00:00:00 2001 From: Philip Oakley Date: Fri, 12 Nov 2021 21:07:03 +0000 Subject: [PATCH 2/7] hash_object_file_literally(): use size_t The previous commit adds a test that demonstrates a problem in the `hash-object --literally` command, manifesting in an unnecessary file size limit on systems using the LLP64 data model (which includes Windows). Walking the affected code path is `cmd_hash_object()` >> `hash_fd()` >> `hash_literally()` >> `hash_object_file_literally()`. The function `hash_object_file_literally()` is the first with a file length parameter (via a mem buffer). This commit changes the type of that parameter to the LLP64 compatible `size_t` type. There are no other uses of the function. The `strbuf` type is already `size_t` compatible. Note: The hash-object test does not yet pass. Subsequent commits will continue to walk the call tree's lower level functions to identify further fixes. Signed-off-by: Philip Oakley Signed-off-by: Johannes Schindelin --- object-file.c | 2 +- object-store.h | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/object-file.c b/object-file.c index 8be57f48de..8d645656b7 100644 --- a/object-file.c +++ b/object-file.c @@ -2014,7 +2014,7 @@ int write_object_file_flags(const void *buf, unsigned long len, return write_loose_object(oid, hdr, hdrlen, buf, len, 0, flags); } -int hash_object_file_literally(const void *buf, unsigned long len, +int hash_object_file_literally(const void *buf, size_t len, const char *type, struct object_id *oid, unsigned flags) { diff --git a/object-store.h b/object-store.h index 6f89482df0..aa74b4f9b6 100644 --- a/object-store.h +++ b/object-store.h @@ -258,7 +258,7 @@ static inline int write_object_file(const void *buf, unsigned long len, return write_object_file_flags(buf, len, type, oid, 0); } -int hash_object_file_literally(const void *buf, unsigned long len, +int hash_object_file_literally(const void *buf, size_t len, const char *type, struct object_id *oid, unsigned flags); From cfe5194ad79c26acbf8fcf80d4355d27641f439a Mon Sep 17 00:00:00 2001 From: Philip Oakley Date: Fri, 12 Nov 2021 21:14:50 +0000 Subject: [PATCH 3/7] object-file.c: use size_t for header lengths Continue walking the code path for the >4GB `hash-object --literally` test. The `hash_object_file_literally()` function internally uses both `hash_object_file()` and `write_object_file_prepare()`. Both function signatures use `unsigned long` rather than `size_t` for the mem buffer sizes. Use `size_t` instead, for LLP64 compatibility. While at it, convert those function's object's header buffer length to `size_t` for consistency. The value is already upcast to `uintmax_t` for print format compatibility. Note: The hash-object test still does not pass. A subsequent commit continues to walk the call tree's lower level hash functions to identify further fixes. Signed-off-by: Philip Oakley Signed-off-by: Johannes Schindelin --- object-file.c | 15 ++++++++------- object-store.h | 4 ++-- 2 files changed, 10 insertions(+), 9 deletions(-) diff --git a/object-file.c b/object-file.c index 8d645656b7..e57e1d00de 100644 --- a/object-file.c +++ b/object-file.c @@ -1770,9 +1770,9 @@ void *read_object_with_reference(struct repository *r, } static void write_object_file_prepare(const struct git_hash_algo *algo, - const void *buf, unsigned long len, + const void *buf, size_t len, const char *type, struct object_id *oid, - char *hdr, int *hdrlen) + char *hdr, size_t *hdrlen) { git_hash_ctx c; @@ -1837,11 +1837,11 @@ static int write_buffer(int fd, const void *buf, size_t len) } int hash_object_file(const struct git_hash_algo *algo, const void *buf, - unsigned long len, const char *type, + size_t len, const char *type, struct object_id *oid) { char hdr[MAX_HEADER_LEN]; - int hdrlen = sizeof(hdr); + size_t hdrlen = sizeof(hdr); write_object_file_prepare(algo, buf, len, type, oid, hdr, &hdrlen); return 0; } @@ -1997,12 +1997,12 @@ static int freshen_packed_object(const struct object_id *oid) return 1; } -int write_object_file_flags(const void *buf, unsigned long len, +int write_object_file_flags(const void *buf, size_t len, const char *type, struct object_id *oid, unsigned flags) { char hdr[MAX_HEADER_LEN]; - int hdrlen = sizeof(hdr); + size_t hdrlen = sizeof(hdr); /* Normally if we have it in the pack then we do not bother writing * it out into .git/objects/??/?{38} file. @@ -2019,7 +2019,8 @@ int hash_object_file_literally(const void *buf, size_t len, unsigned flags) { char *header; - int hdrlen, status = 0; + size_t hdrlen; + int status = 0; /* type string, SP, %lu of the length plus NUL must fit this */ hdrlen = strlen(type) + MAX_HEADER_LEN; diff --git a/object-store.h b/object-store.h index aa74b4f9b6..d05583df38 100644 --- a/object-store.h +++ b/object-store.h @@ -246,10 +246,10 @@ static inline void *repo_read_object_file(struct repository *r, int oid_object_info(struct repository *r, const struct object_id *, unsigned long *); int hash_object_file(const struct git_hash_algo *algo, const void *buf, - unsigned long len, const char *type, + size_t len, const char *type, struct object_id *oid); -int write_object_file_flags(const void *buf, unsigned long len, +int write_object_file_flags(const void *buf, size_t len, const char *type, struct object_id *oid, unsigned flags); static inline int write_object_file(const void *buf, unsigned long len, From 52f44988be4ca5a7226c522593d8eda817e927c9 Mon Sep 17 00:00:00 2001 From: Philip Oakley Date: Fri, 12 Nov 2021 21:16:51 +0000 Subject: [PATCH 4/7] hash algorithms: use size_t for section lengths Continue walking the code path for the >4GB `hash-object --literally` test to the hash algorithm step for LLP64 systems. This patch lets the SHA1DC code use `size_t`, making it compatible with LLP64 data models (as used e.g. by Windows). The interested reader of this patch will note that we adjust the signature of the `git_SHA1DCUpdate()` function without updating _any_ call site. This certainly puzzled at least one reviewer already, so here is an explanation: This function is never called directly, but always via the macro `platform_SHA1_Update`, which is usually called via the macro `git_SHA1_Update`. However, we never call `git_SHA1_Update()` directly in `struct git_hash_algo`. Instead, we call `git_hash_sha1_update()`, which is defined thusly: static void git_hash_sha1_update(git_hash_ctx *ctx, const void *data, size_t len) { git_SHA1_Update(&ctx->sha1, data, len); } i.e. it contains an implicit downcast from `size_t` to `unsigned long` (before this here patch). With this patch, there is no downcast anymore. With this patch, finally, the t1007-hash-object.sh "files over 4GB hash literally" test case is fixed. Signed-off-by: Philip Oakley Signed-off-by: Johannes Schindelin --- object-file.c | 2 +- sha1dc_git.c | 3 +-- sha1dc_git.h | 2 +- t/t1007-hash-object.sh | 2 +- 4 files changed, 4 insertions(+), 5 deletions(-) diff --git a/object-file.c b/object-file.c index e57e1d00de..2e10b801ba 100644 --- a/object-file.c +++ b/object-file.c @@ -1779,7 +1779,7 @@ static void write_object_file_prepare(const struct git_hash_algo *algo, /* Generate the header */ *hdrlen = xsnprintf(hdr, *hdrlen, "%s %"PRIuMAX , type, (uintmax_t)len)+1; - /* Sha1.. */ + /* Hash (function pointers) computation */ algo->init_fn(&c); algo->update_fn(&c, hdr, *hdrlen); algo->update_fn(&c, buf, len); diff --git a/sha1dc_git.c b/sha1dc_git.c index 5c300e812e..65a6f1f2cd 100644 --- a/sha1dc_git.c +++ b/sha1dc_git.c @@ -25,10 +25,9 @@ void git_SHA1DCFinal(unsigned char hash[20], SHA1_CTX *ctx) /* * Same as SHA1DCUpdate, but adjust types to match git's usual interface. */ -void git_SHA1DCUpdate(SHA1_CTX *ctx, const void *vdata, unsigned long len) +void git_SHA1DCUpdate(SHA1_CTX *ctx, const void *vdata, size_t len) { const char *data = vdata; - /* We expect an unsigned long, but sha1dc only takes an int */ while (len > INT_MAX) { SHA1DCUpdate(ctx, data, INT_MAX); data += INT_MAX; diff --git a/sha1dc_git.h b/sha1dc_git.h index 41e1c3fd3f..847b38fb4b 100644 --- a/sha1dc_git.h +++ b/sha1dc_git.h @@ -15,7 +15,7 @@ void git_SHA1DCInit(SHA1_CTX *); #endif void git_SHA1DCFinal(unsigned char [20], SHA1_CTX *); -void git_SHA1DCUpdate(SHA1_CTX *ctx, const void *data, unsigned long len); +void git_SHA1DCUpdate(SHA1_CTX *ctx, const void *data, size_t len); #define platform_SHA_CTX SHA1_CTX #define platform_SHA1_Init git_SHA1DCInit diff --git a/t/t1007-hash-object.sh b/t/t1007-hash-object.sh index 712c822d77..039d9a07ac 100755 --- a/t/t1007-hash-object.sh +++ b/t/t1007-hash-object.sh @@ -251,7 +251,7 @@ test_expect_success '--literally with extra-long type' ' echo example | git hash-object -t $t --literally --stdin ' -test_expect_failure EXPENSIVE,SIZE_T_IS_64BIT,!LONG_IS_64BIT \ +test_expect_success EXPENSIVE,SIZE_T_IS_64BIT,!LONG_IS_64BIT \ 'files over 4GB hash literally' ' test-tool genzeros $((5*1024*1024*1024)) >big && test_oid large5GB >expect && From 6971b073cdbc63646591eeac7b1cf6631b79cc98 Mon Sep 17 00:00:00 2001 From: Philip Oakley Date: Mon, 6 Dec 2021 22:26:50 +0000 Subject: [PATCH 5/7] hash-object --stdin: verify that it works with >4GB/LLP64 Just like the `hash-object --literally` code path, the `--stdin` code path also needs to use `size_t` instead of `unsigned long` to represent memory sizes, otherwise it would cause problems on platforms using the LLP64 data model (such as Windows). To limit the scope of the test case, the object is explicitly not written to the object store, nor are any filters applied. The `big` file from the previous test case is reused to save setup time; To avoid relying on that side effect, it is generated if it does not exist (e.g. when running via `sh t1007-*.sh --long --run=1,41`). Signed-off-by: Philip Oakley Signed-off-by: Johannes Schindelin --- t/t1007-hash-object.sh | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/t/t1007-hash-object.sh b/t/t1007-hash-object.sh index 039d9a07ac..107f201da6 100755 --- a/t/t1007-hash-object.sh +++ b/t/t1007-hash-object.sh @@ -259,4 +259,12 @@ test_expect_success EXPENSIVE,SIZE_T_IS_64BIT,!LONG_IS_64BIT \ test_cmp expect actual ' +test_expect_success EXPENSIVE,SIZE_T_IS_64BIT,!LONG_IS_64BIT \ + 'files over 4GB hash correctly via --stdin' ' + { test -f big || test-tool genzeros $((5*1024*1024*1024)) >big; } && + test_oid large5GB >expect && + git hash-object --stdin actual && + test_cmp expect actual +' + test_done From db0035973a1a2819566c9d59397f10da5e4533c2 Mon Sep 17 00:00:00 2001 From: Philip Oakley Date: Mon, 6 Dec 2021 22:42:46 +0000 Subject: [PATCH 6/7] hash-object: add another >4GB/LLP64 test case To complement the `--stdin` and `--literally` test cases that verify that we can hash files larger than 4GB on 64-bit platforms using the LLP64 data model, here is a test case that exercises `hash-object` _without_ any options. Just as before, we use the `big` file from the previous test case if it exists to save on setup time, otherwise generate it. Signed-off-by: Philip Oakley Signed-off-by: Johannes Schindelin --- t/t1007-hash-object.sh | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/t/t1007-hash-object.sh b/t/t1007-hash-object.sh index 107f201da6..4ef8f6f5bd 100755 --- a/t/t1007-hash-object.sh +++ b/t/t1007-hash-object.sh @@ -267,4 +267,12 @@ test_expect_success EXPENSIVE,SIZE_T_IS_64BIT,!LONG_IS_64BIT \ test_cmp expect actual ' +test_expect_success EXPENSIVE,SIZE_T_IS_64BIT,!LONG_IS_64BIT \ + 'files over 4GB hash correctly' ' + { test -f big || test-tool genzeros $((5*1024*1024*1024)) >big; } && + test_oid large5GB >expect && + git hash-object -- big >actual && + test_cmp expect actual +' + test_done From 61e3cd9398b388286732905189dc4b48cd259878 Mon Sep 17 00:00:00 2001 From: Philip Oakley Date: Tue, 7 Dec 2021 09:53:41 +0000 Subject: [PATCH 7/7] hash-object: add a >4GB/LLP64 test case using filtered input To verify that the `clean` side of the `clean`/`smudge` filter code is correct with regards to LLP64 (read: to ensure that `size_t` is used instead of `unsigned long`), here is a test case using a trivial filter, specifically _not_ writing anything to the object store to limit the scope of the test case. As in previous commits, the `big` file from previous test cases is reused if available, to save setup time, otherwise re-generated. Signed-off-by: Philip Oakley Signed-off-by: Johannes Schindelin --- t/t1007-hash-object.sh | 12 ++++++++++++ 1 file changed, 12 insertions(+) diff --git a/t/t1007-hash-object.sh b/t/t1007-hash-object.sh index 4ef8f6f5bd..855e2c397c 100755 --- a/t/t1007-hash-object.sh +++ b/t/t1007-hash-object.sh @@ -275,4 +275,16 @@ test_expect_success EXPENSIVE,SIZE_T_IS_64BIT,!LONG_IS_64BIT \ test_cmp expect actual ' +# This clean filter does nothing, other than excercising the interface. +# We ensure that cleaning doesn't mangle large files on 64-bit Windows. +test_expect_success EXPENSIVE,SIZE_T_IS_64BIT,!LONG_IS_64BIT \ + 'hash filtered files over 4GB correctly' ' + { test -f big || test-tool genzeros $((5*1024*1024*1024)) >big; } && + test_oid large5GB >expect && + test_config filter.null-filter.clean "cat" && + echo "big filter=null-filter" >.gitattributes && + git hash-object -- big >actual && + test_cmp expect actual +' + test_done