On Thu, Nov 21, 2013 at 11:19:34AM +0000, Philip Martin wrote:
> Cathy Fitzpatrick <cathy_at_cathyjf.com> writes:
>
> > Currently the `svn patch` command changes the permissions on any file
> > it patches to 600. This occurs because it creates a file under the
> > system temporary directory for applying the patch, and then copies
> > this file to the final destination. `apr_file_mktemp` sensibly assigns
> > mode 600 to files created under the system temporary directory to
> > avoid them being exposed to the entire system. So the result is that
> > any file that `svn patch` patches ends up with 600 permissions rather
> > than whatever it had before.
>
> Patch is no different from other commands like revert and update in this
> behaviour:
>
> $ ls -l wc/A/f
> -rw-r--r-- 1 pm pm 2 Nov 21 10:46 wc/A/f
>
> $ # change permissions with patch
> $ (umask 077 ; svn patch x.x wc)
> U wc/A/f
> $ ls -l wc/A/f
> -rw------- 1 pm pm 5 Nov 21 10:47 wc/A/f
>
> $ # change permissions with revert
> $ (umask 070 ; svn revert -R wc)
> Reverted 'wc/A/f'
> $ ls -l wc/A/f
> -rw----rw- 1 pm pm 2 Nov 21 10:50 wc/A/f
>
> $ # change permissions with update
> (umask 007 ; xsvn up wc)
> Updating 'wc':
> U wc/A/f
> Updated to revision 2.
> $ ls -l wc/A/f
> -rw-rw---- 1 pm pm 8 Nov 21 10:51 wc/A/f
>
> Is there some justification for changing patch so that it is different
> from the other commands? Should we change all commands?
I think the proposed patch is right.
However, I think we should start thinking about tweaking the svn_io_
APIs in a way that forces callers to consider what permissions
copy and rename destinations should have.
svn_subst_copy_and_translate4() actually does copy permission from
the source to the target. However, since the source is a temporary file
in /tmp with restrictive permissions, that doesn't really make sense.
So the io APIs don't really offer what 'svn patch' (and possibly
other commands) need.
If svn_subst_copy_and_translate() had an additional argument describing
file permissions to set on the destination, all callers would be forced
to consider this question. That would make more sense than the current
approach of calling svn_subst_copy_and_translate() and then also calling
copy_perms(). When writing future code, we'll keep forgetting to check
whether an additional copy_perms() call is needed.
I believe this problem affects more functions that rename or copy
things, not just svn_subst_copy_and_translate().
Cathy, would you be willing to invest a bit more time into this?
If so, can you help us by identifying which functions in include/svn_io.h
copy files and are setting implied permissions at the destination?
If you don't have time for this, I suggest we apply your patch, and
I'll file my above idea as an issue and get to it later.
Thank you for your contribution, in any case!
Once you have identified the functions, we can add arguments (creating
a new version of each function), and then over time go through all callers
of these functions and upgrade them to the new version. That should
fix such issues everywhere, and will prevent new instances of the
problem from occurring.
Received on 2013-11-21 12:52:59 CET