Ramkumar Ramachandra wrote on Sat, Jan 08, 2011 at 16:06:01 +0530:
> Hi Daniel,
>
> Daniel Shahaf writes:
> > Ping, this doesn't seem to have been fixed yet?
>
> Thanks for the ping, and sorry about the delay. I think this should
> work -- I'll write a commit message if you think this is okay.
>
> Index: subversion/libsvn_subr/io.c
> ===================================================================
> --- subversion/libsvn_subr/io.c (revision 1056662)
> +++ subversion/libsvn_subr/io.c (working copy)
> @@ -1299,59 +1299,64 @@ reown_file(const char *path,
> return svn_error_return(svn_io_remove_file2(unique_name, FALSE, pool));
> }
>
> -/* Determine what the PERMS for a new file should be by looking at the
> - permissions of a temporary file that we create.
> - Unfortunately, umask() as defined in POSIX provides no thread-safe way
> - to get at the current value of the umask, so what we're doing here is
> - the only way we have to determine which combination of write bits
> - (User/Group/World) should be set by default.
> - Make temporary allocations in SCRATCH_POOL. */
> +/* the default permissions as read from the temp folder */
> +static volatile apr_fileperms_t default_perms = 0;
> +static volatile svn_atomic_t perms_init_state;
> +
> +/* Helper function to set default permissions. Passed to
> svn_atomic__init_once */
> static svn_error_t *
> -get_default_file_perms(apr_fileperms_t *perms, apr_pool_t *scratch_pool)
> +set_default_perms(void *baton, apr_pool_t *scratch_pool)
> {
> - /* the default permissions as read from the temp folder */
> - static apr_fileperms_t default_perms = 0;
> + apr_fileperms_t *default_perms = (apr_fileperms_t *) baton;
>
> - /* Technically, this "racy": Multiple threads may use enter here and
> - try to figure out the default permisission concurrently. That's fine
> - since they will end up with the same results. Even more technical,
> - apr_fileperms_t is an atomic type on 32+ bit machines.
> - */
> - if (default_perms == 0)
> - {
> - apr_finfo_t finfo;
> - apr_file_t *fd;
> + if (default_perms != 0)
This should dereference default_perms.
> + /* Nothing to do */
> + return SVN_NO_ERROR;
>
> - /* Get the perms for a newly created file to find out what bits
> - should be set.
> + apr_finfo_t finfo;
> + apr_file_t *fd;
>
Declarations after statement. C89 only allows declarations at the top
of a block, please fix.
> +/* Determine what the PERMS for a new file should be by looking at the
> + permissions of a temporary file that we create.
> + Unfortunately, umask() as defined in POSIX provides no thread-safe way
> + to get at the current value of the umask, so what we're doing here is
> + the only way we have to determine which combination of write bits
> + (User/Group/World) should be set by default.
> + Make temporary allocations in SCRATCH_POOL. */
> +static svn_error_t *
> +get_default_file_perms(apr_fileperms_t *perms, apr_pool_t *scratch_pool)
> +{
> + SVN_ERR(svn_atomic__init_once(&perms_init_state, set_default_perms,
> + default_perms, scratch_pool));
> + *perms = default_perms;
> + return SVN_NO_ERROR;
+1, this should fix the "DEFAULT_PERMS were being determined racily"
problem.
> +}
> +
> /* OR together permission bits of the file FD and the default permissions
> of a file as determined by get_default_file_perms(). Do temporary
> allocations in SCRATCH_POOL. */
If you send another patch that indents/dedents a block, could you please
attach a 'svn diff -x-w' version of the patch too? It would make
reviewing easier.
Thanks,
Daniel
