Re: [PATCH v4 5/8] odb: teach read_blob_entry to use size_t

["Torsten Bögershausen" <tboegi@web.de>]

Nicely explained, some comments inline

On Tue, Nov 02, 2021 at 03:46:08PM +0000, Matt Cooper via GitGitGadget wrote:
> From: Matt Cooper <vtbassmatt@gmail.com>
>
> There is mixed use of size_t and unsigned long to deal with sizes in the
> codebase. Recall that Windows defines unsigned long as 32 bits even on
> 64-bit platforms, meaning that converting size_t to unsigned long narrows
> the range. This mostly doesn't cause a problem since Git rarely deals
> with files larger than 2^32 bytes.

What does this mean ?
> ... This mostly doesn't cause a problem since Git rarely deals
> with files larger than 2^32 bytes.

Is "mostly" is a good wording here ?
May be
This doesn't cause a problem when files smaller than 2^32 bytes are handled by Git.

>
> But adjunct systems such as Git LFS, which use smudge/clean filters to
> keep huge files out of the repository, may have huge file contents passed
> through some of the functions in entry.c and convert.c. On Windows, this
> results in a truncated file being written to the workdir. I traced this to
> one specific use of unsigned long in write_entry (and a similar instance
> in write_pc_item_to_fd for parallel checkout). That appeared to be for
> the call to read_blob_entry, which expects a pointer to unsigned long.
>
> By altering the signature of read_blob_entry to expect a size_t,

"expect" -> "use"

(I am not a native English speaker, would "changing" be better than "altering" ?)
 By changing the signature of read_blob_entry to use size_t,



> write_entry can be switched to use size_t internally (which all of its
> callers and most of its callees already used). To avoid touching dozens of
> additional files, read_blob_entry uses a local unsigned long to call a
> chain of functions which aren't prepared to accept size_t.
>
> Helped-by: Johannes Schindelin <johannes.schindelin@gmx.de>
> Signed-off-by: Matt Cooper <vtbassmatt@gmail.com>
> Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
> ---
>  entry.c                     | 8 +++++---
>  entry.h                     | 2 +-
>  parallel-checkout.c         | 2 +-
>  t/t1051-large-conversion.sh | 2 +-
>  4 files changed, 8 insertions(+), 6 deletions(-)
>
> diff --git a/entry.c b/entry.c
> index 711ee0693c7..4cb3942dbdc 100644
> --- a/entry.c
> +++ b/entry.c
> @@ -82,11 +82,13 @@ static int create_file(const char *path, unsigned int mode)
>  	return open(path, O_WRONLY | O_CREAT | O_EXCL, mode);
>  }
>
> -void *read_blob_entry(const struct cache_entry *ce, unsigned long *size)
> +void *read_blob_entry(const struct cache_entry *ce, size_t *size)
>  {
>  	enum object_type type;
> -	void *blob_data = read_object_file(&ce->oid, &type, size);
> +	unsigned long ul;
> +	void *blob_data = read_object_file(&ce->oid, &type, &ul);
>
> +	*size = ul;
>  	if (blob_data) {
>  		if (type == OBJ_BLOB)
>  			return blob_data;
> @@ -270,7 +272,7 @@ static int write_entry(struct cache_entry *ce, char *path, struct conv_attrs *ca
>  	int fd, ret, fstat_done = 0;
>  	char *new_blob;
>  	struct strbuf buf = STRBUF_INIT;
> -	unsigned long size;
> +	size_t size;
>  	ssize_t wrote;
>  	size_t newsize = 0;
>  	struct stat st;
> diff --git a/entry.h b/entry.h
> index b8c0e170dc7..61ee8c17604 100644
> --- a/entry.h
> +++ b/entry.h
> @@ -51,7 +51,7 @@ int finish_delayed_checkout(struct checkout *state, int *nr_checkouts);
>   */
>  void unlink_entry(const struct cache_entry *ce);
>
> -void *read_blob_entry(const struct cache_entry *ce, unsigned long *size);
> +void *read_blob_entry(const struct cache_entry *ce, size_t *size);
>  int fstat_checkout_output(int fd, const struct checkout *state, struct stat *st);
>  void update_ce_after_write(const struct checkout *state, struct cache_entry *ce,
>  			   struct stat *st);
> diff --git a/parallel-checkout.c b/parallel-checkout.c
> index 6b1af32bb3d..b6f4a25642e 100644
> --- a/parallel-checkout.c
> +++ b/parallel-checkout.c
> @@ -261,7 +261,7 @@ static int write_pc_item_to_fd(struct parallel_checkout_item *pc_item, int fd,
>  	struct stream_filter *filter;
>  	struct strbuf buf = STRBUF_INIT;
>  	char *blob;
> -	unsigned long size;
> +	size_t size;
>  	ssize_t wrote;
>
>  	/* Sanity check */
> diff --git a/t/t1051-large-conversion.sh b/t/t1051-large-conversion.sh
> index e7f9f0bdc56..e6d52f98b15 100755
> --- a/t/t1051-large-conversion.sh
> +++ b/t/t1051-large-conversion.sh
> @@ -85,7 +85,7 @@ test_expect_success 'ident converts on output' '
>
>  # This smudge filter prepends 5GB of zeros to the file it checks out. This
>  # ensures that smudging doesn't mangle large files on 64-bit Windows.
> -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 convert on output' '
>  	test_commit test small "a small file" &&
>  	small_size=$(test_file_size small) &&
> --
> gitgitgadget
>