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 <ejrh00@gmail.com>
Suggested by: Max Bowsher <maxb@ukf.net>
Review by: Peter N. Lundblad <peter@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
Received on Tue Feb 28 15:23:09 2006