Hi,
There was a potentially confusing typo in the comments in my patch, so
I've attached a version with the typo fixed. Note that the code itself
was fine, the typo was just in the comments.
Cheers,
Cathy
On Wed, Nov 20, 2013 at 6:07 PM, Cathy Fitzpatrick <ca...@cathyjf.com> wrote:
> Hello,
>
> 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.
>
> Here is an example of the current behaviour (before my patch):
>
> [Cathy@localhost subversion]$ ls -la INSTALL
> -rw-rw-r--. 1 Cathy Cathy 63057 Nov 20 17:34 INSTALL
> [Cathy@localhost subversion]$ ./svn patch patch.diff
> U INSTALL
> [Cathy@localhost subversion]$ ls -la INSTALL
> -rw-------. 1 Cathy Cathy 63070 Nov 20 17:58 INSTALL
>
> Notice that applying the patch has changed the permissions of INSTALL.
>
> After my patch, we instead get the following:
>
> [Cathy@localhost subversion]$ ls -la INSTALL
> -rw-rw-r--. 1 Cathy Cathy 63057 Nov 20 17:59 INSTALL
> [Cathy@localhost subversion]$ ./svn patch patch.diff
> U INSTALL
> [Cathy@localhost subversion]$ ls -la INSTALL
> -rw-rw-r--. 1 Cathy Cathy 63070 Nov 20 17:59 INSTALL
>
> As expected, patching the file no longer alters its permissions.
>
>
> I have attached my patch to this email and it includes a log message.
>
> Regards,
>
> Cathy
[[[
svn patch: Don't change permissions of patched files
* subversion/libsvn_client/patch.c
(init_patch_target): Create temporary file for result of patch
under the working directory, rather than under the system
temporary directory. This is already how EOL translation
works (see `svn_subst_copy_and_translate4`) and has the
advantage that we can safely change permissions on the file
without potentially exposing the file to the entire system.
(install_patched_target): Make sure the final patched file has
the same permissions as the original file before patching.
Patch by: Cathy J. Fitzpatrick <ca...@cathyjf.com>
]]]
Index: subversion/libsvn_client/patch.c
===================================================================
--- subversion/libsvn_client/patch.c (revision 1543974)
+++ subversion/libsvn_client/patch.c (working copy)
@@ -1076,23 +1076,33 @@
* ### we should have kept the patch field in patch_target_t to be
* ### able to distinguish between 'what the patch says we should do'
* ### and 'what we can do with the given state of our WC'. */
if (patch->operation == svn_diff_op_added)
target->added = TRUE;
else if (patch->operation == svn_diff_op_deleted)
target->deleted = TRUE;
if (! target->is_symlink)
{
- /* Open a temporary file to write the patched result to. */
+ /* Open a temporary file to write the patched result to.
+ *
+ * Create the temporary file under the working directory (the same
+ * as EOL translation uses) so that it is safe to change its file
+ * permissions without exposing the file to the entire system, which
+ * could happen if the file were under the system temporary
+ * directory, rather than the working directory.
+ */
SVN_ERR(svn_io_open_unique_file3(&target->patched_file,
- &target->patched_path, NULL,
+ &target->patched_path,
+ svn_dirent_dirname(
+ wcroot_abspath,
+ scratch_pool),
remove_tempfiles ?
svn_io_file_del_on_pool_cleanup :
svn_io_file_del_none,
result_pool, scratch_pool));
/* Put the write callback in place. */
content->write = write_file;
content->write_baton = target->patched_file;
}
else
@@ -2564,20 +2574,27 @@
svn_boolean_t repair_eol;
/* Copy the patched file on top of the target file.
* Always expand keywords in the patched file, but repair EOL
* only if svn:eol-style dictates a particular style. */
repair_eol = (target->content->eol_style ==
svn_subst_eol_style_fixed ||
target->content->eol_style ==
svn_subst_eol_style_native);
+ /* Make sure the patched file has the same permissions as the
+ * original file. By design, `patched_path` will be under
+ * the working directory, so this does not introduce any
+ * security issues with temp files. */
+ SVN_ERR(svn_io_copy_perms(
+ target->local_abspath, target->patched_path, pool));
+
SVN_ERR(svn_subst_copy_and_translate4(
target->patched_path, target->local_abspath,
target->content->eol_str, repair_eol,
target->content->keywords,
TRUE /* expand */, FALSE /* special */,
ctx->cancel_func, ctx->cancel_baton, pool));
}
if (target->added || target->replaced)
{
