You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@subversion.apache.org by br...@apache.org on 2019/02/21 18:04:16 UTC
svn commit: r1854072 - in /subversion/trunk/subversion: libsvn_subr/io.c
tests/libsvn_subr/io-test.c
Author: brane
Date: Thu Feb 21 18:04:15 2019
New Revision: 1854072
URL: http://svn.apache.org/viewvc?rev=1854072&view=rev
Log:
Fix issue #4806: Remove on-disk trees with read-only directories in them.
* subversion/libsvn_subr/io.c
(io_set_perms): New; helper function for io_set_*_perms.
(io_set_file_perms): Use io_set_perms.
(io_set_dir_perms): New; like io_set_file_perms, but for directories.
(io_set_readonly_flag): New; helper function for setting the read-only flag.
(svn_io_set_file_read_only,
svn_io_set_file_read_write): Use io_set_readonly_flag.
(svn_io_remove_dir2): On Unix, make the parent directory writable before
trying to remove its children.
(svn_io_dir_remove_nonrecursive): On Windows, remove a directory's
read-only flag before trying to remove the directory.
Modified:
subversion/trunk/subversion/libsvn_subr/io.c
subversion/trunk/subversion/tests/libsvn_subr/io-test.c
Modified: subversion/trunk/subversion/libsvn_subr/io.c
URL: http://svn.apache.org/viewvc/subversion/trunk/subversion/libsvn_subr/io.c?rev=1854072&r1=1854071&r2=1854072&view=diff
==============================================================================
--- subversion/trunk/subversion/libsvn_subr/io.c (original)
+++ subversion/trunk/subversion/libsvn_subr/io.c Thu Feb 21 18:04:15 2019
@@ -1622,13 +1622,14 @@ merge_default_file_perms(apr_file_t *fd,
that attempts to honor the users umask when dealing with
permission changes. It is a no-op when invoked on a symlink. */
static svn_error_t *
-io_set_file_perms(const char *path,
- svn_boolean_t change_readwrite,
- svn_boolean_t enable_write,
- svn_boolean_t change_executable,
- svn_boolean_t executable,
- svn_boolean_t ignore_enoent,
- apr_pool_t *pool)
+io_set_perms(const char *path,
+ svn_boolean_t is_file,
+ svn_boolean_t change_readwrite,
+ svn_boolean_t enable_write,
+ svn_boolean_t change_executable,
+ svn_boolean_t executable,
+ svn_boolean_t ignore_enoent,
+ apr_pool_t *pool)
{
apr_status_t status;
const char *path_apr;
@@ -1648,9 +1649,16 @@ io_set_file_perms(const char *path,
|| SVN__APR_STATUS_IS_ENOTDIR(status)))
return SVN_NO_ERROR;
else if (status != APR_ENOTIMPL)
- return svn_error_wrap_apr(status,
- _("Can't change perms of file '%s'"),
- svn_dirent_local_style(path, pool));
+ {
+ if (is_file)
+ return svn_error_wrap_apr(status,
+ _("Can't change perms of file '%s'"),
+ svn_dirent_local_style(path, pool));
+ else
+ return svn_error_wrap_apr(status,
+ _("Can't change perms of directory '%s'"),
+ svn_dirent_local_style(path, pool));
+ }
return SVN_NO_ERROR;
}
@@ -1750,10 +1758,50 @@ io_set_file_perms(const char *path,
status = apr_file_attrs_set(path_apr, attrs, attrs_values, pool);
}
- return svn_error_wrap_apr(status,
- _("Can't change perms of file '%s'"),
- svn_dirent_local_style(path, pool));
+ if (is_file)
+ {
+ return svn_error_wrap_apr(status,
+ _("Can't change perms of file '%s'"),
+ svn_dirent_local_style(path, pool));
+ }
+ else
+ {
+ return svn_error_wrap_apr(status,
+ _("Can't change perms of directory '%s'"),
+ svn_dirent_local_style(path, pool));
+ }
}
+
+static svn_error_t *
+io_set_file_perms(const char *path,
+ svn_boolean_t change_readwrite,
+ svn_boolean_t enable_write,
+ svn_boolean_t change_executable,
+ svn_boolean_t executable,
+ svn_boolean_t ignore_enoent,
+ apr_pool_t *pool)
+{
+ return svn_error_trace(io_set_perms(path, TRUE,
+ change_readwrite, enable_write,
+ change_executable, executable,
+ ignore_enoent, pool));
+}
+
+static svn_error_t *
+io_set_dir_perms(const char *path,
+ svn_boolean_t change_readwrite,
+ svn_boolean_t enable_write,
+ svn_boolean_t change_executable,
+ svn_boolean_t executable,
+ svn_boolean_t ignore_enoent,
+ apr_pool_t *pool)
+{
+ return svn_error_trace(io_set_perms(path, FALSE,
+ change_readwrite, enable_write,
+ change_executable, executable,
+ ignore_enoent, pool));
+}
+
#endif /* !WIN32 && !__OS2__ */
#ifdef WIN32
@@ -2115,6 +2163,55 @@ svn_io_set_file_read_write_carefully(con
return svn_io_set_file_read_only(path, ignore_enoent, pool);
}
+#if defined(WIN32) || defined(__OS2__)
+/* Helper for svn_io_set_file_read_* */
+static svn_error_t *
+io_set_readonly_flag(const char *path_apr, /* file-system path */
+ const char *path, /* UTF-8 path */
+ svn_boolean_t set_flag,
+ svn_boolean_t is_file,
+ svn_boolean_t ignore_enoent,
+ svn_boolean_t pool)
+{
+ apr_status_t status;
+
+ status = apr_file_attrs_set(path_apr,
+ (set_flag ? APR_FILE_ATTR_READONLY : 0),
+ APR_FILE_ATTR_READONLY,
+ pool);
+
+ if (status && status != APR_ENOTIMPL)
+ if (!(ignore_enoent && (APR_STATUS_IS_ENOENT(status)
+ || SVN__APR_STATUS_IS_ENOTDIR(status))))
+ {
+ if (is_file)
+ {
+ if (set_flag)
+ return svn_error_wrap_apr(status,
+ _("Can't set file '%s' read-only"),
+ svn_dirent_local_style(path, pool));
+ else
+ return svn_error_wrap_apr(status,
+ _("Can't set file '%s' read-write"),
+ svn_dirent_local_style(path, pool));
+ }
+ else
+ {
+ if (set_flag)
+ return svn_error_wrap_apr(status,
+ _("Can't set directory '%s' read-only"),
+ svn_dirent_local_style(path, pool));
+ else
+ return svn_error_wrap_apr(status,
+ _("Can't set directory '%s' read-write"),
+ svn_dirent_local_style(path, pool));
+ }
+ }
+ return SVN_NO_ERROR;
+}
+#endif
+
+
svn_error_t *
svn_io_set_file_read_only(const char *path,
svn_boolean_t ignore_enoent,
@@ -2126,24 +2223,11 @@ svn_io_set_file_read_only(const char *pa
return io_set_file_perms(path, TRUE, FALSE, FALSE, FALSE,
ignore_enoent, pool);
#else
- apr_status_t status;
const char *path_apr;
SVN_ERR(cstring_from_utf8(&path_apr, path, pool));
-
- status = apr_file_attrs_set(path_apr,
- APR_FILE_ATTR_READONLY,
- APR_FILE_ATTR_READONLY,
- pool);
-
- if (status && status != APR_ENOTIMPL)
- if (!(ignore_enoent && (APR_STATUS_IS_ENOENT(status)
- || SVN__APR_STATUS_IS_ENOTDIR(status))))
- return svn_error_wrap_apr(status,
- _("Can't set file '%s' read-only"),
- svn_dirent_local_style(path, pool));
-
- return SVN_NO_ERROR;
+ return io_set_readonly_flag(path_apr, path,
+ TRUE, TRUE, ignore_enoent, pool);
#endif
}
@@ -2159,23 +2243,11 @@ svn_io_set_file_read_write(const char *p
return io_set_file_perms(path, TRUE, TRUE, FALSE, FALSE,
ignore_enoent, pool);
#else
- apr_status_t status;
const char *path_apr;
SVN_ERR(cstring_from_utf8(&path_apr, path, pool));
-
- status = apr_file_attrs_set(path_apr,
- 0,
- APR_FILE_ATTR_READONLY,
- pool);
-
- if (status && status != APR_ENOTIMPL)
- if (!ignore_enoent || !APR_STATUS_IS_ENOENT(status))
- return svn_error_wrap_apr(status,
- _("Can't set file '%s' read-write"),
- svn_dirent_local_style(path, pool));
-
- return SVN_NO_ERROR;
+ return io_set_readonly_flag(path_apr, path,
+ FALSE, TRUE, ignore_enoent, pool);
#endif
}
@@ -2761,6 +2833,12 @@ svn_io_remove_dir2(const char *path, svn
return svn_error_trace(err);
}
+ /* On Unix, nothing can be removed from a non-writable directory. */
+#if !defined(WIN32) && !defined(__OS2__)
+ SVN_ERR(io_set_dir_perms(path, TRUE, TRUE, FALSE, FALSE,
+ ignore_enoent, pool));
+#endif
+
for (hi = apr_hash_first(subpool, dirents); hi; hi = apr_hash_next(hi))
{
const char *name = apr_hash_this_key(hi);
@@ -4493,6 +4571,12 @@ svn_io_dir_remove_nonrecursive(const cha
SVN_ERR(cstring_from_utf8(&dirname_apr, dirname, pool));
+ /* On Windows, a read-only directory cannot be removed. */
+#if defined(WIN32) || defined(__OS2__)
+ SVN_ERR(io_set_readonly_flag(dirname_apr, dirname,
+ FALSE, FALSE, FALSE, pool));
+#endif
+
status = apr_dir_remove(dirname_apr, pool);
#ifdef WIN32
Modified: subversion/trunk/subversion/tests/libsvn_subr/io-test.c
URL: http://svn.apache.org/viewvc/subversion/trunk/subversion/tests/libsvn_subr/io-test.c?rev=1854072&r1=1854071&r2=1854072&view=diff
==============================================================================
--- subversion/trunk/subversion/tests/libsvn_subr/io-test.c (original)
+++ subversion/trunk/subversion/tests/libsvn_subr/io-test.c Thu Feb 21 18:04:15 2019
@@ -211,6 +211,38 @@ create_comparison_candidates(struct test
return err;
}
+/* Create an on-disk tree with optional read-only attributes on some
+ files and/or directories. */
+static svn_error_t *
+create_dir_tree(const char **dir_path,
+ const char *testname,
+ svn_boolean_t dir_readonly,
+ svn_boolean_t file_readonly,
+ apr_pool_t *pool)
+{
+ const char *test_dir_path;
+ const char *sub_dir_path;
+ const char *file_path;
+
+ SVN_ERR(svn_test_make_sandbox_dir(&test_dir_path, testname, pool));
+
+ sub_dir_path = svn_dirent_join(test_dir_path, "dir", pool);
+ SVN_ERR(svn_io_dir_make(sub_dir_path, APR_OS_DEFAULT, pool));
+
+ file_path = svn_dirent_join(sub_dir_path, "file", pool);
+ SVN_ERR(svn_io_file_create_empty(file_path, pool));
+
+ if (file_readonly)
+ SVN_ERR(svn_io_set_file_read_only(file_path, FALSE, pool));
+
+ if (dir_readonly)
+ SVN_ERR(svn_io_set_file_read_only(sub_dir_path, FALSE, pool));
+
+ *dir_path = sub_dir_path;
+ return SVN_NO_ERROR;
+}
+
+
/* Functions to check the 2-way and 3-way file comparison functions. */
@@ -1147,6 +1179,56 @@ test_apr_trunc_workaround(apr_pool_t *po
return SVN_NO_ERROR;
}
+
+/* Issue #4806 */
+static svn_error_t *
+test_rmtree_all_writable(apr_pool_t *pool)
+{
+ const char *dir_path = NULL;
+
+ SVN_ERR(create_dir_tree(&dir_path, "test_rmtree_all_writable",
+ FALSE, FALSE, pool));
+ SVN_ERR(svn_io_remove_dir2(dir_path, FALSE, NULL, NULL, pool));
+ return SVN_NO_ERROR;
+}
+
+/* Issue #4806 */
+static svn_error_t *
+test_rmtree_file_readonly(apr_pool_t *pool)
+{
+ const char *dir_path = NULL;
+
+ SVN_ERR(create_dir_tree(&dir_path, "test_rmtree_file_readonly",
+ FALSE, TRUE, pool));
+ SVN_ERR(svn_io_remove_dir2(dir_path, FALSE, NULL, NULL, pool));
+ return SVN_NO_ERROR;
+}
+
+/* Issue #4806 */
+static svn_error_t *
+test_rmtree_dir_readonly(apr_pool_t *pool)
+{
+ const char *dir_path = NULL;
+
+ SVN_ERR(create_dir_tree(&dir_path, "test_rmtree_dir_readonly",
+ TRUE, FALSE, pool));
+ SVN_ERR(svn_io_remove_dir2(dir_path, FALSE, NULL, NULL, pool));
+ return SVN_NO_ERROR;
+}
+
+/* Issue #4806 */
+static svn_error_t *
+test_rmtree_all_readonly(apr_pool_t *pool)
+{
+ const char *dir_path = NULL;
+
+ SVN_ERR(create_dir_tree(&dir_path, "test_rmtree_all_readonly",
+ TRUE, TRUE, pool));
+ SVN_ERR(svn_io_remove_dir2(dir_path, FALSE, NULL, NULL, pool));
+ return SVN_NO_ERROR;
+}
+
+
/* The test table. */
static int max_threads = 3;
@@ -1184,6 +1266,14 @@ static struct svn_test_descriptor_t test
"test svn_io_open_uniquely_named()"),
SVN_TEST_PASS2(test_apr_trunc_workaround,
"test workaround for APR in svn_io_file_trunc"),
+ SVN_TEST_PASS2(test_rmtree_all_writable,
+ "test svn_io_remove_dir2() with writable tree"),
+ SVN_TEST_PASS2(test_rmtree_file_readonly,
+ "test svn_io_remove_dir2() with read-only file"),
+ SVN_TEST_PASS2(test_rmtree_dir_readonly,
+ "test svn_io_remove_dir2() with read-only directory"),
+ SVN_TEST_PASS2(test_rmtree_all_readonly,
+ "test svn_io_remove_dir2() with read-only tree"),
SVN_TEST_NULL
};
Re: svn commit: r1854072 - in /subversion/trunk/subversion:
libsvn_subr/io.c tests/libsvn_subr/io-test.c
Posted by Branko Čibej <br...@apache.org>.
On 21.02.2019 23:37, Evgeny Kotkov wrote:
> Branko Čibej <br...@apache.org> writes:
>
>> @@ -4493,6 +4571,12 @@ svn_io_dir_remove_nonrecursive(const cha
>>
>> SVN_ERR(cstring_from_utf8(&dirname_apr, dirname, pool));
>>
>> + /* On Windows, a read-only directory cannot be removed. */
>> +#if defined(WIN32) || defined(__OS2__)
>> + SVN_ERR(io_set_readonly_flag(dirname_apr, dirname,
>> + FALSE, FALSE, FALSE, pool));
>> +#endif
>> +
>> status = apr_dir_remove(dirname_apr, pool);
> Would it be feasible for us to only attempt to remove the read-only
> attribute after receiving an error? (along the lines of the attached patch)
>
> The reason is that getting and setting file attributes usually results in
> CreateFile() syscalls, whereas opening files and the I/O-related syscalls are
> not cheap on Win32 in general. So changing the directory attributes per every
> remove could increase the amount of I/O operations and maybe slow down the
> cases where we have to remove multiple directories, with the post-commit
> transaction directory cleanup being an example of such case.
Committed, with tweaks, in r1854216.
-- Brane
Re: svn commit: r1854072 - in /subversion/trunk/subversion:
libsvn_subr/io.c tests/libsvn_subr/io-test.c
Posted by Erik Huelsmann <eh...@gmail.com>.
>
> By the way, I'm not sure why we carry around the "defined(__OS2__)"
> check in io.c. As far as I'm aware, no-one has ever actually tested
> Subversion on OS/2 ... these checks are probably just lifted out of APR,
> but don't do anything useful.
>
Maybe not tested, but there are supposedly floating OS/2 binaries around:
https://os2ports.smedley.id.au/index.php?page=subversion
Lacking an OS/2 installation, I have no idea if they actually work...
>
> -- Brane
>
>
Regards,
--
Bye,
Erik.
http://efficito.com -- Hosted accounting and ERP.
Robust and Flexible. No vendor lock-in.
Re: svn commit: r1854072 - in /subversion/trunk/subversion:
libsvn_subr/io.c tests/libsvn_subr/io-test.c
Posted by Branko Čibej <br...@apache.org>.
On 21.02.2019 23:37, Evgeny Kotkov wrote:
> Branko Čibej <br...@apache.org> writes:
>
>> @@ -4493,6 +4571,12 @@ svn_io_dir_remove_nonrecursive(const cha
>>
>> SVN_ERR(cstring_from_utf8(&dirname_apr, dirname, pool));
>>
>> + /* On Windows, a read-only directory cannot be removed. */
>> +#if defined(WIN32) || defined(__OS2__)
>> + SVN_ERR(io_set_readonly_flag(dirname_apr, dirname,
>> + FALSE, FALSE, FALSE, pool));
>> +#endif
>> +
>> status = apr_dir_remove(dirname_apr, pool);
> Would it be feasible for us to only attempt to remove the read-only
> attribute after receiving an error? (along the lines of the attached patch)
>
> The reason is that getting and setting file attributes usually results in
> CreateFile() syscalls, whereas opening files and the I/O-related syscalls are
> not cheap on Win32 in general. So changing the directory attributes per every
> remove could increase the amount of I/O operations and maybe slow down the
> cases where we have to remove multiple directories, with the post-commit
> transaction directory cleanup being an example of such case.
If your patch works, then just commit it; io-test.exe covers these cases
now. I agree it's better to try to remove the directory first (something
we can't do on Unix, where we need a writable directory in order to
delete its children).
Please update the backport proposals, too.
By the way, I'm not sure why we carry around the "defined(__OS2__)"
check in io.c. As far as I'm aware, no-one has ever actually tested
Subversion on OS/2 ... these checks are probably just lifted out of APR,
but don't do anything useful.
-- Brane
Re: svn commit: r1854072 - in /subversion/trunk/subversion:
libsvn_subr/io.c tests/libsvn_subr/io-test.c
Posted by Evgeny Kotkov <ev...@visualsvn.com>.
Branko Čibej <br...@apache.org> writes:
> @@ -4493,6 +4571,12 @@ svn_io_dir_remove_nonrecursive(const cha
>
> SVN_ERR(cstring_from_utf8(&dirname_apr, dirname, pool));
>
> + /* On Windows, a read-only directory cannot be removed. */
> +#if defined(WIN32) || defined(__OS2__)
> + SVN_ERR(io_set_readonly_flag(dirname_apr, dirname,
> + FALSE, FALSE, FALSE, pool));
> +#endif
> +
> status = apr_dir_remove(dirname_apr, pool);
Would it be feasible for us to only attempt to remove the read-only
attribute after receiving an error? (along the lines of the attached patch)
The reason is that getting and setting file attributes usually results in
CreateFile() syscalls, whereas opening files and the I/O-related syscalls are
not cheap on Win32 in general. So changing the directory attributes per every
remove could increase the amount of I/O operations and maybe slow down the
cases where we have to remove multiple directories, with the post-commit
transaction directory cleanup being an example of such case.
Thanks,
Evgeny Kotkov