git/list[1] front-page[2] threads[3] people[4] search[5] about
 

Re: [Patch size_t V3 15/19] Convert archive functions to size_t

From
brian m. carlson <sandals@crustytoothpaste.net>
Date
Aug 22, 2017, 01:19 UTC
Message-ID
<20170822011904.2mow7ejlr22y7ova@genre.crustytoothpaste.net>
In-Reply-To
<xmqqvalh5smc.fsf@gitster.mtv.corp.google.com>
On Sun, Aug 20, 2017 at 11:42:35PM -0700, Junio C Hamano wrote:
Show 22 quoted lines
> Martin Koegler <martin.koegler@chello.at> writes:
> 
> > From: Martin Koegler <martin.koegler@chello.at>
> >
> > Signed-off-by: Martin Koegler <martin.koegler@chello.at>
> > ---
> >  archive-tar.c | 16 ++++++++--------
> >  archive-zip.c | 22 +++++++++++-----------
> >  2 files changed, 19 insertions(+), 19 deletions(-)
> 
> I feel that this needs a careful review from somebody who knows the
> definition of archive formats well.
> 
> I am reasonably confident to say that the part of this patch that
> updates the variable used to interact with zlib to size_t.  Use of
> fixed-width uint32_t for CRC32 may also be correct, I would think.
> 
> But for all the other changes, it makes me nervous to see that
> size_t is used to describe size of things in an archive, and makes
> me suspect that some may want to be off_t, because an archive is a
> concatenation of files, whose sizes are better measured in off_t
> rather than size_t.
I can't speak for Zip archives, but I'm familiar with tar.
Show 10 quoted lines
> > diff --git a/archive-tar.c b/archive-tar.c
> > index 719673d..ee56b2b 100644
> > --- a/archive-tar.c
> > +++ b/archive-tar.c
> > @@ -12,7 +12,7 @@
> >  #define BLOCKSIZE	(RECORDSIZE * 20)
> >  
> >  static char block[BLOCKSIZE];
> > -static unsigned long offset;
> > +static size_t offset;
This is fine.  The value has to be smaller than BLOCKSIZE.
Show 9 quoted lines
> >  
> >  static int tar_umask = 002;
> >  
> > @@ -50,12 +50,12 @@ static void write_if_needed(void)
> >   * queues up writes, so that all our write(2) calls write exactly one
> >   * full block; pads writes to RECORDSIZE
> >   */
> > -static void do_write_blocked(const void *data, unsigned long size)
> > +static void do_write_blocked(const void *data, size_t size)

This is fine because it's only called from write_blocked, which is only called on (a) extended headers, (b) global extended headers, and (c) files below the big file threshold.

We already restrict the big file threshold to unsigned long (in config parsing), which will be no larger than size_t, and we can't allocate a buffer to load data into memory that is larger than size_t, so this should always be safe.

