Re: svn commit: r231167 - /httpd/httpd/trunk/modules/generators/mod_cgi.c

[Paul Querna <ch...@force-elite.com>]

(10 months after the commit...)
jerenkrantz@apache.org wrote:
> Author: jerenkrantz
> Date: Tue Aug  9 21:32:13 2005
> New Revision: 231167
> 
> URL: http://svn.apache.org/viewcvs?rev=231167&view=rev
> Log:
> Fix bug where non-200 CGI responses will not send anything down filter chain.
> This is most notable when mod_cache is used.  This has been used in production
> on wiki.apache.org for a while now.
> 
> * modules/generators/mod_cgi.c
>   (cgi_handler): When a non-zero value is returned by scan_script, set the
>   status field and ensure that we have an EOS to send down the filer stack.


This change is causing a regression in mod_cgi, when the CGI is invalid,
it causes no ISE to be sent downstream, and a zero byte reply is sent to
the client instead:

http://issues.apache.org/bugzilla/show_bug.cgi?id=39710

mod_cgid does not replicate this EOS sending that mod_cgi has.  Was this
an oversight?  It has nearly the same code paths.


> 
> Modified:
>     httpd/httpd/trunk/modules/generators/mod_cgi.c
> 
> Modified: httpd/httpd/trunk/modules/generators/mod_cgi.c
> URL: http://svn.apache.org/viewcvs/httpd/httpd/trunk/modules/generators/mod_cgi.c?rev=231167&r1=231166&r2=231167&view=diff
> ==============================================================================
> --- httpd/httpd/trunk/modules/generators/mod_cgi.c (original)
> +++ httpd/httpd/trunk/modules/generators/mod_cgi.c Tue Aug  9 21:32:13 2005
> @@ -929,7 +929,18 @@
>          int ret;
>  
>          if ((ret = ap_scan_script_header_err_brigade(r, bb, sbuf))) {
> -            return log_script(r, conf, ret, dbuf, sbuf, bb, script_err);
> +            ret = log_script(r, conf, ret, dbuf, sbuf, bb, script_err);
> +
> +            /* Set our status. */
> +            r->status = ret;
> +
> +            /* Pass EOS bucket down the filter chain. */
> +            apr_brigade_cleanup(bb);
> +            b = apr_bucket_eos_create(c->bucket_alloc);
> +            APR_BRIGADE_INSERT_TAIL(bb, b);
> +            ap_pass_brigade(r->output_filters, bb);
> +
> +            return ret;
>          }
>  
>          location = apr_table_get(r->headers_out, "Location");
> 

Setting r->status here seems really wrong. If a script has a "Status:"
header, it is already handled inside ap_scan_script_header_err_core.
Infact, I don't see how adding a EOS here really fixes the problem -- if
you just returned ret like normal, the core should generate the ISE
output for you, and mod_cache should still cache it, if it is added to
the error filter chain....

-Paul