On Wed, Dec 04, 2013 at 07:36:57PM +0100, Stefan Sperling wrote:
> On Wed, Dec 04, 2013 at 06:16:40PM -0000, phi...@apache.org wrote:
> > Author: philip
> > Date: Wed Dec 4 18:16:40 2013
> > New Revision: 1547866
> >
> > URL: http://svn.apache.org/r1547866
> > Log:
> > Fix issue 3437, rep-cache.db created with wrong permissions.
> >
> > * subversion/libsvn_fs_fs/rep-cache.c
> > (open_rep_cache): Create file before calling into SQLite.
>
> Hey Philip,
>
> I was just about to commit this patch and log message.
> Do you see anything you like in here?
>
> [[[
> Fix issue #3437, "rep-cache.db created without group write bit".
>
> Subversion now honours the umask when creating new rep-cache.db databases,
> both during 'svnadmin create' as well as during normal operation (in which
> case the umask of the server process is used).
>
> Note that 'svnadmin create' didn't create a rep-cache.db previously.
> But I don't see any problem with making it do that for purposes of
> file permission consistency.
>
> * subversion/include/private/svn_io_private.h
> (svn_io__merge_default_file_perms): Declare.
>
> * subversion/libsvn_fs_fs/fs_fs.c
> (svn_fs_fs__create): Create rep-cache.db during FSFS creation so that
> permissions are set up in accordance with the umask of the 'svnadmin
> create' process.
>
> * subversion/libsvn_fs_fs/rep-cache.c
> (open_rep_cache): If the rep-cache is newly created, tweak its permissions
> in accordance with the current umask. An existance check is performed every
> time the rep-cache is first opened. I hope the overhead of an extra stat is
> acceptable. No change on Windows, since we don't deal with permissions
> there anyway.
>
> * subversion/libsvn_subr/io.c
> (merge_default_file_perms): Rename to...
> (svn_io__merge_default_file_perms): ... this, to expose it in the private
> API namespace.
> (svn_io_open_unique_file3): Adjust caller of merge_default_file_perms().
>
> ]]]
>
Oops, I pasted an old version of my diff. Here's the final one,
mostly with adjusted comments.
Overall, I like your trick with creating an empty file better.
What do you think about making 'svnadmin create' create rep-cache.db?
Index: subversion/include/private/svn_io_private.h
===================================================================
--- subversion/include/private/svn_io_private.h (revision 1547845)
+++ subversion/include/private/svn_io_private.h (working copy)
@@ -109,5 +109,11 @@
}
#endif /* __cplusplus */
+/* OR together permission bits of the file FD and the default permissions
+ of a file as determined by get_default_file_perms(). Do temporary
+ allocations in SCRATCH_POOL. */
+svn_error_t *
+svn_io__merge_default_file_perms(apr_file_t *fd, apr_fileperms_t *perms,
+ apr_pool_t *scratch_pool);
#endif /* SVN_IO_PRIVATE_H */
Index: subversion/libsvn_fs_fs/fs_fs.c
===================================================================
--- subversion/libsvn_fs_fs/fs_fs.c (revision 1547845)
+++ subversion/libsvn_fs_fs/fs_fs.c (working copy)
@@ -1277,6 +1277,11 @@
pool));
}
+ /* Issue #3437: Create a rep-cache.db if rep-caching is supported
+ * to set its access permissions in accordance with the umask. */
+ if (format >= SVN_FS_FS__MIN_REP_SHARING_FORMAT)
+ SVN_ERR(svn_fs_fs__open_rep_cache(fs, pool));
+
/* This filesystem is ready. Stamp it with a format number. */
SVN_ERR(svn_fs_fs__write_format(fs, FALSE, pool));
Index: subversion/libsvn_fs_fs/rep-cache.c
===================================================================
--- subversion/libsvn_fs_fs/rep-cache.c (revision 1547845)
+++ subversion/libsvn_fs_fs/rep-cache.c (working copy)
@@ -79,7 +79,12 @@
svn_sqlite__db_t *sdb;
const char *db_path;
int version;
+#ifndef WIN32
+ svn_boolean_t rep_cache_existed;
+ SVN_ERR(svn_fs_fs__exists_rep_cache(&rep_cache_existed, fs, pool));
+#endif
+
/* Open (or create) the sqlite database. It will be automatically
closed when fs->pool is destoyed. */
db_path = path_rep_cache_db(fs->path, pool);
@@ -100,6 +105,22 @@
set it earlier. */
ffd->rep_cache_db = sdb;
+#ifndef WIN32
+ if (!rep_cache_existed)
+ {
+ apr_file_t *db_fd;
+ apr_fileperms_t perms;
+
+ /* Issue 3437: When creating a new db, we must tweak access permissions
+ in accordance with the umask because sqlite doesn't do this. */
+ SVN_ERR(svn_io_file_open(&db_fd, db_path, APR_READ, APR_OS_DEFAULT,
+ pool));
+ SVN_ERR(svn_io__merge_default_file_perms(db_fd, &perms, pool));
+ SVN_ERR(apr_file_perms_set(db_path, perms));
+ SVN_ERR(svn_io_file_close(db_fd, pool));
+ }
+#endif
+
return SVN_NO_ERROR;
}
Index: subversion/libsvn_subr/io.c
===================================================================
--- subversion/libsvn_subr/io.c (revision 1547845)
+++ subversion/libsvn_subr/io.c (working copy)
@@ -1533,9 +1533,9 @@
/* OR together permission bits of the file FD and the default permissions
of a file as determined by get_default_file_perms(). Do temporary
allocations in SCRATCH_POOL. */
-static svn_error_t *
-merge_default_file_perms(apr_file_t *fd, apr_fileperms_t *perms,
- apr_pool_t *scratch_pool)
+svn_error_t *
+svn_io__merge_default_file_perms(apr_file_t *fd, apr_fileperms_t *perms,
+ apr_pool_t *scratch_pool)
{
apr_finfo_t finfo;
apr_fileperms_t default_perms;
@@ -4993,7 +4993,7 @@
* case, but only if the umask allows it. */
if (!using_system_temp_dir)
{
- SVN_ERR(merge_default_file_perms(tempfile, &perms, scratch_pool));
+ SVN_ERR(svn_io__merge_default_file_perms(tempfile, &perms,
scratch_pool));
SVN_ERR(file_perms_set2(tempfile, perms, scratch_pool));
}
#endif
