On Tue, Oct 18, 2022 at 10:38:36AM +0200, Omar Polo wrote:
> this fixes some size_t/off_t usage in got-index-pack.  I've spotted
> these while looking into off_t/size_t usage in inflate.c, after a
> stsp' comment on irc.
> 
> this is only a step though: there's still an implicit off_t -> size_t
> conversion when calling the inflate.c functions, so this change is
> almost a no-op effectively, but since the inflate.c diff would vely
> likely be bigger i thought of starting with committing this.
> 
> ok?


> 
> diff aabb25f81b1f8f68a03af422f9ae14ea5c3ae1fd 535672d658a448620f24cd3a0bf61e9a6a98c94c
> commit - aabb25f81b1f8f68a03af422f9ae14ea5c3ae1fd
> commit + 535672d658a448620f24cd3a0bf61e9a6a98c94c
> blob - be8dabb4b70569ee825b8a9571f0915a88776478
> blob + 9ccb27ec5b1aa961b76fabb64c5f2e6094f9e14b
> --- lib/pack.c
> +++ lib/pack.c
> @@ -842,7 +842,7 @@ got_pack_parse_object_type_and_size(uint8_t *type, uin
>  	uint8_t t = 0;
>  	uint64_t s = 0;
>  	uint8_t sizeN;
> -	size_t mapoff = 0;
> +	off_t mapoff = 0;
>  	int i = 0;


So shouldn't mapoff remain a size_t? It describes an offset into the memory
map. And memory offsets should be size_t.
I would expect off_t to be reserved for cases where we address an offset
into a file accessed via a file descriptor.

We cannot address the whole range of a mapped file if the file's size will not
fit into a size_t. So attempting to map such files should fail (I assume).
Note that mapoff is only used if pack->map is set.
For these reasons, my expectation is that assigning offset to mapoff is fine.

Instead of changing the type of mapoff, should we add a range check before
copying the offset into mapoff, and return GOT_ERR_RANGE in case of overflow?
This would catch programming errors where we mix mapped i/o with regular i/o.