RE: Race condition in APR_DECLARE_LATE_DLL_FUNC() implementation
See Branko's reaction for why the atomic handling will be necessary when you
don't want to hardcode hardware assumptions. Bugs in these assumptions are
exactly the problem we try to solve here.
This implementation idea is copied from how Microsoft implemented some of
their thunking in a completely thread safe way, with the least amount of
overhead possible after the first call. You don't even need function
inlining this way for an optimal call.
And to your third point: The function pointer itself is the inline function
with this implementation.
Your suggestion is like:
// A already defined as variable
static APR_INLINE int A()
{
return A();
}
Luckily you just get a compiler error for the duplicated symbol, otherwise
it would just implement endless recursion instead of an actual function
call.
Bert
From: daniel.lescoh...@cbsinteractive.com
[mailto:daniel.lescoh...@cbsinteractive.com] On Behalf Of Daniel Lescohier
Sent: zaterdag 7 december 2013 16:05
To: Bert Huijben
Cc: William A. Rowe Jr.; Stefan Fuhrmann; APR Developer List; Stefan
Fuhrman; Philip Martin; Subversion Development
Subject: Re: Race condition in APR_DECLARE_LATE_DLL_FUNC() implementation
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 b...@qqmail.nl
mailto:b...@qqmail.nl wrote:
-Original Message-
From: Bert Huijben [mailto:b...@qqmail.nl mailto:b...@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:wr...@rowe-clan.net
mailto:wr...@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.fuhrm...@wandisco.com
mailto:stefan.fuhrm...@wandisco.com wrote:
On Fri, Dec 6, 2013 at 6:05 AM, William A. Rowe Jr.
wr...@rowe-clan.net mailto:wr...@rowe-clan.net wrote:
On Thu, 5 Dec 2013 15:01:05 +0100
Bert Huijben b...@qqmail.nl mailto: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.
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
mod_dav_svn changing the request filename - interaction with mod_rewrite
Hi, Revision 1512432 causes a regression when mod_dav_svn is used together with mod_rewrite, which we have done successfully since Subversion 1.5. I have also studied the follow up commits which change the approach somewhat from setting filename to null into a bogus file. Use case: Using mod_rewrite we can serve information pages using the correct Subversion URL. Example: http://svn.example.com/svn/repo/folder/?view=details This page might be served by PHP or other script which is located elsewhere on the server (say /details/index.php). By using a rewrite with PT flag this page can be displayed without redirecting to a separate URL. It was possible before 1.8.3. - In 1.8.1 works. - In 1.8.3 the filename is set to null and we get a 404. - In 1.8.5 the filename is set to something like 'svn:/pathtorepositories/repo/details/'. Also 404. The PT flag sends the rewritten URI back through URL mapping. For some reason, the recently introduced hook dav_svn__translate_name is executed despite the rewritten URL being outside of the /svn Location. Consequently the request filename field is modified by mod_dav_svn. My best guess is that the sequence becomes: - mod_rewrite gets translate_name hook, changes the URI. - mod_dav_svn gets translate_name hook, because it was initially expected to handle the request. - renewed URL mapping finds that mod_dav_svn should not handle, selects another one, e.g. mod_php. - ... I did some searching in dev-list for discussions on these changes, but I didn't find much. Any recommended reading? Perhaps the translate_name hook needs to decline in some additional situations, e.g. when the requested path is actually no longer going to be served by mod_dav_svn. We have failed to find a workaround for this issue so it is a blocker for us to upgrade our production servers beyond 1.8.1. Thanks in advance, Thomas Å.
Re: mod_dav_svn changing the request filename - interaction with mod_rewrite
On 12/11/13 5:18 AM, Thomas Åkesson wrote:
Hi,
Revision 1512432 causes a regression when mod_dav_svn is used together with
mod_rewrite, which we have done successfully since Subversion 1.5. I have
also studied the follow up commits which change the approach somewhat from
setting filename to null into a bogus file.
Use case:
Using mod_rewrite we can serve information pages using the correct
Subversion URL.
Example:
http://svn.example.com/svn/repo/folder/?view=details
This page might be served by PHP or other script which is located elsewhere
on the server (say /details/index.php). By using a rewrite with PT flag this
page can be displayed without redirecting to a separate URL. It was possible
before 1.8.3.
- In 1.8.1 works.
- In 1.8.3 the filename is set to null and we get a 404.
- In 1.8.5 the filename is set to something like
'svn:/pathtorepositories/repo/details/'. Also 404.
The PT flag sends the rewritten URI back through URL mapping. For some
reason, the recently introduced hook dav_svn__translate_name is executed
despite the rewritten URL being outside of the /svn Location. Consequently
the request filename field is modified by mod_dav_svn.
My best guess is that the sequence becomes:
- mod_rewrite gets translate_name hook, changes the URI.
- mod_dav_svn gets translate_name hook, because it was initially expected to
handle the request.
- renewed URL mapping finds that mod_dav_svn should not handle, selects
another one, e.g. mod_php.
- ...
I did some searching in dev-list for discussions on these changes, but I
didn't find much. Any recommended reading?
Perhaps the translate_name hook needs to decline in some additional
situations, e.g. when the requested path is actually no longer going to be
served by mod_dav_svn.
We have failed to find a workaround for this issue so it is a blocker for us
to upgrade our production servers beyond 1.8.1.
Hmm this is going to be a pain to fix (possibly impossible). Because what
mod_rewrite is doing is really hackish. When you use the PT (PassThrough) flag
mod_rewrite puts passthrough:/my/new/URL into r-filename. Then eventually it
moves it to r-uri, but not until after it goes through our translate_name
hook. So we can't reliably know if the request is going to be served by us or
not by asking for the per_dir config.
If we disable handling requests that have a passthrough filename then if the
passthrough ends up coming back to us (e.g. /svn/repo/b is mapped to
/svn/repo/a) then the request ends up being mapped to a bogus filesystem path.
If we enable it then requests (like in your configuration) that aren't going
to be handled by us end up not getting mapped properly.
The following patch (code stolen from mod_charset_lite) avoids the problem
you're having but due to the above isn't a good solution.
[[[
Index: subversion/mod_dav_svn/mod_dav_svn.c
===
--- subversion/mod_dav_svn/mod_dav_svn.c(revision 1549924)
+++ subversion/mod_dav_svn/mod_dav_svn.c(working copy)
@@ -1127,6 +1127,15 @@
if (!conf-fs_path !conf-fs_parent_path)
return DECLINED;
+ /* mod_rewrite indicators */
+ if (r-filename
+ (!strncmp(r-filename, redirect:, 9)
+ || !strncmp(r-filename, gone:, 5)
+ || !strncmp(r-filename, passthrough:, 12)
+ || !strncmp(r-filename, forbidden:, 10))) {
+ return DECLINED;
+ }
+
if (dav_svn__is_parentpath_list(r))
{
/* SVNListParentPath is on and the request is for the conf-root_dir,
]]]
None of these use cases are strictly legal. The namespace both path wise and
query argument wise belong to SVN. Overlaying your own paths or query
arguments is always going to be prone to future incompatibilities. You've just
finally been burned.
The right way to do what you've been doing is some other path like (where /view
is not configured to be handled by Subversion):
http://svn.example.com/view/details/repo/folder
or
http://svn.example.com/view/repo/folder?details
I'll see what I can do but I think you may just have to bite the bullet and
change your configuration.
Re: mod_dav_svn changing the request filename - interaction with mod_rewrite
On 12/11/13 10:45 AM, Ben Reser wrote:
Hmm this is going to be a pain to fix (possibly impossible). Because what
mod_rewrite is doing is really hackish. When you use the PT (PassThrough)
flag
mod_rewrite puts passthrough:/my/new/URL into r-filename. Then eventually it
moves it to r-uri, but not until after it goes through our translate_name
hook. So we can't reliably know if the request is going to be served by us or
not by asking for the per_dir config.
Okay this isn't strictly true. r-uri is set before it gets to us. It's just
that mod_rewrite doesn't trigger a new location walk (that happens after the
map_to_storage hooks run which is the next step after the translate_name hook).
In order for mod_rewrite to work transparently the location_walk would need to
be triggered again.
We could do that on our side with something like this:
[[[
Index: subversion/mod_dav_svn/mod_dav_svn.c
===
--- subversion/mod_dav_svn/mod_dav_svn.c(revision 1549924)
+++ subversion/mod_dav_svn/mod_dav_svn.c(working copy)
@@ -1121,8 +1121,21 @@
const char *fs_path, *repos_basename, *repos_path;
const char *ignore_cleaned_uri, *ignore_relative_path;
int ignore_had_slash;
- dir_conf_t *conf = ap_get_module_config(r-per_dir_config, dav_svn_module);
+ dir_conf_t *conf;
+ /* rewalk the location blocks if mod_rewrite rewrote something */
+ if (apr_table_get(r-notes, mod_rewrite_rewritten))
+{
+ r-per_dir_config = r-server-lookup_defaults;
+ ap_location_walk(r);
+#if AP_MODULE_MAGIC_AT_LEAST(20110117,0)
+ ap_if_walk(r);
+#endif
+}
+
+ /* Now that we have an accurate per_dir_config retrieve our config */
+ conf = ap_get_module_config(r-per_dir_config, dav_svn_module);
+
/* module is not configured, bail out early */
if (!conf-fs_path !conf-fs_parent_path)
return DECLINED;
]]]
But I really don't think we should for several reasons.
First of all it starts messing with httpd internals that I don't think we
should really need to be doing. Note that I had to include the ap_if_walk()
for 2.3.x+. This sort of fix would have to forever be kept in sync with the
code in ap_process_request_internal(). Making it rather brittle.
This changes the conf available to all the other modules that follow us in the
translate_name hooks. Since we have to do this before we can tell if we're
configured, this impacts all traffic not just us. We could go to the effort of
storing the state and then putting things back if we DECLINED, but that's not
as simple as it seems. There is a cache for the location and if walks that I'm
not sure if we have to restore that.
It seems to me that this is something that mod_rewrite should be doing and if
since it isn't there's probably a really good reason for it not to do this.
So at this point I think this needs to be taken to the d...@httpd.apache.org
list. I'll start a new thread over there.
SVN Book diff idea for the HTML version
It would be nice to have SVN book versions on the website that have a differing portions of the text in blue for changes, and green for new additions and red for now obsolete parts. It would make re-reading the book for a new version much easier for old hands, you could immediately spot everything you need to reconsider. So, if I upgrade from 1.6 to 1.7, I would like to have the normal 1.7 book, the 1.7/1.6 'blue/green' version and the 1.6/1.7 'red' version. (blue green might not be possible to automate, but even just the blue version would be very useful, or perhaps it's even possible to have this all in one version.) regards, Gabriela -- Visit my Coding Diary: http://gabriela-gibson.blogspot.com/
Re: SVN Book diff idea for the HTML version
On 12/11/13 4:25 PM, Gabriela Gibson wrote: It would be nice to have SVN book versions on the website that have a differing portions of the text in blue for changes, and green for new additions and red for now obsolete parts. It would make re-reading the book for a new version much easier for old hands, you could immediately spot everything you need to reconsider. So, if I upgrade from 1.6 to 1.7, I would like to have the normal 1.7 book, the 1.7/1.6 'blue/green' version and the 1.6/1.7 'red' version. (blue green might not be possible to automate, but even just the blue version would be very useful, or perhaps it's even possible to have this all in one version.) The book is a separate project. It has a separate list for feedback at svnbook-...@red-bean.com (which is mentioned on http://svnbook.red-bean.com/ site under the Feedback/Contributing section). The listinfo page for the list is here: http://www.red-bean.com/mailman/listinfo/svnbook-dev
