"Samuel GĂ©lineau" <gelisam@gmail.com> writes:
> I've found a bug in the subversion client, I have briefly discussed it
> on irc://irc.freenode.net/#svn, I have written a patch and a test
> case. Here is the story of my adventure.
>
> As you probably know, subversion doesn't cope well with files that are
> both "svn:special" and "svn:executable". Making sure this never
> happens is a one line script:
> find -type l | xargs svn propdel svn:executable
>
> However, after doing this, some of my directories were acting weird
> because their executable permission bit was turned off. Now, normally
> "svn propdel svn:executable" only calls "chmod -x" on files, never on
> directories. However, I was now using propdel on _symlinked_
> directories, and this fooled propdel into calling "chmod -x" on them
> anyway.
>
> I didn't want that to happen, so I wrote this patch. Enjoy.
>
> [[
> Prevent "svn propdel svn:executable symlink-to-dir" from doing "chmod
> -x" on the directory
> * subversion-1.4.5/subversion/libsvn_wc/props.c
> (svn_wc_prop_set2): don't drop permissions if the target has svn:special
> ]]
Thanks for the patch!
I think we may need a slightly different solution. Below, I'll
comment on your patch, then propose a different way.
> --- subversion/libsvn_wc/props.c 2006-07-03 04:51:44.000000000 -0400
> +++ subversion/libsvn_wc/props.c 2007-09-11 23:45:41.884118854 -0400
> @@ -1515,6 +1515,13 @@
> SVN_ERR(svn_wc_adm_retrieve(&adm_access, adm_access,
> svn_path_dirname(path, pool), pool));
>
> + err = svn_wc__load_props(&base_prophash, &prophash, adm_access, base_name,
> + pool);
> + if (err)
> + return
> + svn_error_quick_wrap
> + (err, _("Failed to load properties from disk"));
> +
By the way, today's trunk code has the SVN_ERR_W() macro for this.
> /* Setting an inappropriate property is not allowed (unless
> overridden by 'skip_checks', in some circumstances). Deleting an
> inappropriate property is allowed, however, since older clients
> @@ -1575,7 +1582,10 @@
> in), then chmod -x. */
> if (value == NULL)
> {
> - SVN_ERR(svn_io_set_file_executable(path, FALSE, TRUE, pool));
> + if (!svn_wc__has_special_property(prophash))
> + {
> + SVN_ERR(svn_io_set_file_executable(path, FALSE, TRUE, pool));
> + }
> }
Two problems here:
First, "svn:special" doesn't necessarily mean symlink. Symlink is the
only form of "special" today, but that might not be the case tomorrow.
Second, if the target of the symlink is a file, not a directory, then
do we want to set the executable bit or not? I'm not sure.
However, I thought a better way to address this problem would be to
change svn_io_set_file_executable(), rather than changing its callers.
That function is very short:
svn_error_t *
svn_io_set_file_executable(const char *path,
svn_boolean_t executable,
svn_boolean_t ignore_enoent,
apr_pool_t *pool)
{
/* On Windows, just exit -- on unix call our internal function
which attempts to honor the umask. */
#ifndef WIN32
return io_set_file_perms(path, FALSE, FALSE, TRUE, executable,
ignore_enoent, pool);
#else
return SVN_NO_ERROR;
#endif
}
All the work happens in io_set_file_perms(). But in that function,
when we call apr_stat() we don't set the APR_FINFO_LINK bit in the
'wanted' mask(), so we don't even know if we're dealing with a link or
not. In other words, today we have this...
apr_stat(&finfo, path_apr, APR_FINFO_PROT, pool);
...instead of this:
apr_stat(&finfo, path_apr, APR_FINFO_LINK | APR_FINFO_PROT, pool);
If you print out finfo afterwards, you can see the result:
(gdb) p finfo
$4 = { pool = 0x808dc70,
valid = 7598448,
protection = 1877,
filetype = APR_DIR,
user = 1000,
group = 1000,
inode = 9896226,
device = 2049,
nlink = 3,
size = 4096,
csize = -5191814459127623448,
atime = 1190056538000000,
mtime = 1190056539000000,
ctime = 1190056539000000,
fname = 0x808b350 "wc/symlink",
name = 0xb7f11640 "ö\027",
filehand = 0xb7f8c308 }
The filetype is APR_DIR, but the name is "wc/symlink".
I'll post a separate mail about how we can/should change
io_set_file_perms() to protect against this situation. The subject
line will be "Setting x-bit on symlinks to dirs vs files."
-Karl
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Received on Mon Sep 17 21:38:20 2007