RE: Race condition in APR_DECLARE_LATE_DLL_FUNC() implementation

> -----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 11:52:45 CET