You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@subversion.apache.org by Edmund Horner <ej...@gmail.com> on 2006/02/25 13:56:20 UTC
[PATCH] 'svnlook diff' uses item name as temp file name
Hi all.
This is a little issue I came across while reusing 'svnlook diff' code
for a related project (an indexing hook script).
prepare_tmpfiles in svnlook/main.c will use the item's path in the
repository as part of the temporary file name it uses for diffing files.
This can result in attempted temporary file names that are valid in
the repository but don't work in the system's temp dir. For example
(Windows):
$ svnlook diff -r 1399 d:\svn\svn-depot
svnlook: Can't open file
'C:\DOCUME~1\Edmund\LOCALS~1\Temp\svnlook.1\project\svn\trunk\packages\
freebsd\subversion\files\patch-subversion::libsvn_ra_dav::session.c':
The filename, directory name, or volume label syntax is incorrect.
(My r1399 corresponds to r1398 on svn.collab.net.)
The easiest solution that I can see is to use plain old temporary file
names instead of full paths. Max Bowsher suggested I write a patch to
that effect (attached).
However I've noticed that building out the full diff tree is intentional
in svnlook (print_diff_tree). My patch seems to work (it gives
identical diffs for those revisions I've tried that don't contain
unusual paths), but I'm not 100% certain of the reason for doing it this
way...
FWIW, the temporary files are created, used, and deleted before the rest
of the tree is processed; and the only functions that use the names of
the temporary files are (all in print_diff_tree):
* prepare_tmpfiles (which creates them using tmpdir)
* svn_diff_file_diff
* svn_diff_file_output_unified2
* svn_io_remove_file (which gets rid of them)
tmpdir is $(TEMP)/svnlook.n (with n >= 1), which is created by
create_unique_tmpdir.
The best case scenario I can see is that svnlook diff doesn't need to
build the whole tree any more, and we can simplify things (and possibly
remove create_unique_tmpdir). Max noted that create_unique_tmpdir makes
an effort to set appropriate file permissions on tmpdir.
However, if it does need to build the whole tree then some other method
will be needed to make temporary file names that are compatible with the
OS but still fit into the diffing mechanism. *shrug*
This patch simply stops building the tree. I have also updated some
comments in print_diff_tree to reflect that. The use of tmpdir is retained.
Let me know if you want anything else in the way of a better patch, etc.
Edmund.
[[[
Use simple and safe temporary file names for svnlook diff, rather than
building out the tree as it exists in the repository -- this avoids
problems with paths that can't be used in the temporary directory.
Patch by: Edmund Horner <ej...@gmail.com>
Suggested by: Max Bowsher <ma...@ukf.net>
* subversion/svnlook/main.c
(prepare_tmpfiles): Create a normal unique file in the temporary
directory.
(print_diff_tree): Update comment to omit mention of the built-out
temporary directories, which don't get created any more.
]]]
Re: [PATCH] 'svnlook diff' uses item name as temp file name (second patch
version)
Posted by Peter Lundblad <pe...@famlundblad.se>.
Edmund Horner writes:
> Ok, here's a new patch...
Committed in r18650.
Thanks for the patch,
//Peter
> I have removed create_unique_tmpdir but kept
> the tmpdir argument to print_diff_tree; it seems more efficient to call
> svn_io_temp_dir just once. The difference is that tmpdir is now simply
> the official temp dir instead of a special unique svnlook temp dir.
>
> The comments in print_diff_tree have been trimmed down a bit more. The
> code continues to create the "second" temp file first (which it did in
> case of name conflicts), which isn't necessary now.
>
> I've tested it on the first 10000 svn revisions (yeah, my machine is
> slow) and it produces identical diffs in all cases that the original can
> handle and diffs for those cases the original can't (1398, 1399, 1795,
> 1805). It also passes svnlook-tests.
>
> Edmund.
>
>
>
> [[[
> Use simple and safe temporary file names for svnlook diff, rather than
> building out the tree as it exists in the repository -- this avoids
> problems with paths that can't be used in the temporary directory.
>
> Patch by: Edmund Horner <ej...@gmail.com>
> Suggested by: Max Bowsher <ma...@ukf.net>
> Review by: Peter N. Lundblad <pe...@famlundblad.se>
>
> * subversion/svnlook/main.c
> (prepare_tmpfiles): Create a normal unique file in the temporary
> directory.
> (print_diff_tree): Update comments to omit mention of the built-out
> temporary directories, which don't get created any more.
> (create_unique_tmpdir): Remove function.
> (do_diff): Use the system temporary directory instead of creating
> a special svnlook one; unlike the old one, this temporary directory
> doesn't need to be removed after use, so don't try to delete it.
> ]]]
> Index: main.c
> ===================================================================
> --- main.c (revision 18619)
> +++ main.c (working copy)
> @@ -692,10 +692,9 @@
> }
>
> /* Now, prepare the two temporary files, each of which will either
> - be empty, or will have real contents. The first file we will
> - make in our temporary directory. */
> - *tmpfile2 = svn_path_join(tmpdir, path2, pool);
> - SVN_ERR(open_writable_binary_file(&fh, *tmpfile2, pool));
> + be empty, or will have real contents. */
> + SVN_ERR(svn_io_open_unique_file2(&fh, tmpfile2, apr_psprintf(pool, "%s/diff", tmpdir),
> + ".tmp", svn_io_file_del_none, pool));
> if (root2)
> SVN_ERR(dump_contents(fh, root2, path2, pool));
> apr_file_close(fh);
> @@ -897,24 +896,15 @@
> diff.
>
> - Second, dump the contents of the new version of the file
> - into the svnlook temporary directory, building out the
> - actual directories that need to be created in order to
> - fully represent the filesystem path inside the tmp
> - directory.
> + into the temporary directory.
>
> - Then, dump the contents of the old version of the file into
> - the svnlook temporary directory, also building out intermediate
> - directories as needed, using a unique temporary file name (we
> - do this *after* putting the new version of the file
> - there in case something actually versioned has a name
> - that looks like one of our unique identifiers).
> + the temporary directory.
>
> - Next, we run 'diff', passing the repository paths as the
> labels.
>
> - - Finally, we delete the temporary files (but leave the
> - built-out directories in place until after all diff
> - handling has been finished). */
> + - Finally, we delete the temporary files. */
> if (node->action == 'R' && node->text_mod)
> {
> do_diff = TRUE;
> @@ -1347,48 +1337,7 @@
> return SVN_NO_ERROR;
> }
>
> -/* Create a new temporary directory with an 'svnlook' prefix. */
> -static svn_error_t *
> -create_unique_tmpdir(const char **name, apr_pool_t *pool)
> -{
> - const char *unique_name;
> - const char *sys_tmp_dir;
> - const char *base;
> - unsigned int i;
>
> - SVN_ERR(svn_io_temp_dir(&sys_tmp_dir, pool));
> - base = svn_path_join(sys_tmp_dir, "svnlook", pool);
> -
> - for (i = 1; i <= 99999; i++)
> - {
> - svn_error_t *err;
> -
> - unique_name = apr_psprintf(pool, "%s.%u", base, i);
> - /* The directory has a predictable name so it is made writeable for
> - the owner only (without relying on the umask) to inhibit symlink
> - attacks on the filenames; the filenames are also, to a certain
> - extent, predictable. */
> - err = svn_io_dir_make(unique_name,
> - APR_UREAD | APR_UWRITE | APR_UEXECUTE,
> - pool);
> -
> - if (!err)
> - {
> - *name = unique_name;
> - return SVN_NO_ERROR;
> - }
> -
> - if (! APR_STATUS_IS_EEXIST(err->apr_err))
> - return err;
> -
> - svn_error_clear(err);
> - }
> -
> - *name = NULL;
> - return svn_error_createf(SVN_ERR_IO_UNIQUE_NAMES_EXHAUSTED,
> - NULL, _("Can't create temporary directory"));
> -}
> -
> /* Print some diff-y stuff in a TBD way. :-) */
> static svn_error_t *
> do_diff(svnlook_ctxt_t *c, apr_pool_t *pool)
> @@ -1414,18 +1363,12 @@
> if (tree)
> {
> const char *tmpdir;
> - svn_error_t *err;
>
> SVN_ERR(svn_fs_revision_root(&base_root, c->fs, base_rev_id, pool));
> - SVN_ERR(create_unique_tmpdir(&tmpdir, pool));
> - err = print_diff_tree(root, base_root, tree, "", "",
> - c, tmpdir, pool);
> - if (err)
> - {
> - svn_error_clear(svn_io_remove_dir(tmpdir, pool));
> - return err;
> - }
> - SVN_ERR(svn_io_remove_dir(tmpdir, pool));
> + SVN_ERR(svn_io_temp_dir(&tmpdir, pool));
> +
> + SVN_ERR(print_diff_tree(root, base_root, tree, "", "",
> + c, tmpdir, pool));
> }
> return SVN_NO_ERROR;
> }
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
[PATCH] 'svnlook diff' uses item name as temp file name (second patch
version)
Posted by Edmund Horner <ej...@gmail.com>.
Edmund Horner wrote:
> Peter N. Lundblad wrote:
>
>> I think we can get rid of the temporary dir completely. Setting the
>> permissions was done to avoid a security hole (symlink attack), which
>> isn't necessary when we use the system temporary directory directly.
>>
>> Otherwise, the patch looks good at a quick glance.
>
> Ok, I'll rewrite it to get rid of tmpdir and create_unique_tmpdir,
> unless someone else has anything to say about it.
Ok, here's a new patch... I have removed create_unique_tmpdir but kept
the tmpdir argument to print_diff_tree; it seems more efficient to call
svn_io_temp_dir just once. The difference is that tmpdir is now simply
the official temp dir instead of a special unique svnlook temp dir.
The comments in print_diff_tree have been trimmed down a bit more. The
code continues to create the "second" temp file first (which it did in
case of name conflicts), which isn't necessary now.
I've tested it on the first 10000 svn revisions (yeah, my machine is
slow) and it produces identical diffs in all cases that the original can
handle and diffs for those cases the original can't (1398, 1399, 1795,
1805). It also passes svnlook-tests.
Edmund.
[[[
Use simple and safe temporary file names for svnlook diff, rather than
building out the tree as it exists in the repository -- this avoids
problems with paths that can't be used in the temporary directory.
Patch by: Edmund Horner <ej...@gmail.com>
Suggested by: Max Bowsher <ma...@ukf.net>
Review by: Peter N. Lundblad <pe...@famlundblad.se>
* subversion/svnlook/main.c
(prepare_tmpfiles): Create a normal unique file in the temporary
directory.
(print_diff_tree): Update comments to omit mention of the built-out
temporary directories, which don't get created any more.
(create_unique_tmpdir): Remove function.
(do_diff): Use the system temporary directory instead of creating
a special svnlook one; unlike the old one, this temporary directory
doesn't need to be removed after use, so don't try to delete it.
]]]
Re: [PATCH] 'svnlook diff' uses item name as temp file name
Posted by Edmund Horner <ej...@gmail.com>.
Peter N. Lundblad wrote:
> On Sun, 26 Feb 2006, Edmund Horner wrote:
>
>> The easiest solution that I can see is to use plain old temporary file
>> names instead of full paths. Max Bowsher suggested I write a patch to
>> that effect (attached).
>>
>> However I've noticed that building out the full diff tree is intentional
>> in svnlook (print_diff_tree). My patch seems to work (it gives
>> identical diffs for those revisions I've tried that don't contain
>> unusual paths), but I'm not 100% certain of the reason for doing it this
>> way...
>>
> The only reason I can think of for generating the full paths would be to
> get correct output when svnlook was using an external diff program (which
> it was long ago).
Ah yes that might be it. I guess there are no plans to allow an
external diff tool for svnlook in the near future?
> I think we can get rid of the temporary dir completely. Setting the
> permissions was done to avoid a security hole (symlink attack), which
> isn't necessary when we use the system temporary directory directly.
>
> Otherwise, the patch looks good at a quick glance.
Ok, I'll rewrite it to get rid of tmpdir and create_unique_tmpdir,
unless someone else has anything to say about it.
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: [PATCH] 'svnlook diff' uses item name as temp file name
Posted by "Peter N. Lundblad" <pe...@famlundblad.se>.
On Sun, 26 Feb 2006, Edmund Horner wrote:
> The easiest solution that I can see is to use plain old temporary file
> names instead of full paths. Max Bowsher suggested I write a patch to
> that effect (attached).
>
> However I've noticed that building out the full diff tree is intentional
> in svnlook (print_diff_tree). My patch seems to work (it gives
> identical diffs for those revisions I've tried that don't contain
> unusual paths), but I'm not 100% certain of the reason for doing it this
> way...
>
The only reason I can think of for generating the full paths would be to
get correct output when svnlook was using an external diff program (which
it was long ago).
> tmpdir is $(TEMP)/svnlook.n (with n >= 1), which is created by
> create_unique_tmpdir.
>
> The best case scenario I can see is that svnlook diff doesn't need to
> build the whole tree any more, and we can simplify things (and possibly
> remove create_unique_tmpdir). Max noted that create_unique_tmpdir makes
> an effort to set appropriate file permissions on tmpdir.
>
I think we can get rid of the temporary dir completely. Setting the
permissions was done to avoid a security hole (symlink attack), which
isn't necessary when we use the system temporary directory directly.
Otherwise, the patch looks good at a quick glance.
Thanks,
//Peter
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org