You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@subversion.apache.org by Philip Martin <ph...@codematters.co.uk> on 2005/02/11 18:33:51 UTC
Re: svn commit: r12968 - in branches/locking/subversion: include
libsvn_subr libsvn_wc
fitz@tigris.org writes:
> Author: fitz
> Date: Fri Feb 11 10:47:44 2005
> New Revision: 12968
> --- branches/locking/subversion/libsvn_subr/io.c (original)
> +++ branches/locking/subversion/libsvn_subr/io.c Fri Feb 11 10:47:44 2005
> @@ -1003,6 +1003,139 @@
> return SVN_NO_ERROR;
> }
>
> +/* Used by svn_io_set_file_read_write_carefully for caching this value. */
> +static apr_fileperms_t default_file_perms = 0;
I don't think that's right. An application could, quite legitimately,
change it's umask while running, and then the cached value would be
out-of-date.
> +
> +/* Create a temp file so that we can use the temp file's mask when
> + setting PATH (and any other file for the life of the process) to
> + read-write (on Unix). */
> +static svn_error_t *
> +cache_default_file_perms (const char *path, apr_pool_t *pool)
> +{
> + apr_status_t status;
> + apr_finfo_t finfo;
> + apr_file_t *fd;
> + const char *tmp_path;
> + const char *apr_path;
> +
> + SVN_ERR (svn_path_cstring_from_utf8 (&apr_path, path, pool));
> + SVN_ERR (svn_io_open_unique_file (&fd, &tmp_path, path,
> + "tmp", TRUE, pool));
> + status = apr_stat (&finfo, tmp_path, APR_FINFO_PROT, pool);
> + if (status)
> + return svn_error_wrap_apr (status, _("Can't get default file perms "
> + "for file at '%s' (file stat error"),
> + path);
> +
> + apr_file_close(fd);
> + if (status)
> + return svn_error_wrap_apr (status, _("Can't get default file perms for "
> + "file at '%s' (file close error)"),
> + path);
> +
> + default_file_perms = finfo.protection;
> + return SVN_NO_ERROR;
> +}
> +
> +svn_error_t *
> +svn_io_set_file_read_write_carefully (const char *path,
> + svn_boolean_t enable_write,
> + svn_boolean_t ignore_enoent,
> + apr_pool_t *pool)
> +{
> + apr_status_t status;
> + const char *path_apr;
> + apr_finfo_t finfo;
> + apr_fileperms_t perms_to_set;
> +
> + SVN_ERR (svn_path_cstring_from_utf8 (&path_apr, path, pool));
> +
> + /* Try to change only a minimal amount of the perms first
> + by getting the current perms and adding execute bits
> + only on where read perms are granted. If this fails
> + fall through to the svn_io_set_file* calls. */
> + status = apr_stat (&finfo, path_apr, APR_FINFO_PROT, pool);
> + if (status)
> + {
> + if (ignore_enoent && APR_STATUS_IS_ENOENT (status))
> + return SVN_NO_ERROR;
> + else if (status != APR_ENOTIMPL)
> + return svn_error_wrap_apr (status,
> + _("Can't change read-write perms of "
> + "file '%s'"),
> + svn_path_local_style (path, pool));
> + }
> + else
> + {
> + perms_to_set = finfo.protection;
> + if (enable_write) /* Make read-write. */
> + {
> + if (!default_file_perms)
> + SVN_ERR (cache_default_file_perms (path, pool));
> + perms_to_set = default_file_perms;
> + }
> + else /* Make read-only. */
> + {
> + if (finfo.protection & APR_UREAD)
> + perms_to_set &= ~APR_UWRITE;
> + if (finfo.protection & APR_GREAD)
> + perms_to_set &= ~APR_GWRITE;
> + if (finfo.protection & APR_WREAD)
> + perms_to_set &= ~APR_WWRITE;
> + }
> +
> + /* If we aren't changing anything then just return, this save
> + some system calls and helps with shared working copies */
> + if (perms_to_set == finfo.protection)
> + return SVN_NO_ERROR;
I don't think the cache can work, so this optimisation has to go.
> +
> + status = apr_file_perms_set (path_apr, perms_to_set);
> + if (status)
> + {
> + if (APR_STATUS_IS_EPERM (status))
> + {
> + /* We don't have permissions to change the
> + permissions! Try a move, copy, and delete
> + workaround to see if we can get the file owned by
> + us. If these succeed, try the permissions set
> + again.
> +
> + Note that we only attempt this in the
> + stat-available path. This assumes that the
> + move-copy workaround will only be helpful on
> + platforms that implement apr_stat. */
> + SVN_ERR (reown_file (path_apr, pool));
I think you should be able to use reown_file to determine the
default_file_perms without needing to create any more files.
> + status = apr_file_perms_set (path_apr, perms_to_set);
> + }
> +
> + if (status)
> + {
> + if (ignore_enoent && APR_STATUS_IS_ENOENT (status))
> + return SVN_NO_ERROR;
> + else if (status == APR_ENOTIMPL) /* on win32, for example. */
> + {
> + if (enable_write)
> + SVN_ERR (svn_io_set_file_read_write (path, ignore_enoent,
> + pool));
> +
> + else
> + SVN_ERR (svn_io_set_file_read_only (path, ignore_enoent,
> + pool));
> + }
> + else
> + return svn_error_wrap_apr
> + (status, _("Can't change read-write perms of file '%s'"),
> + svn_path_local_style (path, pool));
> + }
> + else
> + return SVN_NO_ERROR;
> + }
> + else
> + return SVN_NO_ERROR;
> + }
> + return SVN_NO_ERROR;
> +}
That function has a lot of code in common with set_file_executable,
although the latter is more cavalier about enabling permission
probably because it's not seen as such a security issue. I'd prefer
it if the two functions were consistent, which would also mean they
could share a common implementation. If that means making
set_file_executable less efficient but more secure then I don't see
that as a problem.
--
Philip Martin
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: svn commit: r12968 - in branches/locking/subversion: include
libsvn_subr libsvn_wc
Posted by Philip Martin <ph...@codematters.co.uk>.
"Brian W. Fitzpatrick" <fi...@collab.net> writes:
> I think that doing this check each time without the cache could be a
> real slowdown, so for now (based on some conversation with ghudson),
> I'm inclined to pull all of this default perms code and just chmod o+w
> the file outright.
I don't like the fact that we already do o+x, I like o+w even less.
> Or do you think it's worth the effort to do the
> open/stat/close once for each file?
I'd be happy caching the default permissions in the access batons. It
would mean that a process that changes umask while using an access
baton cannot guarantee that the umask will be respected, but that's an
acceptable limitation as far as I am concerned.
It might even be possible to populate such a cache on the fly when
libsvn_wc creates one of it's many temporary files. Obviously that
would mean having a fallback that creates a dummy file if the cache is
empty.
--
Philip Martin
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: svn commit: r12968 - in branches/locking/subversion: include libsvn_subr libsvn_wc
Posted by "Brian W. Fitzpatrick" <fi...@collab.net>.
On Feb 11, 2005, at 12:33 PM, Philip Martin wrote:
> fitz@tigris.org writes:
>
>> Author: fitz
>> Date: Fri Feb 11 10:47:44 2005
>> New Revision: 12968
>
>> --- branches/locking/subversion/libsvn_subr/io.c (original)
>> +++ branches/locking/subversion/libsvn_subr/io.c Fri Feb 11 10:47:44
>> 2005
>> @@ -1003,6 +1003,139 @@
>> return SVN_NO_ERROR;
>> }
>>
>> +/* Used by svn_io_set_file_read_write_carefully for caching this
>> value. */
>> +static apr_fileperms_t default_file_perms = 0;
>
> I don't think that's right. An application could, quite legitimately,
> change it's umask while running, and then the cached value would be
> out-of-date.
Oy. Right. Plus threading issues... ah well.
>> +
>> +/* Create a temp file so that we can use the temp file's mask when
>> + setting PATH (and any other file for the life of the process) to
>> + read-write (on Unix). */
>> +static svn_error_t *
>> +cache_default_file_perms (const char *path, apr_pool_t *pool)
>> +{
>> + apr_status_t status;
>> + apr_finfo_t finfo;
>> + apr_file_t *fd;
>> + const char *tmp_path;
>> + const char *apr_path;
>> +
>> + SVN_ERR (svn_path_cstring_from_utf8 (&apr_path, path, pool));
>> + SVN_ERR (svn_io_open_unique_file (&fd, &tmp_path, path,
>> + "tmp", TRUE, pool));
>> + status = apr_stat (&finfo, tmp_path, APR_FINFO_PROT, pool);
>> + if (status)
>> + return svn_error_wrap_apr (status, _("Can't get default file
>> perms "
>> + "for file at '%s' (file
>> stat error"),
>> + path);
>> +
>> + apr_file_close(fd);
>> + if (status)
>> + return svn_error_wrap_apr (status, _("Can't get default file
>> perms for "
>> + "file at '%s' (file close
>> error)"),
>> + path);
>> +
>> + default_file_perms = finfo.protection;
>> + return SVN_NO_ERROR;
>> +}
>> +
>> +svn_error_t *
>> +svn_io_set_file_read_write_carefully (const char *path,
>> + svn_boolean_t enable_write,
>> + svn_boolean_t ignore_enoent,
>> + apr_pool_t *pool)
>> +{
>> + apr_status_t status;
>> + const char *path_apr;
>> + apr_finfo_t finfo;
>> + apr_fileperms_t perms_to_set;
>> +
>> + SVN_ERR (svn_path_cstring_from_utf8 (&path_apr, path, pool));
>> +
>> + /* Try to change only a minimal amount of the perms first
>> + by getting the current perms and adding execute bits
>> + only on where read perms are granted. If this fails
>> + fall through to the svn_io_set_file* calls. */
>> + status = apr_stat (&finfo, path_apr, APR_FINFO_PROT, pool);
>> + if (status)
>> + {
>> + if (ignore_enoent && APR_STATUS_IS_ENOENT (status))
>> + return SVN_NO_ERROR;
>> + else if (status != APR_ENOTIMPL)
>> + return svn_error_wrap_apr (status,
>> + _("Can't change read-write perms
>> of "
>> + "file '%s'"),
>> + svn_path_local_style (path,
>> pool));
>> + }
>> + else
>> + {
>> + perms_to_set = finfo.protection;
>> + if (enable_write) /* Make read-write. */
>> + {
>> + if (!default_file_perms)
>> + SVN_ERR (cache_default_file_perms (path, pool));
>> + perms_to_set = default_file_perms;
See the three lines above here? That's the optimization that needs to
go...
>> + }
>> + else /* Make read-only. */
>> + {
>> + if (finfo.protection & APR_UREAD)
>> + perms_to_set &= ~APR_UWRITE;
>> + if (finfo.protection & APR_GREAD)
>> + perms_to_set &= ~APR_GWRITE;
>> + if (finfo.protection & APR_WREAD)
>> + perms_to_set &= ~APR_WWRITE;
>> + }
>> +
>> + /* If we aren't changing anything then just return, this save
>> + some system calls and helps with shared working copies */
>> + if (perms_to_set == finfo.protection)
>> + return SVN_NO_ERROR;
>
> I don't think the cache can work, so this optimisation has to go.
... not this. This optimization merely noops if the existing file's
perms are the same as the perms we're going to change it to.
>> +
>> + status = apr_file_perms_set (path_apr, perms_to_set);
>> + if (status)
>> + {
>> + if (APR_STATUS_IS_EPERM (status))
>> + {
>> + /* We don't have permissions to change the
>> + permissions! Try a move, copy, and delete
>> + workaround to see if we can get the file owned by
>> + us. If these succeed, try the permissions set
>> + again.
>> +
>> + Note that we only attempt this in the
>> + stat-available path. This assumes that the
>> + move-copy workaround will only be helpful on
>> + platforms that implement apr_stat. */
>> + SVN_ERR (reown_file (path_apr, pool));
>
> I think you should be able to use reown_file to determine the
> default_file_perms without needing to create any more files.
Maybe so, but it's too late at this point--we've already tried
apr_file_perms_set once.
>> + status = apr_file_perms_set (path_apr, perms_to_set);
>> + }
>> +
>> + if (status)
>> + {
>> + if (ignore_enoent && APR_STATUS_IS_ENOENT (status))
>> + return SVN_NO_ERROR;
>> + else if (status == APR_ENOTIMPL) /* on win32, for
>> example. */
>> + {
>> + if (enable_write)
>> + SVN_ERR (svn_io_set_file_read_write (path,
>> ignore_enoent,
>> + pool));
>> +
>> + else
>> + SVN_ERR (svn_io_set_file_read_only (path,
>> ignore_enoent,
>> + pool));
>> + }
>> + else
>> + return svn_error_wrap_apr
>> + (status, _("Can't change read-write perms of file
>> '%s'"),
>> + svn_path_local_style (path, pool));
>> + }
>> + else
>> + return SVN_NO_ERROR;
>> + }
>> + else
>> + return SVN_NO_ERROR;
>> + }
>> + return SVN_NO_ERROR;
>> +}
>
> That function has a lot of code in common with set_file_executable,
> although the latter is more cavalier about enabling permission
> probably because it's not seen as such a security issue. I'd prefer
> it if the two functions were consistent, which would also mean they
> could share a common implementation. If that means making
> set_file_executable less efficient but more secure then I don't see
> that as a problem.
I actually went down that path at first, but felt like we were winding
up with a huge Frankenfunction that was kind of ugly.
I think that doing this check each time without the cache could be a
real slowdown, so for now (based on some conversation with ghudson),
I'm inclined to pull all of this default perms code and just chmod o+w
the file outright. Or do you think it's worth the effort to do the
open/stat/close once for each file?
-Fitz
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org