httpd-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
Subject Re: [PATCH] dechunking, body filtering
Date Mon, 16 Oct 2000 21:11:39 GMT

> > >   if read_policy is REQUEST_CHUNKED_PASS and the transport encoding is
> > >   chunked...
> > >  
> > >     Disable filtering of the request body, because chunk headers would
> > >     be invalid
> > >     if the filter changed the length.
> > 
> > I'm not sure I like this, but I dislike the other options more.  The
> > problem here, is that we are changing how a request is received based on
> > information in the filter.  Then again, we only use REQUEST_CHUNKED_PASS
> > when we are discarding the whole body anyway.  I have no good answer, but
> > the solution I hate more than disabling filtering, is for the chunk filter
> > to fix the chunk headers.  The solution I like a bit more, is to remove
> > this option altogether, although that too is a bit bogus.  Just some food
> > for thought on this.
> I was wondering out loud some weeks ago on what to do with this
> issue...  the only response I got was a "shrug" from Roy :) 
> I've tried googling for REQUEST_CHUNKED_PASS but I didn't come up with
> anything outside of our code.  I doubt that there is anything.

I agree 100%, I am shrugging in my comment basically, because I don't see
a good answer.  Your solution is as good as any of the others.  :-)

> > > X CORE_IN shouldn't deliver the whole request body if it is big... one I/O
> > >   buffer worth of request body is plenty... otherwise we eat up virtual storage
> > 
> > Core in just passes the brigade up to http_filter, or whatever filter is
> > there.  http_filter should use a length of MAX_STRING_LENGTH when doing a
> > read.
> Yeah, I got my filter names wrong.  Anyway...  When http_filter() does
> a bucket-read to get data to deliver, it doesn't get to choose how
> much to read.  In the normal case where it reads a socket bucket, it
> will get up to IOBUFSIZE bytes.  A simple change for now would be to
> return once it does a read which delivers >0 bytes.  When the block
> parameter to bucket read actually does something for a socket this
> code needs to be revisited anyway.


> > > +static apr_status_t brigade_bgets(ap_bucket_brigade *bb,
> > > +                                  int remove_buckets,
> > > +                                  char *line,
> > > +                                  apr_ssize_t max_line_size)
> > 
> > <brigade_bgets snipped for brevity>
> > 
> > Why isn't this just getline?  This does a bit less than getline, but
> > getline is already there, and it is one less place for things to go
> > wrong.
> Tonight or tomorrow I'll look at using getline()...

BTW, I took a real quick look at getline.  It is always by-passing the
request-based input headers, so this should just work.

> I went to the version of this code in cvs which I replaced today but
> can't find much that I should pick up.  Part of it is that I report
> eos a little sooner...  another difference is that the length parm is
> unused in my version...  another issue is that some of the replaced
> code seemed to have the intention of telling the bucket read routine
> how much data to read.  I've got some minor tweaks which make it look
> more like what I replaced, but there isn't anything major.

Yeah.  After I saw the code completely, with the length parameter removed,
this looks good.

> > > +        if (r->read_body == REQUEST_CHUNKED_PASS) {
> > > +            /* can't filter the body *and* preserve the chunk headers */
> > > +            r->input_filters = r->connection->input_filters;
> > > +        }
> > > +        ap_add_input_filter("DECHUNK", NULL, r, r->connection);
> > 
> > This doesn't actually disable filtering.  I'm not really sure of the
> > correct way to disable filtering, but I have the feeling this will
> > actually need to happen in ap_get_client_block, not
> > ap_setup_client_block.  The problem is that it is perfectly possible for a
> > module to insert a filter between the two function calls.
> > Hmmmm.....
> no obvious solution...

The only other thing I can think of is to just remove all filters from the
request_rec.  Of course, this requires ap_filter_remove be written.

This looks really good now.  It was painful to get here, but I think we
finally got it right.  :-)


Ryan Bloom               
406 29th St.
San Francisco, CA 94131

View raw message