From: Stefan Sperling <stsp@stsp.name>
Subject: Re: convert got_pack filesize to off_t
To: Omar Polo <op@omarpolo.com>
Cc: gameoftrees@openbsd.org
Date: Tue, 25 Oct 2022 10:05:32 +0200

On Tue, Oct 25, 2022 at 09:42:39AM +0200, Omar Polo wrote:
> Here's a rebased version after the recent changes.  One thing that i
> forgot to mention in previous email is that I'm not sure whether
> should i add a range check in got_pack_close when calling munmap;
> there's nothing we can do reached that point if pack->filesize
> overflows size_t, but maybe we could set an error nevertheless?
> yesterday i was thinking it was superfluous, but laying out this mail
> i'm starting to think that it's not a bad idea maybe.

I don't expect that pack->filesize would change across the life-time
of a pack file. If it did then the presence of a bug would be obvious
when someone investigates.

And the consequences of a partial unmap are probably not that bad.
We are not using this map to do writes.

In general, it is probably valid to unmap just a subset of an address
range used for a mapped file. Which means there could always be several
unmap calls which correspond to a given mmap call in a program. It's not
like malloc/free where both functions always refer to the same region.

> @@ -999,15 +999,10 @@ recv_packfile(struct repo_write_client **client, struc
>  
>  	log_debug("pack data received");
>  
> -	/* XXX size_t vs off_t, both should be off_t */
> -	if (pack_filesize >= SIZE_MAX) {
> -		err = got_error_msg(GOT_ERR_BAD_PACKFILE,
> -		    "pack file too large");
> -		goto done;
> -	}
>  	pack->filesize = pack_filesize;

I am glad to see the above lines going away.

> @@ -935,6 +942,13 @@ parse_negative_offset(int64_t *offset, size_t *len, st
>  
>  		if (pack->map) {
>  			size_t mapoff;
> +
> +			if (delta_offset + *len > SIZE_MAX) {
> +				return got_error_fmt(GOT_ERR_PACK_OFFSET,
> +				    "mapoff %lld would overflow size_t",
> +				    delta_offset + *len);

Should this cast to (long long) as per naddy's commit e082ed6708?

> +			}
> +
>  			mapoff = (size_t)delta_offset + *len;
>  			if (mapoff + sizeof(offN) >= pack->filesize)
>  				return got_error(GOT_ERR_PACK_OFFSET);
> @@ -1063,7 +1077,7 @@ resolve_offset_delta(struct got_delta_chain *deltas,
>  	}
>  
>  	err = add_delta(deltas, delta_offset, tslen, delta_type, delta_size,
> -	    delta_data_offset);
> +	    delta_data_offset);	/* XXX: off_t vs size_t! */
>  	if (err)
>  		return err;
>  
> @@ -1085,7 +1099,15 @@ got_pack_parse_ref_delta(struct got_object_id *id,
>      struct got_pack *pack, off_t delta_offset, int tslen)
>  {
>  	if (pack->map) {
> -		size_t mapoff = delta_offset + tslen;
> +		size_t mapoff;
> +
> +		if (delta_offset + tslen > SIZE_MAX) {
> +			return got_error_fmt(GOT_ERR_PACK_OFFSET,
> +			    "mapoff %lld would overflow size_t",
> +			    delta_offset + tslen);

Same here?

> +		}
> +
> +		mapoff = delta_offset + tslen;
>  		if (mapoff + sizeof(*id) >= pack->filesize)
>  			return got_error(GOT_ERR_PACK_OFFSET);
>  		memcpy(id, pack->map + mapoff, sizeof(*id));