Philip Martin <[email protected]> writes:
> Evgeny Kotkov <[email protected]> writes:
>
>> The reason why this error is propagated up the stack is that we only examine
>> the 'ignore_enoent' argument after the first apr_file_remove() call. This is
>> racy — if we get a EACCES during the first attempt to remove a file, and the
>> file is simultaneously removed from the disk, the next attempt to remove it
>> would fail with a ENOENT, even with 'ignore_enoent'. I think we should
>> fix this by suppressing ENOENTs from every apr_file_remove() call, not
>> just the first one.
>
> Sounds plausible.
Here is the patch for this part of the issue. Log message:
[[[
Prevent svn_io_remove_file2() from misbehaving in certain racy situations
with ignore_enoent = TRUE.
On Windows, this function might call apr_file_remove() more than once due
to a special handling of access denied errors. However, we only handle
ENOENTs during the first call. If we happen to receive a EACCES during
the first attempt to remove a file, and the file is gone at the moment we
try again, a caller will receive an ENOENT; this will happen regardless of
the ignore_enoent argument value. Given that the caller explicitly told
us that he/she is okay with the files not being present, returning an error
is just wrong.
* subversion/libsvn_subr/io.c
(svn_io_remove_file2): Respect ignore_enoent argument after every possible
attempt to remove a file, including retries under WIN32. While here,
make the conditional statements a bit more readable by splitting them.
Found by: Cory Riddell <cory{_AT_}codeware.com>
(Message-ID: <[email protected]> in the users mailing list,
http://svn.haxx.se/users/archive-2015-01/0082.shtml)
]]]
Any objections?
Regards,
Evgeny Kotkov
Index: subversion/libsvn_subr/io.c
===================================================================
--- subversion/libsvn_subr/io.c (revision 1653810)
+++ subversion/libsvn_subr/io.c (working copy)
@@ -2456,11 +2456,6 @@ svn_io_remove_file2(const char *path,
SVN_ERR(cstring_from_utf8(&path_apr, path, scratch_pool));
apr_err = apr_file_remove(path_apr, scratch_pool);
- if (!apr_err
- || (ignore_enoent
- && (APR_STATUS_IS_ENOENT(apr_err)
- || SVN__APR_STATUS_IS_ENOTDIR(apr_err))))
- return SVN_NO_ERROR;
#ifdef WIN32
/* If the target is read only NTFS reports EACCESS and FAT/FAT32
@@ -2495,11 +2490,20 @@ svn_io_remove_file2(const char *path,
}
#endif
- if (apr_err)
- return svn_error_wrap_apr(apr_err, _("Can't remove file '%s'"),
- svn_dirent_local_style(path, scratch_pool));
-
- return SVN_NO_ERROR;
+ if (!apr_err)
+ {
+ return SVN_NO_ERROR;
+ }
+ else if (ignore_enoent && (APR_STATUS_IS_ENOENT(apr_err)
+ || SVN__APR_STATUS_IS_ENOTDIR(apr_err)))
+ {
+ return SVN_NO_ERROR;
+ }
+ else
+ {
+ return svn_error_wrap_apr(apr_err, _("Can't remove file '%s'"),
+ svn_dirent_local_style(path, scratch_pool));
+ }
}
