From: Omar Polo Subject: Re: check for size_t overflow when mapping memory To: Stefan Sperling Cc: gameoftrees@openbsd.org Date: Mon, 24 Oct 2022 23:24:12 +0200 On 2022/10/24 22:13:48 +0200, Stefan Sperling wrote: > On Mon, Oct 24, 2022 at 09:57:38PM +0200, Omar Polo wrote: > > #ifndef GOT_PACK_NO_MMAP > > - if (hdrlen + size > 0) { > > - (*obj)->data = mmap(NULL, hdrlen + size, PROT_READ, > > + (*obj)->fd = *outfd; > > + *outfd = -1; > > + if (tot > 0 && tot < SIZE_MAX) { > > + (*obj)->data = mmap(NULL, tot, PROT_READ, > > MAP_PRIVATE, *outfd, 0); > > if ((*obj)->data == MAP_FAILED) { > > if (errno != ENOMEM) { > > @@ -958,9 +963,6 @@ got_object_raw_alloc(struct got_raw_object **obj, uint > > goto done; > > } > > (*obj)->data = NULL; > > - } else { > > - (*obj)->fd = *outfd; > > - *outfd = -1; > > } > > } > > #endif > > Won't setting *outfd to -1 in all cases disable the fallback to > stdio via obj->f (which is a FILE *) in case mmap fails? > The lines below this are as follow, and they would never run with > your change in place: > > if (*outfd != -1) { > (*obj)->f = fdopen(*outfd, "r"); > if ((*obj)->f == NULL) { > err = got_error_from_errno("fdopen"); > goto done; > } > *outfd = -1; > } yup, that change slipped from a previous attempt, but it's clearly wrong. i should have rechecked that part, apologise. > > blob - becc8ffd68217466f0bad6d811196c0d7137e4df > > blob + 0b79146e747f73787b2ee568b7fb74c517ce1d9e > > --- lib/pack.c > > +++ lib/pack.c > > @@ -392,13 +392,15 @@ got_packidx_open(struct got_packidx **packidx, > > } > > > > #ifndef GOT_PACK_NO_MMAP > > - p->map = mmap(NULL, p->len, PROT_READ, MAP_PRIVATE, p->fd, 0); > > - if (p->map == MAP_FAILED) { > > - if (errno != ENOMEM) { > > - err = got_error_from_errno("mmap"); > > - goto done; > > + if (p->len < SIZE_MAX) { > > Not a big deal, but I wonder if this should not say <= SIZE_MAX? > All such checks should also ensure that file size is greater 0. woops, right! fixed, thanks! diff f7b5105d849a571a34d8e5c2674b776679e9b5dd 24933640815f1defd057122da353993e1a60f843 commit - f7b5105d849a571a34d8e5c2674b776679e9b5dd commit + 24933640815f1defd057122da353993e1a60f843 blob - f8e685448165384cfc578c0f089b4aa6fb6219c8 blob + 7bc7eb3dcdcec57157f35f8b6c676e1979ba2b76 --- lib/object.c +++ lib/object.c @@ -926,7 +926,10 @@ got_object_raw_alloc(struct got_raw_object **obj, uint size_t hdrlen, off_t size) { const struct got_error *err = NULL; + off_t tot; + tot = hdrlen + size; + *obj = calloc(1, sizeof(**obj)); if (*obj == NULL) { err = got_error_from_errno("calloc"); @@ -944,13 +947,13 @@ got_object_raw_alloc(struct got_raw_object **obj, uint goto done; } - if (sb.st_size != hdrlen + size) { + if (sb.st_size != tot) { err = got_error(GOT_ERR_PRIVSEP_LEN); goto done; } #ifndef GOT_PACK_NO_MMAP - if (hdrlen + size > 0) { - (*obj)->data = mmap(NULL, hdrlen + size, PROT_READ, + if (tot > 0 && tot <= SIZE_MAX) { + (*obj)->data = mmap(NULL, tot, PROT_READ, MAP_PRIVATE, *outfd, 0); if ((*obj)->data == MAP_FAILED) { if (errno != ENOMEM) { blob - becc8ffd68217466f0bad6d811196c0d7137e4df blob + 5e8b181042e11b8efbc129bd3a9cfb8c448138d3 --- lib/pack.c +++ lib/pack.c @@ -392,13 +392,15 @@ got_packidx_open(struct got_packidx **packidx, } #ifndef GOT_PACK_NO_MMAP - p->map = mmap(NULL, p->len, PROT_READ, MAP_PRIVATE, p->fd, 0); - if (p->map == MAP_FAILED) { - if (errno != ENOMEM) { - err = got_error_from_errno("mmap"); - goto done; + if (p->len > 0 && p->len <= SIZE_MAX) { + p->map = mmap(NULL, p->len, PROT_READ, MAP_PRIVATE, p->fd, 0); + if (p->map == MAP_FAILED) { + if (errno != ENOMEM) { + err = got_error_from_errno("mmap"); + goto done; + } + p->map = NULL; /* fall back to read(2) */ } - p->map = NULL; /* fall back to read(2) */ } #endif @@ -1036,7 +1038,6 @@ resolve_offset_delta(struct got_delta_chain *deltas, resolve_offset_delta(struct got_delta_chain *deltas, struct got_packidx *packidx, struct got_pack *pack, off_t delta_offset, size_t tslen, int delta_type, size_t delta_size, unsigned int recursion) - { const struct got_error *err; off_t base_offset; blob - b07bf0c9c711997031a7d3afa3388a27527ef2db blob + 50237bbf7e4f90efc06e68c84cedd6c84631551e --- lib/repository.c +++ lib/repository.c @@ -1428,14 +1428,16 @@ got_repo_cache_pack(struct got_pack **packp, struct go goto done; #ifndef GOT_PACK_NO_MMAP - pack->map = mmap(NULL, pack->filesize, PROT_READ, MAP_PRIVATE, - pack->fd, 0); - if (pack->map == MAP_FAILED) { - if (errno != ENOMEM) { - err = got_error_from_errno("mmap"); - goto done; + if (pack->filesize > 0 && pack->filesize <= SIZE_MAX) { + pack->map = mmap(NULL, pack->filesize, PROT_READ, MAP_PRIVATE, + pack->fd, 0); + if (pack->map == MAP_FAILED) { + if (errno != ENOMEM) { + err = got_error_from_errno("mmap"); + goto done; + } + pack->map = NULL; /* fall back to read(2) */ } - pack->map = NULL; /* fall back to read(2) */ } #endif done: blob - 8a38382357c6aeb5b642965a26a50d9a6621d34e blob + 7fda0c4e47f3539c4b13427d388cc26e749b75bb --- libexec/got-index-pack/got-index-pack.c +++ libexec/got-index-pack/got-index-pack.c @@ -183,10 +183,12 @@ main(int argc, char **argv) } #ifndef GOT_PACK_NO_MMAP - pack.map = mmap(NULL, pack.filesize, PROT_READ, MAP_PRIVATE, - pack.fd, 0); - if (pack.map == MAP_FAILED) - pack.map = NULL; /* fall back to read(2) */ + if (pack.filesize > 0 && pack.filesize <= SIZE_MAX) { + pack.map = mmap(NULL, pack.filesize, PROT_READ, MAP_PRIVATE, + pack.fd, 0); + if (pack.map == MAP_FAILED) + pack.map = NULL; /* fall back to read(2) */ + } #endif err = got_pack_index(&pack, idxfd, tmpfiles[0], tmpfiles[1], tmpfiles[2], pack_hash, send_index_pack_progress, &ibuf, &rl); blob - 268ab18c5371e622b7f9c3a62c9a894a7050de85 blob + e49fd95ba0b7919efe9005b306782db008b22370 --- libexec/got-read-pack/got-read-pack.c +++ libexec/got-read-pack/got-read-pack.c @@ -1146,9 +1146,11 @@ receive_packidx(struct got_packidx **packidx, struct i } #ifndef GOT_PACK_NO_MMAP - p->map = mmap(NULL, p->len, PROT_READ, MAP_PRIVATE, p->fd, 0); - if (p->map == MAP_FAILED) - p->map = NULL; /* fall back to read(2) */ + if (p->len > 0 && p->len <= SIZE_MAX) { + p->map = mmap(NULL, p->len, PROT_READ, MAP_PRIVATE, p->fd, 0); + if (p->map == MAP_FAILED) + p->map = NULL; /* fall back to read(2) */ + } #endif err = got_packidx_init_hdr(p, 1, ipackidx.packfile_size); done: @@ -1874,10 +1876,12 @@ receive_pack(struct got_pack **packp, struct imsgbuf * goto done; #ifndef GOT_PACK_NO_MMAP - pack->map = mmap(NULL, pack->filesize, PROT_READ, MAP_PRIVATE, - pack->fd, 0); - if (pack->map == MAP_FAILED) - pack->map = NULL; /* fall back to read(2) */ + if (pack->filesize > 0 && pack->filesize <= SIZE_MAX) { + pack->map = mmap(NULL, pack->filesize, PROT_READ, MAP_PRIVATE, + pack->fd, 0); + if (pack->map == MAP_FAILED) + pack->map = NULL; /* fall back to read(2) */ + } #endif done: if (err) {