On Thu, Nov 21, 2013 at 4:52 AM, Stefan Sperling <stsp_at_elego.de> wrote:
> 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.
Hi Stefan,
I agree with you that it would make sense to review the use of the io
functions in a more comprehensive manner. I probably won't be able to
do this in the near future though. However, it would be good to fix
`svn patch` for the reasons I mentioned in my other email to Philip.
Thanks,
Cathy
Received on 2013-11-21 20:28:59 CET