On Wed, Feb 01, 2023 at 10:37:45AM +0100, Omar Polo wrote:
> noticed while sharing /var/www/got/public between gotd and gotwebd.
> gotd installs the files as 0600 (this comes from mkstemps) and so are
> unreadable by gotwebd, while gotadmin does a
> fchmod(GOT_DEFAULT_FILE_MODE) (aka 0644).
> 
> `git repack' installs the packfiles/index/bitmaps as 0444 which seems
> a sane default since packfiles are not meant to change, so i'm
> proposing to do the same for gotd and gotadmin.

Indeed, in particular losing the w bit is important. We don't want
pack files to change after creation ever, and especially not while
they're mapped into memory.

It is also good for interop to align with Git's behaviour in this case.

ok stsp@

I am wondering if we should allow the current umask to strip the r bits
for group and others, such that someone running 'gotadmin pack' doesn't
accidentally create world-readable files. But that is a separate issue.

> 
> diff /tmp/got
> commit - 2aea6fc2c2c95ba4968b598247e00edb6f91048b
> path + /tmp/got
> blob - bbe5f89f20242a5571907a6f813d02388bacddf4
> file + gotd/session.c
> --- gotd/session.c
> +++ gotd/session.c
> @@ -18,6 +18,7 @@
>  #include <sys/queue.h>
>  #include <sys/tree.h>
>  #include <sys/socket.h>
> +#include <sys/stat.h>
>  #include <sys/uio.h>
>  
>  #include <errno.h>
> @@ -883,6 +884,10 @@ recv_packfile(struct gotd_session_client *client)
>  	err = got_opentemp_named_fd(&pack_path, &packfd, basepath, "");
>  	if (err)
>  		goto done;
> +	if (fchmod(packfd, GOT_DEFAULT_PACK_MODE) == -1) {
> +		err = got_error_from_errno2("fchmod", pack_path);
> +		goto done;
> +	}
>  
>  	free(basepath);
>  	if (asprintf(&basepath, "%s/%s/receiving-from-uid-%d.idx",
> @@ -895,6 +900,10 @@ recv_packfile(struct gotd_session_client *client)
>  	err = got_opentemp_named_fd(&idx_path, &idxfd, basepath, "");
>  	if (err)
>  		goto done;
> +	if (fchmod(idxfd, GOT_DEFAULT_PACK_MODE) == -1) {
> +		err = got_error_from_errno2("fchmod", idx_path);
> +		goto done;
> +	}
>  
>  	memset(&ifile, 0, sizeof(ifile));
>  	ifile.client_id = client->id;
> blob - aa121bb3af4c0cba66fb0d062f2f6a8cc33fdd27
> file + include/got_path.h
> --- include/got_path.h
> +++ include/got_path.h
> @@ -16,6 +16,8 @@
>  
>  /* Utilities for dealing with filesystem paths. */
>  
> +#define GOT_DEFAULT_PACK_MODE	(S_IFREG | \
> +	S_IRUSR | S_IRGRP | S_IROTH)
>  #define GOT_DEFAULT_FILE_MODE	(S_IFREG | \
>  	S_IRUSR|S_IWUSR | S_IRGRP | S_IROTH)
>  #define GOT_DEFAULT_DIR_MODE	(S_IFDIR | \
> blob - fd1fae8ec1b8171b5cb576110a381201f1cb4ad9
> file + lib/repository_admin.c
> --- lib/repository_admin.c
> +++ lib/repository_admin.c
> @@ -171,7 +171,7 @@ got_repo_pack_objects(FILE **packfile, struct got_obje
>  	if (err)
>  		goto done;
>  
> -	if (fchmod(packfd, GOT_DEFAULT_FILE_MODE) != 0) {
> +	if (fchmod(packfd, GOT_DEFAULT_PACK_MODE) == -1) {
>  		err = got_error_from_errno2("fchmod", tmpfile_path);
>  		goto done;
>  	}
> @@ -301,7 +301,7 @@ got_repo_index_pack(FILE *packfile, struct got_object_
>  	free(path);
>  	if (err)
>  		goto done;
> -	if (fchmod(idxfd, GOT_DEFAULT_FILE_MODE) != 0) {
> +	if (fchmod(idxfd, GOT_DEFAULT_PACK_MODE) == -1) {
>  		err = got_error_from_errno2("fchmod", tmpidxpath);
>  		goto done;
>  	}
> 
>