From 09b61cf340489deaa8ec8b8ad64e6a4a2627625d 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 ac5ad8c740..428298fa29 100755 --- a/t/t1007-hash-object.sh +++ b/t/t1007-hash-object.sh @@ -50,6 +50,9 @@ test_expect_success 'setup' ' example sha1:ddd3f836d3e3fbb7ae289aa9ae83536f76956399 example sha256:b44fe1fe65589848253737db859bd490453510719d7424daab03daf0767b85ae + + large5GB sha1:0be2be10a4c8764f32c4bf372a98edc731a4b204 + large5GB sha256:dc18ca621300c8d3cfa505a275641ebab00de189859e022a975056882d313e64 EOF ' @@ -249,4 +252,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 ffe05e3ffe3697db5357457aa56277356849281d 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 | 4 ++-- object-store.h | 2 +- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/object-file.c b/object-file.c index 6c8e3b1660..4b56cad44e 100644 --- a/object-file.c +++ b/object-file.c @@ -1810,7 +1810,7 @@ static void write_object_file_prepare(const struct git_hash_algo *algo, } static void write_object_file_prepare_literally(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) { @@ -2067,7 +2067,7 @@ int write_object_file_flags(const void *buf, unsigned long len, return write_loose_object(oid, hdr, hdrlen, buf, len, 0, flags); } -int write_object_file_literally(const void *buf, unsigned long len, +int write_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 539ea43904..58084be24d 100644 --- a/object-store.h +++ b/object-store.h @@ -266,7 +266,7 @@ static inline int write_object_file(const void *buf, unsigned long len, return write_object_file_flags(buf, len, type, oid, 0); } -int write_object_file_literally(const void *buf, unsigned long len, +int write_object_file_literally(const void *buf, size_t len, const char *type, struct object_id *oid, unsigned flags); From 47849d10f82f585d76b9c7c03c80c3533c0e8a15 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 | 21 +++++++++++---------- object-store.h | 4 ++-- 2 files changed, 13 insertions(+), 12 deletions(-) diff --git a/object-file.c b/object-file.c index 4b56cad44e..bd59413eb7 100644 --- a/object-file.c +++ b/object-file.c @@ -1787,7 +1787,7 @@ void *read_object_with_reference(struct repository *r, static void hash_object_body(const struct git_hash_algo *algo, git_hash_ctx *c, const void *buf, unsigned long len, struct object_id *oid, - char *hdr, int *hdrlen) + char *hdr, size_t *hdrlen) { algo->init_fn(c); algo->update_fn(c, hdr, *hdrlen); @@ -1796,9 +1796,9 @@ static void hash_object_body(const struct git_hash_algo *algo, git_hash_ctx *c, } static void write_object_file_prepare(const struct git_hash_algo *algo, - const void *buf, unsigned long len, + const void *buf, size_t len, enum object_type type, struct object_id *oid, - char *hdr, int *hdrlen) + char *hdr, size_t *hdrlen) { git_hash_ctx c; @@ -1812,7 +1812,7 @@ static void write_object_file_prepare(const struct git_hash_algo *algo, static void write_object_file_prepare_literally(const struct git_hash_algo *algo, 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; @@ -1871,17 +1871,17 @@ static int write_buffer(int fd, const void *buf, size_t len) } static void hash_object_file_literally(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[MAX_HEADER_LEN]; - int hdrlen = sizeof(hdr); + size_t hdrlen = sizeof(hdr); write_object_file_prepare_literally(algo, buf, len, type, oid, hdr, &hdrlen); } void hash_object_file(const struct git_hash_algo *algo, const void *buf, - unsigned long len, enum object_type type, + size_t len, enum object_type type, struct object_id *oid) { hash_object_file_literally(algo, buf, len, type_name(type), oid); @@ -2050,12 +2050,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, enum object_type 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. @@ -2072,7 +2072,8 @@ int write_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 58084be24d..4c2050fe7f 100644 --- a/object-store.h +++ b/object-store.h @@ -254,10 +254,10 @@ static inline void *repo_read_object_file(struct repository *r, int oid_object_info(struct repository *r, const struct object_id *, unsigned long *); void hash_object_file(const struct git_hash_algo *algo, const void *buf, - unsigned long len, enum object_type type, + size_t len, enum object_type 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, enum object_type type, struct object_id *oid, unsigned flags); static inline int write_object_file(const void *buf, unsigned long len, From 2c65ff4197090c253fe5c0d7e119266f8b542ecf 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 | 4 ++-- sha1dc_git.c | 3 +-- sha1dc_git.h | 2 +- t/t1007-hash-object.sh | 2 +- 4 files changed, 5 insertions(+), 6 deletions(-) diff --git a/object-file.c b/object-file.c index bd59413eb7..f6257741b9 100644 --- a/object-file.c +++ b/object-file.c @@ -1785,7 +1785,7 @@ void *read_object_with_reference(struct repository *r, } static void hash_object_body(const struct git_hash_algo *algo, git_hash_ctx *c, - const void *buf, unsigned long len, + const void *buf, size_t len, struct object_id *oid, char *hdr, size_t *hdrlen) { @@ -1805,7 +1805,7 @@ static void write_object_file_prepare(const struct git_hash_algo *algo, /* Generate the header */ *hdrlen = format_object_header(hdr, *hdrlen, type, len); - /* Sha1.. */ + /* Hash (function pointers) computation */ hash_object_body(algo, &c, buf, len, oid, hdr, hdrlen); } 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 428298fa29..b30fb1282a 100755 --- a/t/t1007-hash-object.sh +++ b/t/t1007-hash-object.sh @@ -252,7 +252,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 4295666a8252cce769ed55d3f6f8687c19b3fd8b 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 b30fb1282a..b6dbee4bb9 100755 --- a/t/t1007-hash-object.sh +++ b/t/t1007-hash-object.sh @@ -260,4 +260,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 bc48b4cf1aa53b416a39e963dc03eae5eb9db687 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 b6dbee4bb9..f7062ebca1 100755 --- a/t/t1007-hash-object.sh +++ b/t/t1007-hash-object.sh @@ -268,4 +268,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 8975e19d3e960b3f8c3f08ddb04fdf2f01b04151 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 f7062ebca1..df78646b07 100755 --- a/t/t1007-hash-object.sh +++ b/t/t1007-hash-object.sh @@ -276,4 +276,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