Fun with prefer_scripts feature in mod_negotiation

[Koen Holtman <Ko...@cern.ch>]

I am currently going through mod_negotiation and found a fun set of
bugs.  I would be interested in getting comments on the background of
the code concerned, and on how to proceed.  My preference would be to
ax this attempt at a feature completely.

Introduction: mod_negotiation contains some code to prefer cgi scripts
over plain variants in certain cases.  This code is broken.  The result is
that cgi scripts are never preferred.  The functionality which the code
tries to implement is not described in the module documentation.  Here is
the relevant code with some new comments by me:

[..mod_negotiation.c..]

/* Sometimes clients will give us no Accept info at all; this routine sets
 * up the standard default for that case, and also arranges for us to be
 * willing to run a CGI script if we find one.  (In fact, we set up to
 * dramatically prefer CGI scripts in cases where that's appropriate,
 * e.g., POST).
 */

static void maybe_add_default_encodings(negotiation_state *neg, int prefer_scrip
ts)
{
    accept_rec *new_accept = (accept_rec *) ap_push_array(neg->accepts);

    new_accept->type_name = CGI_MAGIC_TYPE;

    new_accept->quality = prefer_scripts ? 1e-20f : 1e20f;
	/* XXX: Eek! This is the wrong way around, we set the script
	   quality to 1e-20 if we _prefer_ scripts.  There is also a
	   bug in handle_multi() however: it always mis-calculates the
	   prefer_scripts argument as 1, so that we end up with
	   new_accept->quality = 1e-20 in all cases, which causes the
	   Apache negotiation algorithm to ignore any scripts if
	   present.  handle_map_file() always supplies
	   prefer_scripts==0, leading to an incorrect script quality
	   of 1e20f in all cases, but this would not hurt unless one
	   uses CGI_MAGIC_TYPE in a map file.

	   I guess nobody is attempting to use this `prefer scripts if
	   the request is scripty' functionality, else they would have
	   found out that never works and filed a bug report.  Maybe
	   we should just ax this feature?  -kh */

    new_accept->level = 0.0f;
    new_accept->max_bytes = 0.0f;

[...]
}
[.....]


static int handle_multi(request_rec *r)
{
[...]

    /* XXX: The calculation of the second (prefer_scripts) argument is
       buggy, we always calculate 1 because r->path_info is never NULL
       but always an empty string. (I don't think r->args and
       r->path_info can be non-empty strings anyway if a multiviews is
       invoked.)  The bug in the calculation is offset by another bug
       in maybe_add_default_encodings() however, see the comment there
       for more info. -kh */
  maybe_add_default_encodings(neg,
                                (r->method_number != M_GET) ||
                                r->args || r->path_info);

[...]
}
[...]

Any comments?  I did a web search and found an older version of the code
which had the prefer_scripts conditional expression the right way around
at
http://dev.apache.org/patches/old_versions/for_Apache_0.3/E24_content_negotiation.txt

[...]
+ void maybe_add_default_encodings(int prefer_scripts, FILE *out)
+ {
+     accept_rec cgi_accept_rec =
+       { CGI_MAGIC_TYPE, prefer_scripts?1e20:1e-20, 0.0, 0.0 };
+     accept_rec default_accept_rec =
+       { "*/*", 1.0, 0.0, 0.0 };
+ 
+     if (accepts.nactive == 0) add_accept (&accepts, &default_accept_rec,
out);
+       
+     add_accept(&accepts, &cgi_accept_rec, out);
+ }
[...]

Koen.