Hi Bert,
Bert Huijben writes:
> > -----Original Message-----
> > From: Ramkumar Ramachandra [mailto:artag...@gmail.com]
> >
> > Index: subversion/libsvn_subr/io.c
> > ===================================================================
> > --- subversion/libsvn_subr/io.c (revision 1005706)
> > +++ subversion/libsvn_subr/io.c (working copy)
> > @@ -1269,7 +1269,8 @@ static svn_error_t *
> > get_default_file_perms(apr_fileperms_t *perms, apr_pool_t
> > *scratch_pool)
> > {
> > /* the default permissions as read from the temp folder */
> > - static apr_fileperms_t default_perms = 0;
> > + static apr_fileperms_t default_perms;
> > + apr_atomic_set32(&default_perms, 0);
>
> This shouldn't change. This = 0 is applied at application/library load (just
> once). After your change it happens at every invocation.
Ok.
> >
> > /* Technically, this "racy": Multiple threads may use enter here and
> > try to figure out the default permisission concurrently. That's
> > fine
> > @@ -1303,7 +1304,7 @@ get_default_file_perms(apr_fileperms_t *perms,
> > apr
> > SVN_ERR(svn_io_file_close(fd, scratch_pool));
> >
> > *perms = finfo.protection;
> > - default_perms = finfo.protection;
> > + apr_atomic_set32(&default_perms, finfo.protection);
>
> Yes, assuming default_perms is a 32 bit integer (which I think it always is)
> this makes it safe on all platforms. Without this it relies on cpu
> architecture, alignment and possibly caching hints.
> (As noted by others, without this patch it should be safe on x86 and x64
> architectures (where compilers handle the proper alignment for the static
> variable). But we can't be sure about the other architectures)
Right, got it. Can I get a +1 for the following patch?
[[[
* subversion/libsvn_subr/io.c
(get_default_file_perms): The 32-bit integer `default_perms` is only
really atomic (and hence thread-safe) on x86 and x64 where compilers
handle the proper alignment for static variables. Handle it in the
more general case using `apr_atomic_set32`.
Suggested by: Bert
]]]
Index: subversion/libsvn_subr/io.c
===================================================================
--- subversion/libsvn_subr/io.c (revision 1005706)
+++ subversion/libsvn_subr/io.c (working copy)
@@ -1303,7 +1303,7 @@ get_default_file_perms(apr_fileperms_t *perms, apr
SVN_ERR(svn_io_file_close(fd, scratch_pool));
*perms = finfo.protection;
- default_perms = finfo.protection;
+ apr_atomic_set32(&default_perms, finfo.protection);
}
else
*perms = default_perms;
