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().
]]]
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,12 @@ svn_stream__aprfile(svn_stream_t *stream);
}
#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,17 @@ svn_fs_fs__create(svn_fs_t *fs,
pool));
}
+ /* Issue #3437: Create a rep-cache.db if rep-caching is supported,
+ * and set its access permissions in accordance with the umask.
+ * Sqlite uses mode 644 in most situations, and ignores the umask.
+ *
+ * We do this at FS creation time since it would imply an extra stat
+ * performance hit during regular operation. There is no way of knowing
+ * if rep-cache.db was newly created after calling svn_sqlite__open().
+ */
+ 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 @@ open_rep_cache(void *baton,
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 @@ open_rep_cache(void *baton,
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 @@ get_default_file_perms(apr_fileperms_t *perms, apr
/* 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 @@ svn_io_open_unique_file3(apr_file_t **file,
* 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