Show 6 quoted lines
> >  {
> >  	const char *buf = data;
> >  
> >  	if (offset) {
> > -		unsigned long chunk = BLOCKSIZE - offset;
> > +		size_t chunk = BLOCKSIZE - offset;
This is safe because it's smaller than BLOCKSIZE.
Show 10 quoted lines
> >  		if (size < chunk)
> >  			chunk = size;
> >  		memcpy(block + offset, buf, chunk);
> > @@ -77,7 +77,7 @@ static void do_write_blocked(const void *data, unsigned long size)
> >  
> >  static void finish_record(void)
> >  {
> > -	unsigned long tail;
> > +	size_t tail;
> >  	tail = offset % RECORDSIZE;
This is safe because it's smaller than RECORDSIZE.
Show 8 quoted lines
> >  	if (tail)  {
> >  		memset(block + offset, 0, RECORDSIZE - tail);
> > @@ -86,7 +86,7 @@ static void finish_record(void)
> >  	write_if_needed();
> >  }
> >  
> > -static void write_blocked(const void *data, unsigned long size)
> > +static void write_blocked(const void *data, size_t size)
This is safe for the same reason do_write_blocked is safe.
Show 13 quoted lines
> >  {
> >  	do_write_blocked(data, size);
> >  	finish_record();
> > @@ -198,10 +198,10 @@ static size_t get_path_prefix(const char *path, size_t pathlen, size_t maxlen)
> >  
> >  static void prepare_header(struct archiver_args *args,
> >  			   struct ustar_header *header,
> > -			   unsigned int mode, unsigned long size)
> > +			   unsigned int mode, size_t size)
> >  {
> >  	xsnprintf(header->mode, sizeof(header->mode), "%07o", mode & 07777);
> > -	xsnprintf(header->size, sizeof(header->size), "%011lo", S_ISREG(mode) ? size : 0);
> > +	xsnprintf(header->size, sizeof(header->size), "%011lo", S_ISREG(mode) ? (unsigned long)size : 0);

These two definitely need to be off_t. These could be up to 2^33 and size_t may only be 32 bits.

Show 9 quoted lines
> >  	xsnprintf(header->mtime, sizeof(header->mtime), "%011lo", (unsigned long) args->time);
> >  
> >  	xsnprintf(header->uid, sizeof(header->uid), "%07o", 0);
> > @@ -219,7 +219,7 @@ static void prepare_header(struct archiver_args *args,
> >  
> >  static void write_extended_header(struct archiver_args *args,
> >  				  const unsigned char *sha1,
> > -				  const void *buffer, unsigned long size)
> > +				  const void *buffer, size_t size)
This is fine as it points to a strbuf length.
-- 
brian m. carlson / brian with sandals: Houston, Texas, US
https://www.crustytoothpaste.net/~bmc | My opinion only
OpenPGP: https://keybase.io/bk2204
Previous: Junio C HamanoNext: Martin Koegler
Message 22 of 33 in “use size_t”
  1. 00/19 use size_tMartin Koegler, Aug 16, 2017
  2. 03/19 Convert zlib.c to size_tMartin Koegler, Aug 16, 2017
  3. 01/19 delta: fix enconding size larger than an "uint" can holdMartin Koegler, Aug 16, 2017
  4. Torsten BögershausenAug 17, 2017
  5. 08/19 Convert fsck.c & commit.c to size_tMartin Koegler, Aug 16, 2017
  6. 11/19 Use size_t for config parsingMartin Koegler, Aug 16, 2017
  7. Johannes SixtAug 24, 2017
  8. 04/19 delta: Fix offset overflowsMartin Koegler, Aug 16, 2017
  9. 12/19 Convert pack-objects to size_tMartin Koegler, Aug 16, 2017
  10. 19/19 Convert xdiff-interface to size_tMartin Koegler, Aug 16, 2017
  11. Junio C HamanoAug 17, 2017
  12. 17/19 Convert ref-filter to size_tMartin Koegler, Aug 16, 2017
  13. Junio C HamanoAug 17, 2017
  14. Junio C HamanoAug 17, 2017
  15. 10/19 Add overflow check to get_delta_hdr_sizeMartin Koegler, Aug 16, 2017
  16. 18/19 Convert tree-walk to size_tMartin Koegler, Aug 16, 2017
  17. Junio C HamanoAug 17, 2017
  18. 16/19 Convert various things to size_tMartin Koegler, Aug 16, 2017
  19. Junio C HamanoAug 21, 2017
  20. 15/19 Convert archive functions to size_tMartin Koegler, Aug 16, 2017
  21. Junio C HamanoAug 21, 2017
  22. brian m. carlsonAug 22, 2017
  23. 07/19 Convert parse_X_buffer to size_tMartin Koegler, Aug 16, 2017
  24. 14/19 Convert unpack-objects to size_tMartin Koegler, Aug 16, 2017
  25. 13/19 Convert index-pack to size_tMartin Koegler, Aug 16, 2017
  26. Ramsay JonesAug 16, 2017
  27. 02/19 Convert size datatype to size_tMartin Koegler, Aug 16, 2017
  28. 09/19 Convert cache functions to size_tMartin Koegler, Aug 16, 2017
  29. 05/19 Convert sha1_file.c to size_tMartin Koegler, Aug 16, 2017
  30. 06/19 Use size_t for sha1Martin Koegler, Aug 16, 2017
  31. Junio C HamanoAug 16, 2017
  32. Torsten BögershausenAug 17, 2017
  33. Martin KoeglerAug 18, 2017

Read the whole thread, see it on lore, or plain text.

$ cat FOOTERMessages come from the public archive at lore.kernel.org/git, fetched every hour. The front page is chosen and written each morning by an AI editor and can be wrong; the threads themselves are the record. About and API. For agents: an MCP server at https://gitlist.dev/mcp, and any thread, story or person page as Markdown by adding .md to its URL (or sending Accept: text/markdown). Details in /llms.txt.