Here's a version of the patch with those fixes in place.
Thanks,
Augie
[[[
Fix issue #2816: Interactive Merge Callback Handles Paths Safely
* subversion/svn/util.c
(svn_cl__edit_file_externally) Change to the directory containing the
temporary file and edit from there, then change back to the user's
working
directory.
]]]
Index: subversion/svn/util.c
===================================================================
--- subversion/svn/util.c (revision 25820)
+++ subversion/svn/util.c (working copy)
@@ -149,14 +149,40 @@
apr_hash_t *config,
apr_pool_t *pool)
{
- const char *editor, *cmd;
+ const char *editor, *cmd, *base_dir, *file_name, *base_dir_apr;
+ char *old_cwd;
int sys_err;
+ apr_status_t apr_err;
+
+ svn_path_split(path, &base_dir, &file_name, pool);
SVN_ERR(find_editor_binary(&editor, editor_cmd, config));
- cmd = apr_psprintf(pool, "%s %s", editor, path);
+ apr_err = apr_filepath_get(&old_cwd, APR_FILEPATH_NATIVE, pool);
+ if (apr_err)
+ return svn_error_wrap_apr(apr_err, _("Can't get working
directory"));
+
+ /* APR doesn't like "" directories */
+ if (base_dir[0] == '\0')
+ base_dir_apr = ".";
+ else
+ SVN_ERR(svn_path_cstring_from_utf8(&base_dir_apr, base_dir, pool));
+
+ apr_err = apr_filepath_set(base_dir_apr, pool);
+ if (apr_err)
+ return svn_error_wrap_apr
+ (apr_err, _("Can't change working directory to '%s'"), base_dir);
+
+ cmd = apr_psprintf(pool, "%s \'%s\'", editor, file_name);
sys_err = system(cmd);
- if (sys_err != 0)
+
+ apr_err = apr_filepath_set(old_cwd, pool);
+ if (apr_err)
+ svn_handle_error2(svn_error_wrap_apr
+ (apr_err, _("Can't restore working directory")),
+ stderr, TRUE /* fatal */, "svn: ");
+
+ if (sys_err)
/* Extracting any meaning from sys_err is platform specific, so
just
use the raw value. */
return svn_error_createf(SVN_ERR_EXTERNAL_PROGRAM, NULL,
On Jul 22, 2007, at 12:55 PM, Ben Collins-Sussman wrote:
> OK, before I apply this patch, about some stylistic nits? :-)
>
> {
> - const char *editor, *cmd;
> + const char *editor, *cmd, *base_dir, *file_name, *base_dir_apr;
> + char *old_cwd;
> int sys_err;
> + apr_status_t apr_err;
> + svn_error_t *err = SVN_NO_ERROR;
>
> Our convention, usually, is to end all functions with 'return
> SVN_NO_ERROR'.
>
>
> + base_dir = svn_path_dirname(path, pool);
> + file_name = svn_path_basename(path, pool);
>
> Couldn't you do this all at once with a single call to
> svn_path_split()?
>
> +
> SVN_ERR(find_editor_binary(&editor, editor_cmd, config));
>
> - cmd = apr_psprintf(pool, "%s %s", editor, path);
> + apr_err = apr_filepath_get(&old_cwd, APR_FILEPATH_NATIVE, pool);
> + if (apr_err)
> + return svn_error_wrap_apr(apr_err, _("Can't get working
> directory"));
> +
> + /* APR doesn't like "" directories */
> + if (base_dir[0] == '\0')
> + base_dir_apr = ".";
> + else
> + SVN_ERR(svn_path_cstring_from_utf8(&base_dir_apr, base_dir,
> pool));
> +
> + apr_err = apr_filepath_set(base_dir_apr, pool);
> + if (apr_err)
> + {
> + return svn_error_wrap_apr
> + (apr_err, _("Can't change working directory to '%s'"),
> base_dir);
> + }
>
> No need for curly braces around a single-line block.
>
> +
> + cmd = apr_psprintf(pool, "%s \'%s\'", editor, file_name);
> sys_err = system(cmd);
> if (sys_err != 0)
> - /* Extracting any meaning from sys_err is platform specific,
> so just
> - use the raw value. */
> - return svn_error_createf(SVN_ERR_EXTERNAL_PROGRAM, NULL,
> - _("system('%s') returned %d"), cmd,
> sys_err);
> + {
> + /* Extracting any meaning from sys_err is platform specific,
> so just
> + use the raw value. */
> + err = svn_error_createf(SVN_ERR_EXTERNAL_PROGRAM, NULL,
> + _("system('%s') returned %d"), cmd,
> sys_err);
> + }
>
>
> Same thing here.
>
>
> - return SVN_NO_ERROR;
> + apr_err = apr_filepath_set(old_cwd, pool);
> + if (apr_err)
> + {
> + svn_handle_error2(svn_error_wrap_apr
> + (apr_err, _("Can't restore working
> directory")),
> + stderr, TRUE /* fatal */, "svn: ");
> + }
> +
> + return err;
> }
>
> Same thing here.
>
> Actually, why do you define 'err' at the top of the function at all?
> I don't see it used.
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Received on Wed Jul 25 16:59:27 2007