Race condition in APR_DECLARE_LATE_DLL_FUNC() implementation
Hi,
Someone in the Subversion team made our C tests run in parallel. With that
our buildbot found a race condition in the Windows specific
APR_DECLARE_LATE_DLL_FUNC() implementation.
The current code is
[[
#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;} }; \
#define APR_HAVE_LATE_DLL_FUNC(fn) apr_winapi_ld_##fn()
]]
(I can reproduce the problem locally)
The ++ on the second line of the function body makes calls in other threads
skip the initialization, which may cause them to see no function (or
theoretically a bad pointer).
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.
Bert
CMake build doesn't define 'WINNT', but apr headers check for it anyway
Hi, On Windows apr_arch_misc.h checks for the 'WINNT' define, which was unconditionally defined on Windows before the CMake build. But now it is no longer defined. This enables some additional dynamic load operations that could fail in some cases (See Race condition in APR_DECLARE_LATE_DLL_FUNC() implementation). We could fix this by defining WINNT, but we might as well just remove the check as we don't support systems older than NT 4.0 anyway.. Bert
Re: CMake build doesn't define 'WINNT', but apr headers check for it anyway
On Thu, Dec 5, 2013 at 9:13 AM, Bert Huijben b...@qqmail.nl wrote: Hi, On Windows apr_arch_misc.h checks for the ‘WINNT’ define, which was unconditionally defined on Windows before the CMake build. But now it is no longer defined. This enables some additional dynamic load operations that could fail in some cases (See “Race condition in APR_DECLARE_LATE_DLL_FUNC() implementation”). We could fix this by defining WINNT, but we might as well just remove the check as we don’t support systems older than NT 4.0 anyway.. Bert Thanks! Visual Studio project file maintainers, can I just remove the /D WINNT everywhere? I hate to leave that dangling. (Obviously the C code will be changed to stop referencing it too :) ) -- Born in Roswell... married an alien... http://emptyhammock.com/
Re: CMake build doesn't define 'WINNT', but apr headers check for it anyway
G. On 12/5/2013 9:23 AM, Gregg Smith wrote: On 12/5/2013 6:48 AM, Jeff Trawick wrote: On Thu, Dec 5, 2013 at 9:13 AM, Bert Huijben b...@qqmail.nl mailto:b...@qqmail.nl wrote: Hi, On Windows apr_arch_misc.h checks for the ‘WINNT’ define, which was unconditionally defined on Windows before the CMake build. But now it is no longer defined. This enables some additional dynamic load operations that could fail in some cases (See “Race condition in APR_DECLARE_LATE_DLL_FUNC() implementation”). We could fix this by defining WINNT, but we might as well just remove the check as we don’t support systems older than NT 4.0 anyway.. Bert Thanks! Visual Studio project file maintainers, can I just remove the /D WINNT everywhere? I hate to leave that dangling. (Obviously the C code will be changed to stop referencing it too :) ) There's no reason to support 9x, we just want all the WINNT code. Once all the 9x stuff is gone, we can drop the define.
Re: CMake build doesn't define 'WINNT', but apr headers check for it anyway
On Thu, 05 Dec 2013 09:24:24 -0800 Gregg Smith g...@gknw.net wrote: Once all the 9x stuff is gone, we can drop the define. Precisely. And that would be a version-major/minor change, imho. In the interim, simply -D (ugh... /D it) WINNT. Problem solved. Leave it there for someone to discover 7 years from now and ask why an unused macro has been defined.
Re: Race condition in APR_DECLARE_LATE_DLL_FUNC() implementation
On Thu, 5 Dec 2013 15:01:05 +0100 Bert Huijben b...@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. Several of these must die with the deprecation of Win2k/XP, but several others must be added or persist due to Win 8.1/8.0/7.0 discrepancies.
