From: Stefan Sperling <stsp@stsp.name>
Subject: Re: Packfile permissions
To: Sebastien Marie <semarie@online.fr>
Cc: Alisdair MacLeod <got@alisdairmacleod.co.uk>, gameoftrees@openbsd.org
Date: Thu, 29 Oct 2020 21:57:05 +0100

On Wed, Oct 28, 2020 at 05:58:04PM +0100, Sebastien Marie wrote:
> On Wed, Oct 28, 2020 at 04:56:56PM +0100, Stefan Sperling wrote:
> > On Wed, Oct 28, 2020 at 12:38:15PM +0000, Alisdair MacLeod wrote:
> > > Here is a patch that I am currently running successfully on my server.
> > 
> > Thank you! Your patch is now in the main repository.
> 
> Per habit, I am not really fan of chmod(2) usage, specially when it is
> as simple to use fchmod(2) (we opened the files for writing just
> before).
> 
> The diff below should be equivalent in result. It uses fchmod(2) just
> after opening the file.
> 
> Thanks.

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?
 
> diff 1c25ba414b5c74d25a8745f25673abb74be2dd42 /home/semarie/repos/openbsd/got
> blob - 0e60b2156e5a9ef85594f7ddccaba178ac16f667
> file + lib/fetch.c
> --- lib/fetch.c
> +++ lib/fetch.c
> @@ -491,37 +491,45 @@ got_fetch_pack(struct got_object_id **pack_hash, struc
>  	} else {
>  		if (asprintf(&path, "%s/%s/fetching.pack",
>  		    repo_path, GOT_OBJECTS_PACK_DIR) == -1) {
>  			err = got_error_from_errno("asprintf");
>  			goto done;
>  		}
>  		err = got_opentemp_named_fd(&tmppackpath, &packfd, path);
>  		free(path);
>  		if (err)
>  			goto done;
> +		if (fchmod(packfd, GOT_DEFAULT_FILE_MODE) != 0) {
> +			err = got_error_from_errno2("fchmod", tmppackpath);
> +			goto done;
> +		}
>  	}
>  	if (list_refs_only) {
>  		idxfd = got_opentempfd();
>  		if (idxfd == -1) {
>  			err = got_error_from_errno("got_opentempfd");
>  			goto done;
>  		}
>  	} else {
>  		if (asprintf(&path, "%s/%s/fetching.idx",
>  		    repo_path, GOT_OBJECTS_PACK_DIR) == -1) {
>  			err = got_error_from_errno("asprintf");
>  			goto done;
>  		}
>  		err = got_opentemp_named_fd(&tmpidxpath, &idxfd, path);
>  		free(path);
>  		if (err)
>  			goto done;
> +		if (fchmod(idxfd, GOT_DEFAULT_FILE_MODE) != 0) {
> +			err = got_error_from_errno2("fchmod", tmpidxpath);
> +			goto done;
> +		}
>  	}
>  	nidxfd = dup(idxfd);
>  	if (nidxfd == -1) {
>  		err = got_error_from_errno("dup");
>  		goto done;
>  	}
>  
>  	for (i = 0; i < nitems(tmpfds); i++) {
>  		tmpfds[i] = got_opentempfd();
>  		if (tmpfds[i] == -1) {
> @@ -792,29 +800,20 @@ got_fetch_pack(struct got_object_id **pack_hash, struc
>  		goto done;
>  	}
>  	free(tmppackpath);
>  	tmppackpath = NULL;
>  	if (rename(tmpidxpath, idxpath) == -1) {
>  		err = got_error_from_errno3("rename", tmpidxpath, idxpath);
>  		goto done;
>  	}
>  	free(tmpidxpath);
>  	tmpidxpath = NULL;
> -
> -	if (chmod(packpath, GOT_DEFAULT_FILE_MODE) != 0) {
> -		err = got_error_from_errno2("chmod", packpath);
> -		goto done;
> -	}
> -	if (chmod(idxpath, GOT_DEFAULT_FILE_MODE) != 0) {
> -		err = got_error_from_errno2("chmod", idxpath);
> -		goto done;
> -	}
>  
>  done:
>  	if (tmppackpath && unlink(tmppackpath) == -1 && err == NULL)
>  		err = got_error_from_errno2("unlink", tmppackpath);
>  	if (tmpidxpath && unlink(tmpidxpath) == -1 && err == NULL)
>  		err = got_error_from_errno2("unlink", tmpidxpath);
>  	if (nfetchfd != -1 && close(nfetchfd) == -1 && err == NULL)
>  		err = got_error_from_errno("close");
>  	if (npackfd != -1 && close(npackfd) == -1 && err == NULL)
>  		err = got_error_from_errno("close");
> 
>