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
Received on Fri Feb 11 19:35:28 2005