You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@subversion.apache.org by Samuel Gélineau <ge...@gmail.com> on 2007/09/12 04:48:34 UTC
[PATCH] don't "chmod -x" directories
Hi,
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
]]
– Samuel Gélineau
Re: Setting x-bit on symlinks to dirs vs files.
Posted by Philip Martin <ph...@codematters.co.uk>.
Karl Fogel <kf...@red-bean.com> writes:
> Philip Martin <ph...@codematters.co.uk> writes:
>>
>> I think that setting svn:executable on a symbolic link should have no
>> effect, perhaps even be an error, irrespective of whether the symbolic
>> link points at a file or directory. I think it is wrong for the
>> client to change the permission of a target outside the working copy,
>> if that is where the symbolic link points, and if it points inside the
>> working copy then we can require the user to set permissions on the
>> target not the link.
>
> Do you mean that setting the property should be a noop-or-error, or
> that the consequent attempt to tweak the x-bit should noop-or-error?
The client should not tweak the x-bit. The client could also
warn/error/ignore attempts to set the property but that's not so
important, and relying on the client enforcing rules about properties
is not good since earlier/other clients won't necessarily do it.
> I prefer the latter way, with no-op, partly because it's easier to
> implement, and partly because it might give saner behavior when a
> file's type changes.
>
> It could be implemented entirely in io_set_file_perms(). You'd be
> allowed to set/unset the property, it just won't have any effect on
> the x-bit on platforms that care about the x-bit, because the calls to
> change the x-bit will have no effect. As for saner behavior,
> consider:
>
> The x-bit behavior would be based on the working type, not the
> versioned type. In other words, in this sequence of commands
>
> $ echo "I'm a regular file." > newfile
> $ svn add newfile
> $ svn commit -m "Add new file."
> $ rm newfile
> $ ln -s oldfile newfile
> $ svn propverb svn:executable newfile
>
> the last command would set the property, but have no effect on the
> x-bit. And in this sequence of commands
>
> $ ln -s oldfile newfile
> $ svn add newfile
> $ svn commit -m "Add new file."
> $ rm newfile
> $ echo "Now I'm a regular file." > newfile
> $ svn propverb svn:executable newfile
At present, attempts to change a file type like generate obstructions
in the working copy, the user needs to manually set/clear svn:special
as well.
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: Setting x-bit on symlinks to dirs vs files.
Posted by Karl Fogel <kf...@red-bean.com>.
Philip Martin <ph...@codematters.co.uk> writes:
> Karl Fogel <kf...@red-bean.com> writes:
> I think the permissions of a symbolic link are completely ignored on a
> Unix system.
>
>> I guess I lean toward Solution #1 right now, but would like to hear
>> others' thoughts.
>
> I think that setting svn:executable on a symbolic link should have no
> effect, perhaps even be an error, irrespective of whether the symbolic
> link points at a file or directory. I think it is wrong for the
> client to change the permission of a target outside the working copy,
> if that is where the symbolic link points, and if it points inside the
> working copy then we can require the user to set permissions on the
> target not the link.
Do you mean that setting the property should be a noop-or-error, or
that the consequent attempt to tweak the x-bit should noop-or-error?
I prefer the latter way, with no-op, partly because it's easier to
implement, and partly because it might give saner behavior when a
file's type changes.
It could be implemented entirely in io_set_file_perms(). You'd be
allowed to set/unset the property, it just won't have any effect on
the x-bit on platforms that care about the x-bit, because the calls to
change the x-bit will have no effect. As for saner behavior,
consider:
The x-bit behavior would be based on the working type, not the
versioned type. In other words, in this sequence of commands
$ echo "I'm a regular file." > newfile
$ svn add newfile
$ svn commit -m "Add new file."
$ rm newfile
$ ln -s oldfile newfile
$ svn propverb svn:executable newfile
the last command would set the property, but have no effect on the
x-bit. And in this sequence of commands
$ ln -s oldfile newfile
$ svn add newfile
$ svn commit -m "Add new file."
$ rm newfile
$ echo "Now I'm a regular file." > newfile
$ svn propverb svn:executable newfile
the last command could affect the x-bit. That way, one could commit a
single revision that changes a file to a regular file and sets the
x-bit on it; yet one would be prevented from simultaneously changing
it to a symlink and tweaking the x-bit on the symlink.
I understand the attraction of trying to keep the svn:executable
property's presence inextricably linked to the x-bit, but I think that
would be tricky to implement in practice. (On Windows, an svn:special
file is just a file, whose contents indicate that it's a link. Would
we read and parse those contents, just to make sure we've got the
right kind of specialness?) Whereas if we just punt on the x-bit
where it won't do any good, and let people set the property however
they want, I think we still get consistent results.
-Karl
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: Setting x-bit on symlinks to dirs vs files.
Posted by Philip Martin <ph...@codematters.co.uk>.
Karl Fogel <kf...@red-bean.com> writes:
> As far as I know, the only way to affect the bits on the link
> itself is via the system call lchmod() (which is to chmod() as
> lstat() is to stat()).
I think the permissions of a symbolic link are completely ignored on a
Unix system.
> I guess I lean toward Solution #1 right now, but would like to hear
> others' thoughts.
I think that setting svn:executable on a symbolic link should have no
effect, perhaps even be an error, irrespective of whether the symbolic
link points at a file or directory. I think it is wrong for the
client to change the permission of a target outside the working copy,
if that is where the symbolic link points, and if it points inside the
working copy then we can require the user to set permissions on the
target not the link.
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: Setting x-bit on symlinks to dirs vs files.
Posted by Karl Fogel <kf...@red-bean.com>.
Malcolm Rowe <ma...@farside.org.uk> writes:
> On Mon, Sep 17, 2007 at 01:22:12PM -0700, Karl Fogel wrote:
>> Last week, Samuel Gélineau reported problems with:
>>
>> $ svn propdel svn:executable symlink-to-directory
>>
>> Deleting the svn:executable property causes 'chmod -x'. However, that
>> follows links, so it changes the x-bit on the link target -- meaning
>> it can remove the executable bit on the directory target of the
>> symlink, which usually isn't the desired result!
>
> Just by the way, if you're looking to fix this, there was also a very
> similar problem reported recently whereby setting svn:needs-lock on
> symlinks breaks when Subversion tries to remove the w-bit.
>
> See http://svn.haxx.se/dev/archive-2007-09/0130.shtml.
All fixed now, r26757 (resolving issue #2581).
-Karl
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: Setting x-bit on symlinks to dirs vs files.
Posted by Malcolm Rowe <ma...@farside.org.uk>.
Hi Karl,
On Mon, Sep 17, 2007 at 01:22:12PM -0700, Karl Fogel wrote:
> Last week, Samuel Gélineau reported problems with:
>
> $ svn propdel svn:executable symlink-to-directory
>
> Deleting the svn:executable property causes 'chmod -x'. However, that
> follows links, so it changes the x-bit on the link target -- meaning
> it can remove the executable bit on the directory target of the
> symlink, which usually isn't the desired result!
Just by the way, if you're looking to fix this, there was also a very
similar problem reported recently whereby setting svn:needs-lock on
symlinks breaks when Subversion tries to remove the w-bit.
See http://svn.haxx.se/dev/archive-2007-09/0130.shtml.
Regards,
Malcolm
Setting x-bit on symlinks to dirs vs files.
Posted by Karl Fogel <kf...@red-bean.com>.
Last week, Samuel Gélineau reported problems with:
$ svn propdel svn:executable symlink-to-directory
(See http://subversion.tigris.org/servlets/ReadMsg?list=dev&msgNo=130081.)
Deleting the svn:executable property causes 'chmod -x'. However, that
follows links, so it changes the x-bit on the link target -- meaning
it can remove the executable bit on the directory target of the
symlink, which usually isn't the desired result!
There are a couple of ways to solve the problem. This requires some
background; please bear with me.
In io_set_file_perms() -- the guts of svn_io_set_file_executable() --
we don't distinguish between links and their targets. The call to
apr_stat()
status = apr_stat(&finfo, path_apr, APR_FINFO_PROT, pool);
requests only APR_FINFO_PROT information; note how we don't also set
the APR_FINFO_LINK bit for the 'wanted' mask. Because we don't set
that bit, apr_stat() gets info about the target of the link rather
than about the link itself. Here's the relevant code from apr_stat():
if (wanted & APR_FINFO_LINK)
srv = lstat(fname, &info);
else
srv = stat(fname, &info);
And here's the result in the 'finfo' structure we get back:
(gdb) p finfo
$4 = { ...
filetype = APR_DIR,
...
fname = 0x808b350 "wc/symlink"
... }
Yes, that's right: the filetype is APR_DIR, the name is "wc/symlink" :-).
IOW, io_set_file_perms() doesn't even know if it's dealing with a link
or not when it starts banging on the permissions of 'path'... But that
doesn't matter so much, because apr_file_perms_set() also resolves
links and operates on their targets. Thus, the real issue isn't
io_set_file_perms() not knowing if 'path' is a link, but rather not
*caring* whether 'path' ultimately resolves to a file or directory
(even though that information is readily available).
I can think of two solutions, given the above. I'm not sure which is
more correct.
Simple Solution #1:
Make svn_io_set_file_executable() a no-op when invoked on a directory
(even via a link).
In other words, in io_set_file_perms(), if only 'change_executable'
is set and finfo.filetype == APR_DIR, then simply don't do
anything. We'd document that svn_io_set_file_executable() is a
no-op on non-files; change io_set_file_perms()'s name to
io_set_path_perms() (because all its *other* operations would still
work on non-files); and document the exception.
By the way, I don't think this would be an important enough API
change to warrant svn_io_set_file_executable2(), but we could rev
it if people are worried about that.
Not-So-Simple Solution #2:
Have io_set_file_perms() detect when its operand is a link, and
have it set the perms on the link instead of the link's target.
The thing is, I'm not sure that's even possible. The APR function
we use to tweak perms (or anything else about a file) is this:
APR_DECLARE(apr_status_t) apr_file_perms_set(const char *fname,
apr_fileperms_t perms)
{
mode_t mode = apr_unix_perms2mode(perms);
if (chmod(fname, mode) == -1)
return errno;
return APR_SUCCESS;
}
As you can see, it takes the path as a string argument. In other
words, it starts and ends at the symlink too! And it's just
passing off to the chmod(). (apr_unix_perms2mode() doesn't do
anything interesting, by the way -- that is, it doesn't affect any
bits that might mean "act on the link not its target".)
As far as I know, the only way to affect the bits on the link
itself is via the system call lchmod() (which is to chmod() as
lstat() is to stat()). However, APR doesn't even offer an
interface to lchmod():
$ find apr -type f | xargs grep lchmod
$ find apr-util -type f | xargs grep lchmod
$
So Solution #2 would be quite involved. And I'm not even sure it's
better than #1. In Subversion, when would we ever want to change
permissions on a link, as opposed to its target?
I guess I lean toward Solution #1 right now, but would like to hear
others' thoughts.
-Karl
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: [PATCH] don't "chmod -x" directories
Posted by Karl Fogel <kf...@red-bean.com>.
"Samuel Gélineau" <ge...@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