[svn.haxx.se] · SVN Dev · SVN Users · SVN Org · TSVN Dev · TSVN Users · Subclipse Dev · Subclipse Users · this month's index

Re: svn commit: r12968 - in branches/locking/subversion: include libsvn_subr libsvn_wc

From: Brian W. Fitzpatrick <fitz_at_collab.net>
Date: 2005-02-11 20:57:09 CET

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
Received on Fri Feb 11 20:58:42 2005

This is an archived mail posted to the Subversion Dev mailing list.