From: Stefan Sperling Subject: Re: some size_t/off_t fixes for got-index-pack To: Omar Polo Cc: gameoftrees@openbsd.org Date: Tue, 18 Oct 2022 10:57:11 +0200 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.