I don't think the static function pointer should be declared volatile.
That would mean that for the normal use of the function pointer in the main
part of the program, compiler optimizations of memory address load
reordering would be disabled. It's only operations that require strict
memory ordering that should declare it volatile. That is why all the
apr_atomic functions "cast" values through a volatile pointer.
Second: why use apr_atomic_casptr? Writing a pointer to a memory address
is already atomic: another thread cannot see a halfway-changed pointer.
What the apr_atomic_casptr would prevent would it from being written twice
with the new address value. But why do we need to prevent that? I think
it's all right to have a race to write the new function pointer; it's all
right if it's written multiple times, since they are all writing the same
value.
Third: missing is the declaration of apr_winapi_##fn, referenced later in
the file, which should be:
static APR_INLINE rettype apr_winapi_##fn args \
{ return apr_winapi_##fn names; } /* call the function pointer */
On Sat, Dec 7, 2013 at 5:51 AM, Bert Huijben <bert_at_qqmail.nl> wrote:
>
>
> > -----Original Message-----
> > From: Bert Huijben [mailto:bert_at_qqmail.nl]
> > Sent: vrijdag 6 december 2013 19:14
> > To: 'William A. Rowe Jr.'; 'Stefan Fuhrmann'
> > Cc: 'APR Developer List'; 'Stefan Fuhrman'; 'Philip Martin'; 'Subversion
> > Development'
> > Subject: RE: Race condition in APR_DECLARE_LATE_DLL_FUNC()
> > implementation
> >
> >
> >
> > > -----Original Message-----
> > > From: William A. Rowe Jr. [mailto:wrowe_at_rowe-clan.net]
> > > Sent: vrijdag 6 december 2013 18:24
> > > To: Stefan Fuhrmann
> > > Cc: Bert Huijben; APR Developer List; Stefan Fuhrman; Philip Martin;
> > > Subversion Development
> > > Subject: Re: Race condition in APR_DECLARE_LATE_DLL_FUNC()
> > > implementation
> > >
> > > On Fri, 6 Dec 2013 16:44:52 +0100
> > > Stefan Fuhrmann <stefan.fuhrmann_at_wandisco.com> wrote:
> > >
> > > > On Fri, Dec 6, 2013 at 6:05 AM, William A. Rowe Jr.
> > > > <wrowe_at_rowe-clan.net>wrote:
> > > >
> > > > > On Thu, 5 Dec 2013 15:01:05 +0100
> > > > > "Bert Huijben" <bert_at_qqmail.nl> wrote:
> > > > >
> > > > > > I think the dll load function should be converted to a more
> stable
> > > > > > pattern, that properly handles multiple threads. And perhaps we
> > > > > > should just assume a few more NT functions to be alsways there
> > > > > > instead of loading them dynamically.
> > > > >
> > > > > This is possible with the 'mandatory' call to apr_init, but I think
> > > > > the existing pattern should persist for those who don't like to
> > > > > call the initialization logic.
> > > > >
> > > >
> > > > We currently call apr_initialize() before spawning threads or doing
> > > > anything other APR. What else do we need to become thread-safe
> > > > under Windows?
> > >
> > > Working on a patch. Simply, we just need an _initialize hack that
> > > triggers this lookup for each internally required (or not-present) fn.
> >
> > It is also possible to rewrite the functions to do an atomic replace of
> the
> > function pointer, which makes them safe for multithreaded usage. (Same
> > pattern as we use in Subversion for atomic initializations or even
> simpler
> > variants as it is safe to do the same LoadLibraryXX() and
> GetProcAddress()
> > in multiple threads at once, via the Windows loader lock.)
> >
> > But as far as I can see all the functions are always available on Windows
> XP
> > and later (and the others are not even used by APR and APR-Util), so I'm
> not
> > really sure if we should really spend more time on this.
>
> A patch like the following would make the code thread safe.
>
> [[
> Index: include/arch/win32/apr_arch_misc.h
> ===================================================================
> --- include/arch/win32/apr_arch_misc.h (revision 1547134)
> +++ include/arch/win32/apr_arch_misc.h (working copy)
> @@ -18,6 +18,7 @@
> #define MISC_H
>
> #include "apr.h"
> +#include "apr_atomic.h"
> #include "apr_portable.h"
> #include "apr_private.h"
> #include "apr_general.h"
> @@ -188,22 +189,23 @@
> * ERROR_INVALID_FUNCTION if the function cannot be loaded
> */
> #define APR_DECLARE_LATE_DLL_FUNC(lib, rettype, calltype, fn, ord, args,
> names) \
> - typedef rettype (calltype *apr_winapi_fpt_##fn) args; \
> - static apr_winapi_fpt_##fn apr_winapi_pfn_##fn = NULL; \
> - static int apr_winapi_chk_##fn = 0; \
> - static APR_INLINE int apr_winapi_ld_##fn(void) \
> - { if (apr_winapi_pfn_##fn) return 1; \
> - if (apr_winapi_chk_##fn ++) return 0; \
> - if (!apr_winapi_pfn_##fn) \
> - apr_winapi_pfn_##fn = (apr_winapi_fpt_##fn) \
> - apr_load_dll_func(lib, #fn, ord); \
> - if (apr_winapi_pfn_##fn) return 1; else return 0; }; \
> - static APR_INLINE rettype apr_winapi_##fn args \
> - { if (apr_winapi_ld_##fn()) \
> - return (*(apr_winapi_pfn_##fn)) names; \
> - else { SetLastError(ERROR_INVALID_FUNCTION); return 0;} }; \
> + typedef rettype (calltype *apr_winapi_fpt_##fn) args;
> \
> + static rettype calltype apr_winapi_ld_##fn args;
> \
> + static volatile apr_winapi_fpt_##fn apr_winapi_##fn =
> apr_winapi_ld_##fn; \
> + static rettype calltype apr_winapi_unavail_##fn args
> \
> + { SetLastError(ERROR_INVALID_FUNCTION); return 0; }
> \
> + static rettype calltype apr_winapi_ld_##fn args
> \
> + {
> \
> + apr_winapi_fpt_##fn dl_func = (apr_winapi_fpt_##fn)
> \
> + apr_load_dll_func(lib, #fn, ord);
> \
> + if (! dl_func)
> \
> + dl_func = apr_winapi_unavail_##fn;
> \
> + apr_atomic_casptr((volatile void**)&apr_winapi_##fn, dl_func,
> \
> + apr_winapi_ld_##fn);
> \
> + return apr_winapi_##fn names;
> \
> + }
>
> -#define APR_HAVE_LATE_DLL_FUNC(fn) apr_winapi_ld_##fn()
> +/*#define APR_HAVE_LATE_DLL_FUNC(fn) apr_winapi_ld_##fn() */
>
> /* Provide late bound declarations of every API function missing from
> * one or more supported releases of the Win32 API
> ]]
>
> It is not a complete solution as it breaks APR_HAVE_LATE_DLL_FUNC().
> The only caller of this macro uses it to check for the existence of
> WsaPoll(). This should just be checked with the Windows version/feature
> level like how we check all others functions in APR, or better yet the
> usage
> of WsaPoll should just be removed for the know issues this function has.
>
> See http://daniel.haxx.se/blog/2012/10/10/wsapoll-is-broken/ for a summary
> of the problems and Microsoft's explanation that they are not going to fix
> this 'for compatibility reasons'.
>
> In all our uses in Subversion and Serf we have explicit code to *not* use
> WsaPoll via apr for these reasons.
>
> If we use it the network IO will just get stuck on network problems.
>
> >
> > Bert
>
>
>
Received on 2013-12-07 16:17:33 CET