From: Stefan Sperling <stsp@stsp.name>
Subject: Re: Packfile permissions
To: Christian Weisgerber <naddy@mips.inka.de>
Cc: gameoftrees@openbsd.org
Date: Sun, 1 Nov 2020 13:29:06 +0100

On Sat, Oct 31, 2020 at 10:19:03PM +0100, Christian Weisgerber wrote:
> Stefan Sperling:
> 
> > Sure. OK by me. Using fchmod is clearly less racy. There might be similar
> > code elsewhere in the code base that could be fixed like this?
> 
> This converts all remaining instances of chmod() into fchmod().
> 
> Where the program logic allows, fchmod() moves into place next after
> file creation in case of fixed permissions, and next after a stat()
> if that's the source of the permissions.

Reads great and tests are passing. OK

> diff 0843a4ce462d9a691a3b5f38a465b129b69fdd5f /home/naddy/got
> blob - a1f3f64950abf6a28988d7499f93bc440c52d438
> file + lib/object_create.c
> --- lib/object_create.c
> +++ lib/object_create.c
> @@ -78,6 +78,11 @@ create_object_file(struct got_object_id *id, FILE *con
>  			goto done;
>  	}
>  
> +	if (fchmod(fileno(tmpfile), GOT_DEFAULT_FILE_MODE) != 0) {
> +		err = got_error_from_errno2("fchmod", tmppath);
> +		goto done;
> +	}
> +
>  	err = got_deflate_to_file(&tmplen, content, tmpfile);
>  	if (err)
>  		goto done;
> @@ -92,11 +97,6 @@ create_object_file(struct got_object_id *id, FILE *con
>  	}
>  	free(tmppath);
>  	tmppath = NULL;
> -
> -	if (chmod(objpath, GOT_DEFAULT_FILE_MODE) != 0) {
> -		err = got_error_from_errno2("chmod", objpath);
> -		goto done;
> -	}
>  done:
>  	free(objpath);
>  	if (tmppath) {
> blob - 2b33b7d2fe30a50299fc2d669978a76c66ce98c0
> file + lib/reference.c
> --- lib/reference.c
> +++ lib/reference.c
> @@ -1140,17 +1140,17 @@ got_ref_write(struct got_reference *ref, struct got_re
>  		sb.st_mode = GOT_DEFAULT_FILE_MODE;
>  	}
>  
> +	if (fchmod(fileno(f), sb.st_mode) != 0) {
> +		err = got_error_from_errno2("fchmod", tmppath);
> +		goto done;
> +	}
> +
>  	if (rename(tmppath, path) != 0) {
>  		err = got_error_from_errno3("rename", tmppath, path);
>  		goto done;
>  	}
>  	free(tmppath);
>  	tmppath = NULL;
> -
> -	if (chmod(path, sb.st_mode) != 0) {
> -		err = got_error_from_errno2("chmod", path);
> -		goto done;
> -	}
>  done:
>  	if (ref->lf == NULL && lf)
>  		unlock_err = got_lockfile_unlock(lf);
> @@ -1276,23 +1276,22 @@ delete_packed_ref(struct got_reference *delref, struct
>  			goto done;
>  		}
>  
> -		if (stat(packed_refs_path, &sb) != 0) {
> +		if (fstat(fileno(f), &sb) != 0) {
>  			if (errno != ENOENT) {
> -				err = got_error_from_errno2("stat",
> +				err = got_error_from_errno2("fstat",
>  				    packed_refs_path);
>  				goto done;
>  			}
>  			sb.st_mode = GOT_DEFAULT_FILE_MODE;
>  		}
>  
> -		if (rename(tmppath, packed_refs_path) != 0) {
> -			err = got_error_from_errno3("rename", tmppath,
> -			    packed_refs_path);
> +		if (fchmod(fileno(tmpf), sb.st_mode) != 0) {
> +			err = got_error_from_errno2("fchmod", tmppath);
>  			goto done;
>  		}
>  
> -		if (chmod(packed_refs_path, sb.st_mode) != 0) {
> -			err = got_error_from_errno2("chmod",
> +		if (rename(tmppath, packed_refs_path) != 0) {
> +			err = got_error_from_errno3("rename", tmppath,
>  			    packed_refs_path);
>  			goto done;
>  		}
> blob - 54a1c38fbbcfb2ede54f20d464e12db1a15f0447
> file + lib/worktree.c
> --- lib/worktree.c
> +++ lib/worktree.c
> @@ -968,6 +968,11 @@ install_symlink_conflict(const char *deriv_target,
>  	if (err)
>  		goto done;
>  
> +	if (fchmod(fileno(f), GOT_DEFAULT_FILE_MODE) == -1) {
> +		err = got_error_from_errno2("fchmod", path);
> +		goto done;
> +	}
> +
>  	if (fprintf(f, "%s %s\n%s\n%s%s%s%s%s\n%s\n%s\n",
>  	    GOT_DIFF_CONFLICT_MARKER_BEGIN, label_deriv,
>  	    deriv_target ? deriv_target : "(symlink was deleted)",
> @@ -989,10 +994,6 @@ install_symlink_conflict(const char *deriv_target,
>  		err = got_error_from_errno3("rename", path, ondisk_path);
>  		goto done;
>  	}
> -	if (chmod(ondisk_path, GOT_DEFAULT_FILE_MODE) == -1) {
> -		err = got_error_from_errno2("chmod", ondisk_path);
> -		goto done;
> -	}
>  done:
>  	if (f != NULL && fclose(f) == EOF && err == NULL)
>  		err = got_error_from_errno2("fclose", path);
> @@ -1511,6 +1512,12 @@ install_blob(struct got_worktree *worktree, const char
>  			return got_error_from_errno2("open", ondisk_path);
>  	}
>  
> +	if (fchmod(fd, get_ondisk_perms(te_mode & S_IXUSR, st_mode)) == -1) {
> +		err = got_error_from_errno2("fchmod",
> +		    update ? tmppath : ondisk_path);
> +		goto done;
> +	}
> +
>  	if (progress_cb) {
>  		if (restoring_missing_file)
>  			err = (*progress_cb)(progress_arg, GOT_STATUS_MISSING,
> @@ -1559,12 +1566,6 @@ install_blob(struct got_worktree *worktree, const char
>  		}
>  	}
>  
> -	if (chmod(ondisk_path,
> -	    get_ondisk_perms(te_mode & S_IXUSR, st_mode)) == -1) {
> -		err = got_error_from_errno2("chmod", ondisk_path);
> -		goto done;
> -	}
> -
>  done:
>  	if (fd != -1 && close(fd) != 0 && err == NULL)
>  		err = got_error_from_errno("close");
> @@ -4332,8 +4333,8 @@ create_patched_content(char **path_outfile, int revers
>  			goto done;
>  
>  		if (!S_ISLNK(sb2.st_mode)) {
> -			if (chmod(*path_outfile, sb2.st_mode) == -1) {
> -				err = got_error_from_errno2("chmod", path2);
> +			if (fchmod(fileno(outfile), sb2.st_mode) == -1) {
> +				err = got_error_from_errno2("fchmod", path2);
>  				goto done;
>  			}
>  		}
> -- 
> Christian "naddy" Weisgerber                          naddy@mips.inka.de
> 
>