On Sun, Aug 20, 2017 at 11:42:35PM -0700, Junio C Hamano wrote:
> Martin Koegler <martin.koegler@xxxxxxxxx> writes:
>
> > From: Martin Koegler <martin.koegler@xxxxxxxxx>
> >
> > Signed-off-by: Martin Koegler <martin.koegler@xxxxxxxxx>
> > ---
> > 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.
> > 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.
> >
> > 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.
> > {
> > 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.
> > 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.
> > 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.
> > {
> > 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.
> > 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
Attachment:
signature.asc
Description: PGP signature
