From: Tracey Emery <tracey@traceyemery.net>
Subject: Re: fix tempfile handling in gotd's got_object_raw_open()
To: gameoftrees@openbsd.org
Date: Mon, 9 Jan 2023 13:37:05 -0700

On Mon, Jan 09, 2023 at 09:19:42PM +0100, Stefan Sperling wrote:
> There is a bug where we reuse a file stored in *outfd from a
> previous call, resulting in a raw object backed by a file but
> with tempfile_idx -1. This causes bad confusion during deltification.
> Fix this by passing tempfd to read_raw() functions and only
> setting *outfd in case it is actually required, and returning
> tempfd to the repository tempfile pool otherwise.
> 
> With this, a final "input/output" error I see while cloning
> ports.git from gotd disappears. I attributed this error to
> having egdb attached to gotd at first, but that was wrong.
> 
> The problem seen during deltification was that an object
> ended up being backed by the wrong file, namely a slightly
> smaller file which contained the delta-base instead of the object.
> gotd would then an attempt to read beyond the end of this file.

ok

>  
> diff 60c140aebbd1069094e75d8358cb305bc305b7f8 639adcb774ebc1459df58b791ffcc037002de351
> commit - 60c140aebbd1069094e75d8358cb305bc305b7f8
> commit + 639adcb774ebc1459df58b791ffcc037002de351
> blob - 312719b3bcc84abe056dab97015657c4a30442f6
> blob + dc5a7253bb299918f457cf7b0a2116b13328d7b1
> --- lib/object_open_io.c
> +++ lib/object_open_io.c
> @@ -250,7 +250,7 @@ got_object_raw_open(struct got_raw_object **obj, int *
>  {
>  	const struct got_error *err = NULL;
>  	struct got_packidx *packidx = NULL;
> -	int idx, tempfile_idx = -1;
> +	int idx, tempfd, tempfile_idx;
>  	uint8_t *outbuf = NULL;
>  	off_t size = 0;
>  	size_t hdrlen = 0;
> @@ -262,21 +262,10 @@ got_object_raw_open(struct got_raw_object **obj, int *
>  		return NULL;
>  	}
>  
> -	if (*outfd == -1) {
> -		int tempfd;
> +	err = got_repo_temp_fds_get(&tempfd, &tempfile_idx, repo);
> +	if (err)
> +		return err;
>  
> -		err = got_repo_temp_fds_get(&tempfd, &tempfile_idx, repo);
> -		if (err)
> -			return err;
> -
> -		/* Duplicate tempfile descriptor to allow use of fdopen(3). */
> -		*outfd = dup(tempfd);
> -		if (*outfd == -1) {
> -			got_repo_temp_fds_put(tempfile_idx, repo);
> -			return got_error_from_errno("dup");
> -		}
> -	}
> -
>  	err = got_repo_search_packidx(&packidx, &idx, repo, id);
>  	if (err == NULL) {
>  		struct got_pack *pack = NULL;
> @@ -294,7 +283,7 @@ got_object_raw_open(struct got_raw_object **obj, int *
>  				goto done;
>  		}
>  		err = read_packed_object_raw(&outbuf, &size, &hdrlen,
> -		    *outfd, pack, packidx, idx, id);
> +		    tempfd, pack, packidx, idx, id);
>  		if (err)
>  			goto done;
>  	} else if (err->code == GOT_ERR_NO_OBJ) {
> @@ -304,13 +293,30 @@ got_object_raw_open(struct got_raw_object **obj, int *
>  		if (err)
>  			goto done;
>  		err = got_object_read_raw(&outbuf, &size, &hdrlen,
> -		    GOT_DELTA_RESULT_SIZE_CACHED_MAX, *outfd, id, fd);
> +		    GOT_DELTA_RESULT_SIZE_CACHED_MAX, tempfd, id, fd);
>  		if (close(fd) == -1 && err == NULL)
>  			err = got_error_from_errno("close");
>  		if (err)
>  			goto done;
>  	}
>  
> +	if (outbuf == NULL) {
> +		if (*outfd != -1) {
> +			err = got_error_msg(GOT_ERR_NOT_IMPL, "bad outfd");
> +			goto done;
> +		}
> +
> +		/*
> +		 * Duplicate tempfile descriptor to allow use of
> +		 * fdopen(3) inside got_object_raw_alloc().
> +		 */
> +		*outfd = dup(tempfd);
> +		if (*outfd == -1) {
> +			err = got_error_from_errno("dup");
> +			goto done;
> +		}
> +	}
> +		
>  	err = got_object_raw_alloc(obj, outbuf, outfd,
>  	    GOT_DELTA_RESULT_SIZE_CACHED_MAX, hdrlen, size);
>  	if (err)
> @@ -330,12 +336,24 @@ done:
>  			*obj = NULL;
>  		}
>  		free(outbuf);
> -		if (tempfile_idx != -1)
> -			got_repo_temp_fds_put(tempfile_idx, repo);
> +		got_repo_temp_fds_put(tempfile_idx, repo);
> +		if (*outfd != -1) {
> +			close(*outfd);
> +			*outfd = -1;
> +		}
>  	} else {
> -		(*obj)->tempfile_idx = tempfile_idx;
> -		(*obj)->close_cb = put_raw_object_tempfile;
> -		(*obj)->close_arg = repo;
> +		if (((*obj)->f == NULL && (*obj)->fd == -1)) {
> +			/* This raw object is not backed by a file. */
> +			got_repo_temp_fds_put(tempfile_idx, repo);
> +			if (*outfd != -1) {
> +				close(*outfd);
> +				*outfd = -1;
> +			}
> +		} else {
> +			(*obj)->tempfile_idx = tempfile_idx;
> +			(*obj)->close_cb = put_raw_object_tempfile;
> +			(*obj)->close_arg = repo;
> +		}
>  	}
>  	return err;
>  }
> 

-- 
Tracey Emery
IT Manager/Security Officer

Hospice Visions, Inc.
(208) 735-0121
http://hospicevisions.org

Visions Home Health & Visions Home Care, LLC.
(208) 732-5365
http://visionshomecare.com

DISCLOSURE:
This message is intended only for the use of the individual(s) to whom
it is addressed and contains information that is privileged,
confidential, and exempt from disclosure under applicable law. Any
further dissemination or copying of this communication is strictly
prohibited. If you have received this communication in error, please
notify us immediately by telephone or email as listed in our signature
above. This message is provided in accordance with HIPAA Omnibus Rule
of 2013.