| Message ID | aa86e8df403eef31295e6036f280995fa74cc3b4.1587422630.git.me@ttaylorr.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | commit-graph: write non-split graphs as read-only | expand |
Taylor Blau <me@ttaylorr.com> writes: > Arguably, all temporary files should have permission 0444, since they > are likely to be renamed into place and then not written to again. If the repository is not shared, it is more than likely that these files ought to have permission narrower than that. As you'd be concluding any file that you created and written to in $GIT_DIR/ with adjust_shared_perm(), I do not think it is a good idea to allow callers to pass an arbitrary mode and let them expect the mode would stick. Shoudln't we instead be passing "int read_only" and depending on the boolean value, choosing between 0444 and 0666 in the function? > Signed-off-by: Taylor Blau <me@ttaylorr.com> > --- > tempfile.c | 6 +++--- > tempfile.h | 7 ++++++- > 2 files changed, 9 insertions(+), 4 deletions(-) > > diff --git a/tempfile.c b/tempfile.c > index d43ad8c191..94aa18f3f7 100644 > --- a/tempfile.c > +++ b/tempfile.c > @@ -130,17 +130,17 @@ static void deactivate_tempfile(struct tempfile *tempfile) > } > > /* Make sure errno contains a meaningful value on error */ > -struct tempfile *create_tempfile(const char *path) > +struct tempfile *create_tempfile_mode(const char *path, int mode) > { > struct tempfile *tempfile = new_tempfile(); > > strbuf_add_absolute_path(&tempfile->filename, path); > tempfile->fd = open(tempfile->filename.buf, > - O_RDWR | O_CREAT | O_EXCL | O_CLOEXEC, 0666); > + O_RDWR | O_CREAT | O_EXCL | O_CLOEXEC, mode); > if (O_CLOEXEC && tempfile->fd < 0 && errno == EINVAL) > /* Try again w/o O_CLOEXEC: the kernel might not support it */ > tempfile->fd = open(tempfile->filename.buf, > - O_RDWR | O_CREAT | O_EXCL, 0666); > + O_RDWR | O_CREAT | O_EXCL, mode); > if (tempfile->fd < 0) { > deactivate_tempfile(tempfile); > return NULL; > diff --git a/tempfile.h b/tempfile.h > index cddda0a33c..b5760d4581 100644 > --- a/tempfile.h > +++ b/tempfile.h > @@ -89,7 +89,12 @@ struct tempfile { > * a tempfile (whose "fd" member can be used for writing to it), or > * NULL on error. It is an error if a file already exists at that path. > */ > -struct tempfile *create_tempfile(const char *path); > +struct tempfile *create_tempfile_mode(const char *path, int mode); > + > +static inline struct tempfile *create_tempfile(const char *path) > +{ > + return create_tempfile_mode(path, 0666); > +} > > /* > * Register an existing file as a tempfile, meaning that it will be
diff --git a/tempfile.c b/tempfile.c index d43ad8c191..94aa18f3f7 100644 --- a/tempfile.c +++ b/tempfile.c @@ -130,17 +130,17 @@ static void deactivate_tempfile(struct tempfile *tempfile) } /* Make sure errno contains a meaningful value on error */ -struct tempfile *create_tempfile(const char *path) +struct tempfile *create_tempfile_mode(const char *path, int mode) { struct tempfile *tempfile = new_tempfile(); strbuf_add_absolute_path(&tempfile->filename, path); tempfile->fd = open(tempfile->filename.buf, - O_RDWR | O_CREAT | O_EXCL | O_CLOEXEC, 0666); + O_RDWR | O_CREAT | O_EXCL | O_CLOEXEC, mode); if (O_CLOEXEC && tempfile->fd < 0 && errno == EINVAL) /* Try again w/o O_CLOEXEC: the kernel might not support it */ tempfile->fd = open(tempfile->filename.buf, - O_RDWR | O_CREAT | O_EXCL, 0666); + O_RDWR | O_CREAT | O_EXCL, mode); if (tempfile->fd < 0) { deactivate_tempfile(tempfile); return NULL; diff --git a/tempfile.h b/tempfile.h index cddda0a33c..b5760d4581 100644 --- a/tempfile.h +++ b/tempfile.h @@ -89,7 +89,12 @@ struct tempfile { * a tempfile (whose "fd" member can be used for writing to it), or * NULL on error. It is an error if a file already exists at that path. */ -struct tempfile *create_tempfile(const char *path); +struct tempfile *create_tempfile_mode(const char *path, int mode); + +static inline struct tempfile *create_tempfile(const char *path) +{ + return create_tempfile_mode(path, 0666); +} /* * Register an existing file as a tempfile, meaning that it will be
In the next patch, 'hold_lock_file_for_update' will gain an additional 'mode' parameter to specify permissions for the associated temporary file. Since the lockfile.c machinery uses 'create_tempfile' which always creates a temporary file with global read-write permissions, introduce a variant here that allows specifying the mode. Arguably, all temporary files should have permission 0444, since they are likely to be renamed into place and then not written to again. This is a much larger change than we may want to take on in this otherwise small patch, so for the time being, make 'create_tempfile' behave as it has always done by inlining it to 'create_tempfile_mode' with mode set to '0666'. Signed-off-by: Taylor Blau <me@ttaylorr.com> --- tempfile.c | 6 +++--- tempfile.h | 7 ++++++- 2 files changed, 9 insertions(+), 4 deletions(-)