Bug 29755 - mod_usertrack should use err_headers_out
Summary: mod_usertrack should use err_headers_out
Status: RESOLVED LATER
Alias: None
Product: Apache httpd-2
Classification: Unclassified
Component: mod_usertrack (show other bugs)
Version: 2.0.52
Hardware: All other
: P1 normal (vote)
Target Milestone: ---
Assignee: Apache HTTPD Bugs Mailing List
URL: http://ken.coar.org/burrow/index?entr...
Keywords: FixedInTrunk, PatchAvailable
Depends on:
Blocks:
 
Reported: 2004-06-23 09:49 UTC by Sami J. Mäkinen
Modified: 2016-12-31 00:19 UTC (History)
0 users



Attachments

Note You need to log in before you can comment on or make changes to this bug.
Description Sami J. Mäkinen 2004-06-23 09:49:13 UTC
In short, we are running several large websites that have banner advertisements.
A friendly way to serve banners is to configure the IMG SRC tags so that
they refer to an apache server using mod_usertrack and mod_rewrite.
This way, we get a decently reliable visitor log and banner exposure log.
The redirect points to the "real" image server that gives long expire times
so that the browser does not have to download each banner every time,
but we still get nice logs for our advertisers.

The problem that we ran into, is that whenever you make a redirect from
mod_rewrite, the fixups hooks stop and mod_usertrack is never called.
So we don't get cookies logged when http status is 302.

I fixed this quite simply:

in mod_usertrack, modify the function

--- 8< ---
static void register_hooks(apr_pool_t *p)
{
    ap_hook_fixups(spot_cookie,NULL,NULL,APR_HOOK_MIDDLE);
}
--- 8< ---

to be

--- 8< ---
static void register_hooks(apr_pool_t *p)
{
    ap_hook_fixups(spot_cookie,NULL,NULL,APR_HOOK_FIRST);
}
--- 8< ---

...and make sure that mod_usertrack is loaded before mod_rewrite,
if using dynamic modules. We use RHEL3ES, and the apache distributed
by Red Hat is using dynamic modules for almost everything.

I would be very pleased if stock apache's mod_usertrack
would use APR_HOOK_FIRST in the future. I don't think
this small but important modification would break anything. :-o

P.S.

request.c says

AP_IMPLEMENT_HOOK_RUN_ALL(int,fixups,
                          (request_rec *r), (r), OK, DECLINED)

it took some time for me to find out that "RUN_ALL" here
does not mean "run all hooks in any case". It means actually
"run every hook until one of them does not return DECLINED or OK".
Sheesh.
Comment 1 Paul Querna 2004-11-04 03:28:58 UTC
Changed in 2.1 CVS.

Thanks for using Apache!
Comment 2 Sami J. Mäkinen 2004-11-15 21:47:36 UTC
Uh, the following hit us:

http://ken.coar.org/burrow/index?entry=511

in short, mod_usertrack really should set the cookie
header in err_headers_out, NOT headers_out.

The headers in headers_out will not get sent
if the request ends in a redirect. Sigh.

Furthermore, register_hooks should look like this:

static void register_hooks(apr_pool_t *p)
{
    /* fixup before mod_proxy, so that the proxied url will not
     * be escaped accidentally by our fixup.
     */
    static const char * const aszSucc[]={ "mod_rewrite.c", "mod_proxy.c",
                                          "mod_alias.c", NULL };
    ap_hook_fixups(spot_cookie, NULL, aszSucc, APR_HOOK_FIRST);
}
Comment 3 Sami J. Mäkinen 2004-11-15 22:04:17 UTC
Sigh.

I just checked Apache 2.0.52 source. It should be fixed.
A simple and suitable patch is attached below:

*** mod_usertrack_orig.c        2004-11-15 22:54:20.000000000 +0200
--- mod_usertrack.c     2004-11-15 22:57:57.000000000 +0200
***************
*** 145,151 ****
                                   NULL);
      }
  
!     apr_table_addn(r->headers_out,
                     (dcfg->style == CT_COOKIE2 ? "Set-Cookie2" : "Set-Cookie"),
                     new_cookie);
      apr_table_setn(r->notes, "cookie", apr_pstrdup(r->pool, cookiebuf));   /*
log first time */
--- 145,151 ----
                                   NULL);
      }
  
!     apr_table_addn(r->err_headers_out,
                     (dcfg->style == CT_COOKIE2 ? "Set-Cookie2" : "Set-Cookie"),
                     new_cookie);
      apr_table_setn(r->notes, "cookie", apr_pstrdup(r->pool, cookiebuf));   /*
log first time */
***************
*** 439,445 ****
  
  static void register_hooks(apr_pool_t *p)
  {
!     ap_hook_fixups(spot_cookie,NULL,NULL,APR_HOOK_MIDDLE);
  }
  
  module AP_MODULE_DECLARE_DATA usertrack_module = {
--- 439,450 ----
  
  static void register_hooks(apr_pool_t *p)
  {
!     /* fixup before mod_proxy, so that the proxied url will not
!      *      * escaped accidentally by our fixup.
!      */
!     static const char * const aszSucc[]={ "mod_rewrite.c", "mod_proxy.c",
!                                         "mod_alias.c", NULL };
!     ap_hook_fixups(spot_cookie, NULL, aszSucc, APR_HOOK_FIRST);
  }
  
  module AP_MODULE_DECLARE_DATA usertrack_module = {
Comment 4 Paul Querna 2005-04-06 23:04:42 UTC
This has beend fixed in 2.1.x, and not backported to 2.0.x.  I didn't feel it
was important enough to backport to 2.0.x.
Comment 5 Philippe Lantin 2011-03-22 13:05:14 UTC
Part of this fix has not made it to 2.2 or 2.3. Fix needed:


149c149
<     apr_table_addn(r->err_headers_out,
---
>     apr_table_addn(r->headers_out,
442,447c442
<     /* fixup before mod_proxy, so that the proxied url will not
<      *      * escaped accidentally by our fixup.
<     */
<     static const char * const aszSucc[]={ "mod_rewrite.c", "mod_proxy.c",
<                                          "mod_alias.c", NULL };
<     ap_hook_fixups(spot_cookie,NULL,aszSucc,APR_HOOK_FIRST);
---
>     ap_hook_fixups(spot_cookie,NULL,NULL,APR_HOOK_FIRST);
Comment 6 Eric Covener 2011-08-06 23:16:47 UTC
Applied with modifications as http://svn.apache.org/viewvc?rev=1154620&view=rev
Comment 7 Eric Covener 2016-12-31 00:19:59 UTC
Fixed in 2.4.x and later