httpd-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Justin Erenkrantz <jerenkra...@apache.org>
Subject Re: [PATCH] fix LimitRequestBody for all input handlers
Date Fri, 12 Apr 2002 20:49:20 GMT
On Fri, Apr 12, 2002 at 01:31:04PM -0700, Aaron Bannert wrote:
> On Fri, Apr 12, 2002 at 01:18:19PM -0700, Justin Erenkrantz wrote:
> > On Fri, Apr 12, 2002 at 12:29:52PM -0700, 'Aaron Bannert' wrote:
> > > Ok, what's the word on this? Is my patch good or can we settle on another
> > > way to do this and just be consistent?
> > 
> > Please figure out another way to do it.  Your change is wrong.  
> 
> I don't care if my change is wrong, this is a bug that needs to be
> fixed and it seems to me that we can't agree what the correct solution
> is. "Please figure out another way to do it" is not good enough.

You asked if your patch was good.  Your change breaks any
ap_get_brigade()-using modules.  So, yes, we need to find another
way (and the we *was* implicit in my earlier post).  I have tried to
suggest other ways to do it, but you and Ryan don't seem to care to
hear them.  Fine, but your patch isn't going in the repository.

> > My take is that the non-ap_get_brigade() path (i.e.
> > ap_get_client_block/mod_cgi) isn't handling the error case when
> > ap_get_brigade says there is no data available due to the
> > Limit case.
> 
> If we are allowing the non-ap_get_brigade() path then are we
> requiring *all* modules that might deal with HTTP to have to
> understand Content-length: and chunked encoding? If that is
> the case, then we have far more than one bug to deal with.

No, they don't.  That is all handled by ap_http_filter().
I don't see why any module even needs to know that information.
Just call ap_get_brigade.  If all is well, you'll see an EOS at
the end of the request input.  The only problem comes in with how
to deal with 100-Continue responses.  But, that is notoriously
module-specific, and I'm not aware of any modules (save ones
I've written) that use 100-Continue.  Those modules can be smart
enough to handle that on their own.  (They can send the 100-Continue
with a flush bucket on the output as it reads the request.)

> That has nothing to do with my patch. Besides, I'm not even sure what
> you are saying. When should and should not (in your opinion) -1 be
> returned from ap_get_client_block?

That API is broken as there is no way to indicate to the caller of
ap_get_client_block that an error has occurred.  This is very
similar to the prior problems with ap_getline that resulted
in the AP_MODE_GETLINE mode to ap_get_brigade.  As I said,
ap_get_client_block() marks the connection as aborted if an
error occurred.  I think that whole segment of code is
broken.  

I'm still not clear why you guys are so leery of using buckets on
the input side.  -- justin

Mime
View raw message