You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@subversion.apache.org by ph...@apache.org on 2013/01/10 12:10:29 UTC
svn commit: r1431279 - /subversion/trunk/subversion/libsvn_wc/wc_db_util.c
Author: philip
Date: Thu Jan 10 11:10:28 2013
New Revision: 1431279
URL: http://svn.apache.org/viewvc?rev=1431279&view=rev
Log:
Ensure wc.db has the permissions allowed by umask.
* subversion/libsvn_wc/wc_db_util.c
(svn_wc__db_util_open_db): Ensure wc.db exists before it is opened by SQLite.
Modified:
subversion/trunk/subversion/libsvn_wc/wc_db_util.c
Modified: subversion/trunk/subversion/libsvn_wc/wc_db_util.c
URL: http://svn.apache.org/viewvc/subversion/trunk/subversion/libsvn_wc/wc_db_util.c?rev=1431279&r1=1431278&r2=1431279&view=diff
==============================================================================
--- subversion/trunk/subversion/libsvn_wc/wc_db_util.c (original)
+++ subversion/trunk/subversion/libsvn_wc/wc_db_util.c Thu Jan 10 11:10:28 2013
@@ -136,6 +136,22 @@ svn_wc__db_util_open_db(svn_sqlite__db_t
svn_dirent_local_style(sdb_abspath,
scratch_pool));
}
+#ifndef WIN32
+ else
+ {
+ apr_file_t *f;
+
+ /* A standard SQLite build creates a DB with mode 644 ^ !umask
+ which means the file doesn't have group/world write access
+ even when umask allows it. By ensuring the file exists before
+ SQLite gets involved we give it the permissions allowed by
+ umask. */
+ SVN_ERR(svn_io_file_open(&f, sdb_abspath,
+ (APR_READ | APR_WRITE | APR_CREATE),
+ APR_OS_DEFAULT, scratch_pool));
+ SVN_ERR(svn_io_file_close(f, scratch_pool));
+ }
+#endif
SVN_ERR(svn_sqlite__open(sdb, sdb_abspath, smode,
my_statements ? my_statements : statements,
Re: svn commit: r1431279 - /subversion/trunk/subversion/libsvn_wc/wc_db_util.c
Posted by Philip Martin <ph...@wandisco.com>.
Philip Martin <ph...@wandisco.com> writes:
> The WC layer has a clear distinction between create and open so fixing
> in the WC layer only adds an overhead to checkout, the other operations
> are unaffected. The other user is FSFS and it does not have this
> distinction so the overhead would apply to all operations. I admit the
> overhead is small.
We could probably add this distinction to FSFS by passing the create
flag only when writing.
Perhaps we should also set the default SQLite permissions to 666 when
building the amalgamation.
--
Certified & Supported Apache Subversion Downloads:
http://www.wandisco.com/subversion/download
Re: svn commit: r1431279 - /subversion/trunk/subversion/libsvn_wc/wc_db_util.c
Posted by Philip Martin <ph...@wandisco.com>.
Julian Foad <ju...@btopenworld.com> writes:
> Philip Martin wrote:
>
>> URL: http://svn.apache.org/viewvc?rev=1431279&view=rev
>
>> Log:
>> Ensure wc.db has the permissions allowed by umask.
>
> ... in order to be consistent with the other files we write,
> and to fix sharing a WC?
>
> If this bug was breaking sharing a WC, should we back-port this fix?
Probably.
> If we consider this to be a fix for the way SQLite behaves, do we want
> the same throughout Subversion? Move the fix into
> svn_sqlite__open()?
The WC layer has a clear distinction between create and open so fixing
in the WC layer only adds an overhead to checkout, the other operations
are unaffected. The other user is FSFS and it does not have this
distinction so the overhead would apply to all operations. I admit the
overhead is small.
--
Certified & Supported Apache Subversion Downloads:
http://www.wandisco.com/subversion/download
RE: svn commit: r1431279 - /subversion/trunk/subversion/libsvn_wc/wc_db_util.c
Posted by Bert Huijben <be...@qqmail.nl>.
> -----Original Message-----
> From: Branko Čibej [mailto:brane@wandisco.com]
> Sent: donderdag 10 januari 2013 14:33
> To: dev@subversion.apache.org
> Subject: Re: svn commit: r1431279 -
> /subversion/trunk/subversion/libsvn_wc/wc_db_util.c
>
> On 10.01.2013 14:23, Julian Foad wrote:
> > If we consider this to be a fix for the way SQLite behaves, do we want the
> same throughout Subversion? Move the fix into svn_sqlite__open()?
>
> Only when creating a new database.
>
> > File a bug report with SQLite project?
>
> SQLite has a bug related to permissions in that is sets the process
> umask to 0 when creating log files, in order to make them match the
> permissions of the main database file. This is a very bad idea in
> multi-threaded environments. So, we should report at least that bug.
Philip already reported this and the fix was applied to sqlite trunk.
(Including several testcases to make sure there won't be regressions)
Bert
Re: svn commit: r1431279 - /subversion/trunk/subversion/libsvn_wc/wc_db_util.c
Posted by Branko Čibej <br...@wandisco.com>.
On 10.01.2013 14:23, Julian Foad wrote:
> If we consider this to be a fix for the way SQLite behaves, do we want the same throughout Subversion? Move the fix into svn_sqlite__open()?
Only when creating a new database.
> File a bug report with SQLite project?
SQLite has a bug related to permissions in that is sets the process
umask to 0 when creating log files, in order to make them match the
permissions of the main database file. This is a very bad idea in
multi-threaded environments. So, we should report at least that bug.
-- Brane
--
Branko Čibej
Director of Subversion | WANdisco | www.wandisco.com
Re: svn commit: r1431279 - /subversion/trunk/subversion/libsvn_wc/wc_db_util.c
Posted by Julian Foad <ju...@btopenworld.com>.
Philip Martin wrote:
> URL: http://svn.apache.org/viewvc?rev=1431279&view=rev
> Log:
> Ensure wc.db has the permissions allowed by umask.
... in order to be consistent with the other files we write,
and to fix sharing a WC?
If this bug was breaking sharing a WC, should we back-port this fix?
> * subversion/libsvn_wc/wc_db_util.c
> (svn_wc__db_util_open_db): Ensure wc.db exists before it is opened by SQLite.
> +#ifndef WIN32
> + else
> + {
> + apr_file_t *f;
> +
> + /* A standard SQLite build creates a DB with mode 644 ^ !umask
Should the expression say "0644 & ~umask", using C syntax?
> + which means the file doesn't have group/world write access
> + even when umask allows it. By ensuring the file exists before
> + SQLite gets involved we give it the permissions allowed by
> + umask. */
It would be good to have a hint in the comment about *why*, even if just "for consistency".
> + SVN_ERR(svn_io_file_open(&f, sdb_abspath,
> + (APR_READ | APR_WRITE | APR_CREATE),
> + APR_OS_DEFAULT, scratch_pool));
> + SVN_ERR(svn_io_file_close(f, scratch_pool));
> + }
> +#endif
>
> SVN_ERR(svn_sqlite__open(sdb, sdb_abspath, smode,
> my_statements ? my_statements : statements,
If we consider this to be a fix for the way SQLite behaves, do we want the same throughout Subversion? Move the fix into svn_sqlite__open()? File a bug report with SQLite project?
- Julian