On Tue, Jun 28, 2022 at 04:36:28AM -0600, Tracey Emery wrote:
> This moves got_opentempfd out of open_blob. All regress passes. There
> are also a couple of white-space removals in here.

> @@ -3575,8 +3586,12 @@ diff_blobs(struct got_object_id *blob_id1, struct got_
>  	err = got_diff_blob(NULL, NULL, blob1, blob2, f1, f2, path, path,
>  	    diff_context, ignore_whitespace, force_text_diff, outfile);
>  done:
> +	if (fd1 != -1 && close(fd1) != 0 && err == NULL)

Please check close() == -1 instead of != 0.
The man page says errno is valid after -1 was returned.
There are more instances of != 0 in this patch which should be adjusted.

> @@ -4619,12 +4634,16 @@ print_diff(void *arg, unsigned char status, unsigned c
>  	struct print_diff_arg *a = arg;
>  	const struct got_error *err = NULL;
>  	struct got_blob_object *blob1 = NULL;
> -	int fd = -1;
> +	int fd = -1, fd1 = -1;
>  	FILE *f1 = NULL, *f2 = NULL;
>  	char *abspath = NULL, *label1 = NULL;
>  	struct stat sb;
>  	off_t size1 = 0;
>  
> +	fd1 = got_opentempfd();
> +	if (fd1 == -1)
> +		return got_error_from_errno("got_opentempfd");
> +
>  	if (a->diff_staged) {
>  		if (staged_status != GOT_STATUS_MODIFY &&
>  		    staged_status != GOT_STATUS_ADD &&

Error handling in here does 'return err' and will now leak this open fd.

> @@ -12229,12 +12257,19 @@ cat_blob(struct got_object_id *id, struct got_reposito
>  {
>  	const struct got_error *err;
>  	struct got_blob_object *blob;
> +	int fd = -1;
>  
> -	err = got_object_open_as_blob(&blob, repo, id, 8192);
> +	fd = got_opentempfd();
> +	if (fd == -1)
> +		return got_error_from_errno("got_opentempfd");
> +
> +	err = got_object_open_as_blob(&blob, repo, id, 8192, fd);
>  	if (err)
>  		return err;

The above error case leaks an open fd as well.

> blob - 5e6b96770dafdf895ef7140b2fc28b3d2c8e370b
> file + lib/blame.c
> --- lib/blame.c
> +++ lib/blame.c
> @@ -204,7 +204,12 @@ blame_commit(struct got_blame *blame, struct got_objec
>  	struct got_object_id *pblob_id = NULL;
>  	struct got_blob_object *pblob = NULL;
>  	struct diff_result *diff_result = NULL;
> +	int fd = -1;
>  
> +	fd = got_opentempfd();
> +	if (fd == -1)
> +		return got_error_from_errno("got_opentempfd");
> +
>  	err = got_object_open_as_commit(&commit, repo, id);
>  	if (err)
>  		return err;

Above error case leaks fd, too.

> blob - 773169c32d3cb119ac330201afe43d53b115085a
> file + lib/diff.c
> --- lib/diff.c
> +++ lib/diff.c
> @@ -280,18 +280,25 @@ diff_added_blob(struct got_object_id *id, FILE *f, con
>  	const struct got_error *err;
>  	struct got_blob_object  *blob = NULL;
>  	struct got_object *obj = NULL;
> +	int fd = -1;
>  
> +	fd = got_opentempfd();
> +	if (fd == -1)
> +		return got_error_from_errno("got_opentempfd");
> +
>  	err = got_object_open(&obj, repo, id);
>  	if (err)
>  		return err;

Another leak above.

This brings opentemp() calls back to lib/diff.c, so I guess we'll have
to do more follow-up work here to move them up even further...

> @@ -354,18 +376,25 @@ diff_deleted_blob(struct got_object_id *id, FILE *f, c
>  	const struct got_error *err;
>  	struct got_blob_object  *blob = NULL;
>  	struct got_object *obj = NULL;
> +	int fd = -1;
>  
> +	fd = got_opentempfd();
> +	if (fd == -1)
> +		return got_error_from_errno("got_opentempfd");
> +
>  	err = got_object_open(&obj, repo, id);
>  	if (err)
>  		return err;

Leaks fd on error.

> @@ -820,13 +872,13 @@ diff_paths(struct got_tree_object *tree1, struct got_t
>  		    type2 == GOT_OBJ_TYPE_BLOB) {
>  			if (id1) {
>  				err = got_object_open_as_blob(&blob1, repo,
> -				    id1, 8192);
> +				    id1, 8192, fd1);
>  				if (err)
>  					goto done;
>  			}
>  			if (id2) {
>  				err = got_object_open_as_blob(&blob2, repo,
> -				    id2, 8192);
> +				    id2, 8192, fd2);
>  				if (err)
>  					goto done;
>  			}

Above code runs in a loop.
Don't we need to call ftruncate() on fd1/fd2 before reusing them?

> blob - 06dc960b0d46db574b800f47ce6891fc926fa989
> file + lib/object.c
> --- lib/object.c
> +++ lib/object.c
> @@ -1160,18 +1160,25 @@ got_tree_entry_get_symlink_target(char **link_target, 
>  {
>  	const struct got_error *err = NULL;
>  	struct got_blob_object *blob = NULL;
> +	int fd = -1;
>  
> +	fd = got_opentempfd();
> +	if (fd == -1)
> +		return got_error_from_errno("got_opentempfd");
> +
>  	*link_target = NULL;
>  
>  	if (!got_object_tree_entry_is_symlink(te))
>  		return got_error(GOT_ERR_TREE_ENTRY_TYPE);

Leaks fd on error.

>  
>  	err = got_object_open_as_blob(&blob, repo,
> -	    got_tree_entry_get_id(te), PATH_MAX);
> +	    got_tree_entry_get_id(te), PATH_MAX, fd);
>  	if (err)
>  		return err;

Leaks fd on error.

>  const struct got_error *
> blob - 1bc4c535c2483d8ebfffd91a5553da6d6da60ce8
> file + lib/patch.c
> --- lib/patch.c
> +++ lib/patch.c
> @@ -588,7 +588,12 @@ open_blob(char **path, FILE **fp, const char *blobid,
>  	const struct got_error *err = NULL;
>  	struct got_blob_object *blob = NULL;
>  	struct got_object_id id, *idptr, *matched_id = NULL;
> +	int fd = -1;
>  
> +	fd = got_opentempfd();
> +	if (fd == -1)
> +		return got_error_from_errno("got_opentempfd");
> +
>  	*fp = NULL;
>  	*path = NULL;

There are 'return err' below in this function which now leak fd.
  
> blob - ae1ce6d136c77ab98e72f28c8a75c47893044c9b
> file + lib/worktree.c
> --- lib/worktree.c
> +++ lib/worktree.c
> @@ -4405,7 +4433,7 @@ create_patched_content(char **path_outfile, int revers
>  	const struct got_error *err, *free_err;
>  	struct got_blob_object *blob = NULL;
>  	FILE *f1 = NULL, *f2 = NULL, *outfile = NULL;
> -	int fd2 = -1;
> +	int fd = -1, fd2 = -1;
>  	char link_target[PATH_MAX];
>  	ssize_t link_len = 0;
>  	char *path1 = NULL, *id_str = NULL;
> @@ -4416,6 +4444,10 @@ create_patched_content(char **path_outfile, int revers
>  
>  	*path_outfile = NULL;
>  
> +	fd = got_opentempfd();
> +	if (fd == -1)
> +		return got_error_from_errno("got_opentempfd");
> +
>  	err = got_object_id_str(&id_str, blob_id);
>  	if (err)
>  		return err;

Leaks fd.

> @@ -4589,7 +4623,12 @@ revert_file(void *arg, unsigned char status, unsigned 
>  	char *tree_path = NULL, *te_name;
>  	char *ondisk_path = NULL, *path_content = NULL;
>  	struct got_blob_object *blob = NULL;
> +	int fd = -1;
>  
> +	fd = got_opentempfd();
> +	if (fd == -1)
> +		return got_error_from_errno("got_opentempfd");
> +
>  	/* Reverting a staged deletion is a no-op. */
>  	if (status == GOT_STATUS_DELETE &&
>  	    staged_status != GOT_STATUS_NO_CHANGE)

Another 3 leaks in error paths below here.


> @@ -8254,14 +8295,24 @@ create_unstaged_content(char **path_unstaged_content,
>  	struct got_diffreg_result *diffreg_result = NULL;
>  	int line_cur1 = 1, line_cur2 = 1, n = 0, nchunks_used = 0;
>  	int have_content = 0, have_rejected_content = 0, i = 0, nchanges = 0;
> +	int fd1 = -1, fd2 = -1;
>  
> +	fd1 = got_opentempfd();
> +	if (fd1 == -1)
> +		return got_error_from_errno("got_opentempfd");
> +	fd2 = got_opentempfd();
> +	if (fd2 == -1) {
> +		err = got_error_from_errno("got_opentempfd");
> +		goto done;
> +	}
> +
>  	*path_unstaged_content = NULL;
>  	*path_new_staged_content = NULL;
>  
>  	err = got_object_id_str(&label1, blob_id);
>  	if (err)
>  		return err;

Leak.

> @@ -8537,7 +8593,17 @@ unstage_path(void *arg, unsigned char status,
>  	char *id_str = NULL, *label_orig = NULL;
>  	int local_changes_subsumed;
>  	struct stat sb;
> +	int fd1 = -1, fd2 = -1;
>  
> +	fd1 = got_opentempfd();
> +	if (fd1 == -1)
> +		return got_error_from_errno("got_opentempfd");
> +	fd2 = got_opentempfd();
> +	if (fd2 == -1) {
> +		err = got_error_from_errno("got_opentempfd");
> +		goto done;
> +	}
> +
>  	if (staged_status != GOT_STATUS_ADD &&
>  	    staged_status != GOT_STATUS_MODIFY &&
>  	    staged_status != GOT_STATUS_DELETE)

More leak problems below here.