From: Stefan Sperling <stsp@stsp.name>
Subject: Re: check for size_t overflow when mapping memory
To: Omar Polo <op@omarpolo.com>
Cc: gameoftrees@openbsd.org
Date: Mon, 24 Oct 2022 22:13:48 +0200

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;
		}

> 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.

> +		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 + 7372c43fc3a9bd8d47fb3d868c90311eb3c51451
> --- lib/repository.c
> +++ lib/repository.c
> @@ -1427,15 +1427,18 @@ got_repo_cache_pack(struct got_pack **packp, struct go
>  	if (err)
>  		goto done;
>  
> +	pack->map = NULL;
>  #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 < 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 + e914ae79dd30bfd6591ea94dfadb6d91ddd80c65
> --- 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 < 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 + fd77f35cfff0c52c0cf09457929ce3c48d8470c8
> --- 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 < 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 < 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) {
> 
> 
>