On Thu, Nov 21, 2013 at 4:52 AM, Stefan Sperling <s...@elego.de> wrote: > On Thu, Nov 21, 2013 at 11:19:34AM +0000, Philip Martin wrote: >> Cathy Fitzpatrick <ca...@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
