Re: cvs commit: httpd-2.0/modules/http http_protocol.c
Jeff Trawick wrote: > > Modified:modules/http http_protocol.c > > Log: > > don't apply byte ranges to redirects, error documents, etc. > > This needs to be listed in CHANGES. done. Greg
Re: cvs commit: httpd-2.0/modules/http http_protocol.c
[EMAIL PROTECTED] writes: > gregames2002/10/25 11:25:12 > > Modified:modules/http http_protocol.c > Log: > don't apply byte ranges to redirects, error documents, etc. This needs to be listed in CHANGES. -- Jeff Trawick | [EMAIL PROTECTED] Born in Roswell... married an alien...
Re: cvs commit: httpd-2.0/modules/http http_protocol.c
Cliff Woolley wrote: >On Wed, 4 Sep 2002, Brian Pane wrote: > > > >>That's not guaranteed. The API, as currently documented, only >>guarantees that it will return an apr_bucket_brigade*, not that >>it will be non-null. >> >> > >It's non-null as long as apr_palloc returns non-null. Which it is. In >other words, it is part of the API. You want me to document it? Fine, I >will. :) > > Thanks. With the current documentation, there's no guarantee that apr_brigade_create() uses apr_palloc(). In fact, the comments for apr_brigade_create() explicitly state that "data is not allocated of the pool." Brian
Re: cvs commit: httpd-2.0/modules/http http_protocol.c
On Wed, 4 Sep 2002, Brian Pane wrote: > That's not guaranteed. The API, as currently documented, only > guarantees that it will return an apr_bucket_brigade*, not that > it will be non-null. It's non-null as long as apr_palloc returns non-null. Which it is. In other words, it is part of the API. You want me to document it? Fine, I will. :)
Re: cvs commit: httpd-2.0/modules/http http_protocol.c
Cliff Woolley wrote:
>On 5 Sep 2002 [EMAIL PROTECTED] wrote:
>
>
>
>> bb = apr_brigade_create(r->pool, r->connection->bucket_alloc);
>> +if (bb == NULL) {
>> +r->connection->keepalive = AP_CONN_CLOSE;
>> +return -1;
>> +}
>>
>>
>
>apr_brigade_create() will never return NULL. This bit is unnecessary.
>
>
That's not guaranteed. The API, as currently documented, only
guarantees that it will return an apr_bucket_brigade*, not that
it will be non-null.
Brian
Re: cvs commit: httpd-2.0/modules/http http_protocol.c
On 5 Sep 2002 [EMAIL PROTECTED] wrote:
>bb = apr_brigade_create(r->pool, r->connection->bucket_alloc);
> +if (bb == NULL) {
> +r->connection->keepalive = AP_CONN_CLOSE;
> +return -1;
> +}
apr_brigade_create() will never return NULL. This bit is unnecessary.
--Cliff
Re: cvs commit: httpd-2.0/modules/http http_protocol.c
On Fri, 31 May 2002, Justin Erenkrantz wrote: > httpd-test has no tests for input filtering. mod_input_body_filter.c at least, no? the protocol/ tests also hit input filters. > If I knew how to > get perl to send bogus requests, I would. But, my perl-fu is > severely lacking. -- justin see t/protocol/echo,nntp-like.t, you can send a request in any my $module = 'default'; #normally connects to port 8529 my $sock = Apache::TestRequest::vhost_socket($module); print $socket "SET \ ppp/2.2" ...
Re: cvs commit: httpd-2.0/modules/http http_protocol.c
On Fri, May 31, 2002 at 02:21:52PM -0700, Ryan Bloom wrote: > Without this fix, the entire test suite fails, because the HTTP_IN > filter is sending requests with 0 Content-Length to the > CORE_INPUT_FILTER to read the body. This means that every request times > out after some timeout. It has nothing to do with Jeff's problem, > because EVERY test was taking forever. I did run the test-suite, so if > this breaks anything, there is no test for it. As I said, I didn't see this problem. I'm currently building an updated tree to test again. If I can reproduce it, I'll try to fix it. However, we need the BODY_NONE state in order to handle trailers. Your commit breaks trailers. httpd-test has no tests for input filtering. If I knew how to get perl to send bogus requests, I would. But, my perl-fu is severely lacking. -- justin
RE: cvs commit: httpd-2.0/modules/http http_protocol.c
> From: Justin Erenkrantz [mailto:[EMAIL PROTECTED]] > > On Fri, May 31, 2002 at 08:41:06PM -, [EMAIL PROTECTED] wrote: > > rbb 2002/05/31 13:41:06 > > > > Modified:modules/http http_protocol.c > > Log: > > If the request doesn't have a body, then don't try to read it. > Without > > this, the httpd-test suite was taking five minutes for EVERY test. > > This breaks chunk trailers. Please revert. > > What is the exact problem you are seeing? httpd-test was working > fine for me. But, I will revert this locally and see if I can > reproduce this. This may have been related to Jeff's autoindex > problem. -- Justin Without this fix, the entire test suite fails, because the HTTP_IN filter is sending requests with 0 Content-Length to the CORE_INPUT_FILTER to read the body. This means that every request times out after some timeout. It has nothing to do with Jeff's problem, because EVERY test was taking forever. I did run the test-suite, so if this breaks anything, there is no test for it. Ryan
Re: cvs commit: httpd-2.0/modules/http http_protocol.c
On Fri, May 31, 2002 at 08:41:06PM -, [EMAIL PROTECTED] wrote: > rbb 2002/05/31 13:41:06 > > Modified:modules/http http_protocol.c > Log: > If the request doesn't have a body, then don't try to read it. Without > this, the httpd-test suite was taking five minutes for EVERY test. This breaks chunk trailers. Please revert. What is the exact problem you are seeing? httpd-test was working fine for me. But, I will revert this locally and see if I can reproduce this. This may have been related to Jeff's autoindex problem. -- justin
Re: cvs commit: httpd-2.0/modules/http http_protocol.c
On Thu, May 30, 2002 at 05:29:26PM -0700, Justin Erenkrantz wrote: > Per my earlier message, I believe this is the wrong way to do this. > > If we get a neg chunk length, we shouldn't *ever* get back into > ap_http_filter (HTTP_IN). The whole point of the 413 error is > that we refuse to read the request. -- justin Yeah, I agree that we need to fix how we deal with fatal errors like this, but we still shouldn't leave bogus values lying around in things like r->remaining. I'm only trying to fix the assert, and I agree that this doesn't fix the problem you're describing. I'm testing the conditional you posted right now, but if it works for you then commit it. :) -aaron
Re: cvs commit: httpd-2.0/modules/http http_protocol.c
On Fri, May 31, 2002 at 12:23:34AM -, [EMAIL PROTECTED] wrote: > aaron 02/05/30 17:23:34 > > Modified:modules/http http_protocol.c > Log: > This fixes a failed assert when r->remaining is left in a negative > state and we hit some other error (like permission failure) causing > an internal redirect causing us to reevaluate the input buffers > (for discarding the request body). Per my earlier message, I believe this is the wrong way to do this. If we get a neg chunk length, we shouldn't *ever* get back into ap_http_filter (HTTP_IN). The whole point of the 413 error is that we refuse to read the request. -- justin
Re: cvs commit: httpd-2.0/modules/http http_protocol.c
On 30 May 2002 [EMAIL PROTECTED] wrote: > -bb = apr_brigade_create(f->c->pool, f->c->bucket_alloc); > +apr_brigade_cleanup(bb); The old code could actually cause some problems. Some bucket types (eg pool buckets) have cleanups that run. If the bucket is cleaned up when the r->pool dies but it is still sitting in a brigade that's forgotten about until c->pool is cleaned up (as the old code here would have caused), then the c->pool cleanup is going to do nasty things to the memory now being used by the already-dead bucket it thinks is still within it. Don't know that that's actually what's happening here, just thought I'd point it out. There's really not much way around it other than cleaning up buckets when you're done with them (meaning don't allocate brigades out of c->pool unless you're sure all the buckets in it will live that long :). --Cliff
Re: cvs commit: httpd-2.0/modules/http http_protocol.c
On Wed, May 29, 2002 at 10:07:46PM +0200, Martin Kraemer wrote:
> On Wed, May 29, 2002 at 02:57:27PM -, [EMAIL PROTECTED] wrote:
> > Ignore leading zeros when parsing hex value for chunk extensions.
> >
> > +/* Skip leading zeros */
> > +while (*b == '0') {
> > +++b;
> > +}
> > +
> >while (apr_isxdigit(*b) && (chunkbits > 0)) {
>
> This patch will IMHO not change anything at all. Leading zeros are
> added by the while (apr_isxdigit..) loop by shifting 0 << 4 and adding 0.
> They never produce any overflow condition, no matter how many there are.
Actually, the theory is that we can only handle a known number of 4-bit
hex digits, so we ignore leading zeros before we start counting those
digits. If we use too many digits, we run out of chunkbits and detect
the overflow and return -1. If we use exactly the right amount of
digits, but overflow the sign bit, the result will be <0 (is this
a valid assumption?). So, if the result is negative, it overflowed.
-aaron
Re: cvs commit: httpd-2.0/modules/http http_protocol.c
On Wed, May 29, 2002 at 02:57:27PM -, [EMAIL PROTECTED] wrote:
> Ignore leading zeros when parsing hex value for chunk extensions.
>
> +/* Skip leading zeros */
> +while (*b == '0') {
> +++b;
> +}
> +
>while (apr_isxdigit(*b) && (chunkbits > 0)) {
This patch will IMHO not change anything at all. Leading zeros are
added by the while (apr_isxdigit..) loop by shifting 0 << 4 and adding 0.
They never produce any overflow condition, no matter how many there are.
But it might be interesting to check the current value of
chunksize within the loop:
while (apr_isxdigit(*b)) {
int xvalue = 0;
...set xvalue to the next hex digit, value 0 thru 15...
/* ---> Add here: a check whether the chunk will overflow the limit */
if (chunksize > ((limit_req_line + 15) >> 4))
signal an error;
chunksize = (chunksize << 4) | xvalue;
++b;
}
But we need
a) an extra parameter to pass the limit's value
(something like r->server->limit_req_line or a new configurable
max.chunk size) and
b) an error condition (get_chunk_size() currently has none)
to signal such an error.
Martin
--
<[EMAIL PROTECTED]> | Fujitsu Siemens
Fon: +49-89-636-46021, FAX: +49-89-636-47655 | 81730 Munich, Germany
Re: cvs commit: httpd-2.0/modules/http http_protocol.c
On Wed, May 29, 2002 at 06:42:59AM -, [EMAIL PROTECTED] wrote: >ctx->remaining = get_chunk_size(line); There remains a type mismatch in this code (ctx->remaining is an apr_off_t while get_chunk_size() returns a long). Will this cause any problems? I'm thinking get_chunk_size can just return an apr_off_t, but I'm too sleepy to think about this right now. :) -aaron
Re: cvs commit: httpd-2.0/modules/http http_protocol.c http_request.c
On Tue, May 28, 2002 at 11:38:31PM -, Justin Erenkrantz wrote: > jerenkrantz02/05/28 16:38:31 > > Modified:.CHANGES >modules/http http_protocol.c http_request.c > Log: > Correctly return 413 when an invalid chunk size is given on input. > > - If get_chunk_size() returns a negative number, that probably implies > an overflow. So, create a 413 error and pass it to the output filters. I think get_chunk_size() should make sure that it's not overflowing a long, and then return failure if that happens. I'll see if I can produce a patch soonish. -aaron
Re: cvs commit: httpd-2.0/modules/http http_protocol.c http_request.c
On Tue, May 28, 2002 at 11:38:31PM -, [EMAIL PROTECTED] wrote: > jerenkrantz02/05/28 16:38:31 > > Modified:.CHANGES >modules/http http_protocol.c http_request.c > Log: > Correctly return 413 when an invalid chunk size is given on input. > > - If get_chunk_size() returns a negative number, that probably implies > an overflow. So, create a 413 error and pass it to the output filters. > - Modify ap_discard_request_body() to return OK quickly if we're a subreq > or our status code implies that we will be dropping the connection. > - Modify ap_die() so that if the new status implies that we will drop > the connection, that we correctly indicate that we can not keepalive > this connection. (Without this, the error is returned, but the connection > is not closed.) This should resolve the invalid chunk line problem by returning 413. I ran with httpd-test and all of the tests passed - except for some SSL tests which are still broken for me due to my OpenSSL version. Please let me know if you find any problems with this. This took a LOT longer than it should have. =( -- justin
Re: cvs commit: httpd-2.0/modules/http http_protocol.c
[EMAIL PROTECTED] wrote: > > jerenkrantz02/05/06 00:43:40 > > Modified:.CHANGES STATUS >include httpd.h >modules/http http_protocol.c > Log: > Rewrite ap_byterange_filter so that it can work with data that does not > have a predetermined C-L - such as data that passes through mod_include. just tried this with my SSI/Range: test cases...I can't break it any more. Nice job, Justin! Greg
Re: cvs commit: httpd-2.0/modules/http http_protocol.c
On Mon, 6 May 2002, Justin Erenkrantz wrote: > You know, I *thought* that macro existed. =) Should have checked > first. Thanks for catching this! -- justin No prob. Functionally equivalent in every way, just ... prettier. :) There are probably many places in the code that still use APR_BRIGADE_CONCAT in a backwards way (_PREPEND is a very recent addition)... I'll do a grep later and weed out any remaining cases. --Cliff -- Cliff Woolley [EMAIL PROTECTED] Charlottesville, VA
Re: cvs commit: httpd-2.0/modules/http http_protocol.c
On Mon, May 06, 2002 at 08:14:53AM -, [EMAIL PROTECTED] wrote: > +/* Prepend any earlier saved brigades. */ > +APR_BRIGADE_PREPEND(bb, ctx->bb); You know, I *thought* that macro existed. =) Should have checked first. Thanks for catching this! -- justin
Re: cvs commit: httpd-2.0/modules/http http_protocol.c
On Mon, 6 May 2002, Cliff Woolley wrote: > > +ctx->boundary = apr_psprintf(r->pool, "%qx%lx", > > + r->request_time, (long) getpid()); > Is %qx portable? Shouldn't that be APR_TIME_T_FMT? Oh duh, this is apr_psprintf(), not printf(). Nevermind. ;) --Cliff -- Cliff Woolley [EMAIL PROTECTED] Charlottesville, VA
Re: cvs commit: httpd-2.0/modules/http http_protocol.c
On 6 May 2002 [EMAIL PROTECTED] wrote:
> - Remove r->boundary since it is possible to have this self-contained in
> boundary's ctx. (May require MMN bump?)
That would definitely require an MMN bump.
> +ctx->boundary = apr_psprintf(r->pool, "%qx%lx",
> + r->request_time, (long) getpid());
Is %qx portable? Shouldn't that be APR_TIME_T_FMT?
> -if (ctx->bb) {
> +if (!APR_BRIGADE_EMPTY(ctx->bb)) {
>APR_BRIGADE_CONCAT(ctx->bb, bb);
>bb = ctx->bb;
> -ctx->bb = NULL; /* ### strictly necessary? call brigade_destroy? */
>}
> +apr_brigade_destroy(ctx->bb);
That apr_brigade_destroy() call is unnecessary. ctx->bb is allocated from
a pool, and you know it's empty because you just emptied it. There's
nothing to destroy. Ie, the answer to the ### comment was "no, and no."
:)
PS: If you're looking for a good way to test this stuff, run a big PDF
through it and view with Acrobat Reader. If Acrobat locks up, the
byterange filter is broken. ;)
--Cliff
--
Cliff Woolley
[EMAIL PROTECTED]
Charlottesville, VA
Re: cvs commit: httpd-2.0/modules/http http_protocol.c
On Mon, May 06, 2002 at 07:43:40AM -, [EMAIL PROTECTED] wrote: > - Remove r->boundary since it is possible to have this self-contained in > boundary's ctx. (May require MMN bump?) I believe removing a field from request_rec requires a MMN bump. Am I right? -- justin
Re: cvs commit: httpd-2.0/modules/http http_protocol.c
"Sander Striker" <[EMAIL PROTECTED]> writes: > > "Sander Striker" <[EMAIL PROTECTED]> writes: > > > > > > From: [EMAIL PROTECTED] [mailto:[EMAIL PROTECTED]] > > > > Sent: 22 March 2002 21:37 > > > > > > > trawick 02/03/22 12:37:04 > > > > > > > > Modified:modules/http http_protocol.c > > > > Log: > > > > add an extra level of parentheses to say "yes I know what I'm > > > > doing with that single '='" and more importantly to quiet a > > > > gcc -Wall warning > It is not about cleaning up the warning, which is a good thing. It is > about making it more readable. > > if ((a = b)) > > is less obvious as > > if ((a = b) != 0) That's exactly the kind of information I can understand :) I'll fix it right now. Thanks for persevering! Jeff -- Jeff Trawick | [EMAIL PROTECTED] Born in Roswell... married an alien...
Re: cvs commit: httpd-2.0/modules/http http_protocol.c
On Tue, 26 Mar 2002, Greg Stein wrote: > D'oh!!! > I generated the input from the M_FOO symbols. Of course, there isn't an > M_HEAD symbol, so... > :-) > Thanks for fixing this! No problem! --Cliff -- Cliff Woolley [EMAIL PROTECTED] Charlottesville, VA
Re: cvs commit: httpd-2.0/modules/http http_protocol.c
On Mon, Mar 25, 2002 at 01:10:06AM -, [EMAIL PROTECTED] wrote: > jwoolley02/03/24 17:10:06 > > Modified:modules/http http_protocol.c > Log: > What, we don't support HEAD requests now? ;) D'oh!!! I generated the input from the M_FOO symbols. Of course, there isn't an M_HEAD symbol, so... :-) Thanks for fixing this! CHeers, -g -- Greg Stein, http://www.lyra.org/
RE: cvs commit: httpd-2.0/modules/http http_protocol.c
> From: [EMAIL PROTECTED] > [mailto:[EMAIL PROTECTED]]On Behalf Of Jeff Trawick > Sent: 25 March 2002 14:05 > "Sander Striker" <[EMAIL PROTECTED]> writes: > > > > From: [EMAIL PROTECTED] [mailto:[EMAIL PROTECTED]] > > > Sent: 22 March 2002 21:37 > > > > > trawick 02/03/22 12:37:04 > > > > > > Modified:modules/http http_protocol.c > > > Log: > > > add an extra level of parentheses to say "yes I know what I'm > > > doing with that single '='" and more importantly to quiet a > > > gcc -Wall warning > > > > Please put the comment in the code Bit of a bogus suggestion from my side since I prefer not to have long comments when it can be expressed in 4 extra characters. > > or make it explicit that you are > > testing for != 0. The extra braces are bound to be removed by some > > overactive style nitter ;) > > Nope. Style nitters better pay attention to gcc -Wall to avoid doing > anything stupid. True. > (Actually everybody should. I can't believe how > freakin' lazy some people are.) Unfortunately true as well. This also goes for running the test suite. > The only time I feel the need to put a comment in the code to describe > a change to clean up warnings is when an incorrect "foo is used before > set" warning is cleared up by initializing the variable. It is not about cleaning up the warning, which is a good thing. It is about making it more readable. if ((a = b)) is less obvious as if ((a = b) != 0) I was kind of hoping you would be able to detect the joking tone I was trying to use in the last sentence. What I probably should have done is make it explicit that IMO being explicit is the better way to go about this; especially from a review point of view. > Jeff Trawick Sander
Re: cvs commit: httpd-2.0/modules/http http_protocol.c
"Sander Striker" <[EMAIL PROTECTED]> writes: > > From: [EMAIL PROTECTED] [mailto:[EMAIL PROTECTED]] > > Sent: 22 March 2002 21:37 > > > trawick 02/03/22 12:37:04 > > > > Modified:modules/http http_protocol.c > > Log: > > add an extra level of parentheses to say "yes I know what I'm > > doing with that single '='" and more importantly to quiet a > > gcc -Wall warning > > Please put the comment in the code or make it explicit that you are > testing for != 0. The extra braces are bound to be removed by some > overactive style nitter ;) Nope. Style nitters better pay attention to gcc -Wall to avoid doing anything stupid. (Actually everybody should. I can't believe how freakin' lazy some people are.) The only time I feel the need to put a comment in the code to describe a change to clean up warnings is when an incorrect "foo is used before set" warning is cleared up by initializing the variable. -- Jeff Trawick | [EMAIL PROTECTED] Born in Roswell... married an alien...
RE: cvs commit: httpd-2.0/modules/http http_protocol.c
> From: [EMAIL PROTECTED] [mailto:[EMAIL PROTECTED]]
> Sent: 22 March 2002 21:37
> trawick 02/03/22 12:37:04
>
> Modified:modules/http http_protocol.c
> Log:
> add an extra level of parentheses to say "yes I know what I'm
> doing with that single '='" and more importantly to quiet a
> gcc -Wall warning
>
> Revision ChangesPath
> 1.401 +1 -1 httpd-2.0/modules/http/http_protocol.c
>
> Index: http_protocol.c
> ===
> RCS file: /home/cvs/httpd-2.0/modules/http/http_protocol.c,v
> retrieving revision 1.400
> retrieving revision 1.401
> diff -u -r1.400 -r1.401
> --- http_protocol.c 22 Mar 2002 18:34:46 - 1.400
> +++ http_protocol.c 22 Mar 2002 20:37:04 - 1.401
> @@ -1030,7 +1030,7 @@
>
>/* keep a previously set server header (possibly from proxy), otherwise
> * generate a new server header */
> -if (server = apr_table_get(r->headers_out, "Server")) {
> +if ((server = apr_table_get(r->headers_out, "Server"))) {
>form_header_field(&h, "Server", server);
>}
>else {
Please put the comment in the code or make it explicit that you are
testing for != 0. The extra braces are bound to be removed by some
overactive style nitter ;)
Sander
Re: cvs commit: httpd-2.0/modules/http http_protocol.c
Joshua Slive wrote:
> This may be my imagination, but won't this allow any module (or even cgi
> script) to set the Server header and override the default one. Do we want
> this? (I'm undecided, but it is a significant change from previous
> behavior.)
The attached patch fixes this so that the server header may only be
overriden when a request is proxied. Comments?
Regards,
Graham
--
-
[EMAIL PROTECTED]"There's a moon
over Bourbon Street
tonight..."
--- /home/minfrin/src/apache/pristine/apache-1.3/src/main/http_protocol.c Sun
Mar 24 11:59:57 2002
+++ src/main/http_protocol.cSun Mar 24 13:35:00 2002
@@ -1513,7 +1513,6 @@
API_EXPORT(void) ap_basic_http_header(request_rec *r)
{
char *protocol;
-const char *server;
if (r->assbackwards)
return;
@@ -1542,10 +1541,13 @@
/* output the date header */
ap_send_header_field(r, "Date", ap_gm_timestr_822(r->pool, r->request_time));
-/* keep a previously set server header (possible from proxy), otherwise
+/* keep the set-by-proxy server header, otherwise
* generate a new server header */
-if (server = ap_table_get(r->headers_out, "Server")) {
-ap_send_header_field(r, "Server", server);
+if (r->proxyreq) {
+const char *server = ap_table_get(r->headers_out, "Server");
+if (server) {
+ap_send_header_field(r, "Server", server);
+}
}
else {
ap_send_header_field(r, "Server", ap_get_server_version());
smime.p7s
Description: S/MIME Cryptographic Signature
Re: cvs commit: httpd-2.0/modules/http http_protocol.c
Joshua Slive wrote: > This may be my imagination, but won't this allow any module (or even cgi > script) to set the Server header and override the default one. Do we want > this? (I'm undecided, but it is a significant change from previous > behavior.) I will change this to detect whether a proxy is being used (with r->proxyreq) and if so, keep the original header. This will fix the bug, but not change previous behaviour. Regards, Graham -- - [EMAIL PROTECTED]"There's a moon over Bourbon Street tonight..." smime.p7s Description: S/MIME Cryptographic Signature
Re: cvs commit: httpd-2.0/modules/http http_protocol.c
> > minfrin 02/03/22 10:34:46 > > > > Modified:.CHANGES > >modules/http http_protocol.c > > Log: > > When a proxied site was being served, Apache was replacing > > the original site Server header with it's own, which is not > > allowed by RFC2616. Fixed. > > This may be my imagination, but won't this allow any module (or even cgi > script) to set the Server header and override the default one. Do we want > this? (I'm undecided, but it is a significant change from previous > behavior.) Agree, it should be an option at least. There are certain instances where you may want to prevent the original server header from being exposed, to avoid information leaking. For example if you are load balancing IIS servers or specific application server versions. Daniel
Re: cvs commit: httpd-2.0/modules/http http_protocol.c
On 22 Mar 2002 [EMAIL PROTECTED] wrote: > minfrin 02/03/22 10:34:46 > > Modified:.CHANGES >modules/http http_protocol.c > Log: > When a proxied site was being served, Apache was replacing > the original site Server header with it's own, which is not > allowed by RFC2616. Fixed. This may be my imagination, but won't this allow any module (or even cgi script) to set the Server header and override the default one. Do we want this? (I'm undecided, but it is a significant change from previous behavior.) Joshua.
Re: cvs commit: httpd-2.0/modules/http http_protocol.c
[EMAIL PROTECTED] writes: > jerenkrantz02/02/26 19:55:31 > > Modified:.CHANGES STATUS >modules/http http_protocol.c > Log: > Don't set bytes_sent to be 0 when r->assbackwards since this screws up > logging. kicking ass! -- Jeff Trawick | [EMAIL PROTECTED] | PGP public key at web site: http://www.geocities.com/SiliconValley/Park/9289/ Born in Roswell... married an alien...
Re: cvs commit: httpd-2.0/modules/http http_protocol.c
On Sun, Aug 26, 2001 at 05:20:40PM -0700, Greg Stein wrote:
> On Sun, Aug 26, 2001 at 03:18:41PM -0700, Ryan Bloom wrote:
> > On Sunday 26 August 2001 12:54, Doug MacEachern wrote:
> > > On Sun, 26 Aug 2001, Marc Slemko wrote:
> > > > hang on, is this about keepalives or chunked encoding?
> > >
> > > both.
> > >
> > > the check always fails because ap_content_length_filter has set content
> > > length before ap_set_keepalive is called. the right fix would probably be
> > > to check http/1.1-oneness eariler and remove (or not add) the
> > > ap_content_length_filter if r->chunked.
> >
> > Can't do that. The content-length filter always computes the full content length,
> > even if it isn't put in the response. That way, we can log it correctly.
>
> I'd prefer to log it as "0" or somesuch, rather than buffer the response
> just for the sake of logging.
>
> Not of a particular mind here, but it *does* seem expensive to buffer just
> for logging's sake.
>
> Thoughts?
I'd like to see ap_read_request and ap_set_sub_req_protocol set
r->clength to -1 or AP_CONTENT_LENGTH_UNSET and use that to
distinguish an empty response from one that hasn't had its content
length set.
I'm screwing around with some packaging filters which end up computing
the content length while they're at it. Unless I remove
ap_content_length_filter from the filter chain, it re-walks the
brigade. I'm a little leary of removing it by name ("CONTENT-LENGTH")
as it's not a constant and may change from version to version. I'd
like to just leave it alone, set the clength, and count on it to
if (f->r->clength != AP_CONTENT_LENGTH_UNSET)
return ap_pass_brigade(f->next, b)
Reasonable? Crazy?
--
-eric
([EMAIL PROTECTED])
Feel free to forward this message to any list for any purpose other than
email address distribution.
Re: cvs commit: httpd-2.0/modules/http http_protocol.c
On Sunday 26 August 2001 17:20, Greg Stein wrote: > On Sun, Aug 26, 2001 at 03:18:41PM -0700, Ryan Bloom wrote: > > On Sunday 26 August 2001 12:54, Doug MacEachern wrote: > > > On Sun, 26 Aug 2001, Marc Slemko wrote: > > > > hang on, is this about keepalives or chunked encoding? > > > > > > both. > > > > > > the check always fails because ap_content_length_filter has set content > > > length before ap_set_keepalive is called. the right fix would probably > > > be to check http/1.1-oneness eariler and remove (or not add) the > > > ap_content_length_filter if r->chunked. > > > > Can't do that. The content-length filter always computes the full > > content length, even if it isn't put in the response. That way, we can > > log it correctly. > > I'd prefer to log it as "0" or somesuch, rather than buffer the response > just for the sake of logging. > > Not of a particular mind here, but it *does* seem expensive to buffer just > for logging's sake. I wasn't clear. We don't buffer, we just keep on counting. Ryan __ Ryan Bloom [EMAIL PROTECTED] Covalent Technologies [EMAIL PROTECTED] --
Re: cvs commit: httpd-2.0/modules/http http_protocol.c
On Sunday 26 August 2001 12:54, Doug MacEachern wrote: > On Sun, 26 Aug 2001, Marc Slemko wrote: > > hang on, is this about keepalives or chunked encoding? > > both. > > the check always fails because ap_content_length_filter has set content > length before ap_set_keepalive is called. the right fix would probably be > to check http/1.1-oneness eariler and remove (or not add) the > ap_content_length_filter if r->chunked. Can't do that. The content-length filter always computes the full content length, even if it isn't put in the response. That way, we can log it correctly. Ryan __ Ryan Bloom [EMAIL PROTECTED] Covalent Technologies [EMAIL PROTECTED] --
Re: cvs commit: httpd-2.0/modules/http http_protocol.c
you're right, keepalives were working fine without the change. and the chunked stuff was misunderstanding on my part. sorry for the confusion.
Re: cvs commit: httpd-2.0/modules/http http_protocol.c
On Sun, 26 Aug 2001, Doug MacEachern wrote: > On Sun, 26 Aug 2001, Marc Slemko wrote: > > > hang on, is this about keepalives or chunked encoding? > > both. > > the check always fails because ap_content_length_filter has set content > length before ap_set_keepalive is called. the right fix would probably be > to check http/1.1-oneness eariler and remove (or not add) the > ap_content_length_filter if r->chunked. keepalives work just fine for me without the patch. chunked encoding works just fine for me without the patch. Some things that did use chunked encoding in 1.3 will now have a content length instead in 2.0, but that isn't a bug, it is a feature. ap_content_length_filter does _NOT_ always add a content length, so it can't be causing chunking to "never" be used. sure, sometimes a content length is being set when it may be cheaper to use chunked encoding, but that is an optimization not a functionality problem... and this change doesn't do anything to make that optimization, it just ignores the fact that we have no need to chunk and chunks anyway. There is nothing that says you need to use chunked encoding in HTTP/1.1 responses if you have a content length. It is completely wrong to be chunking things "just because" even though we have a content length.
Re: cvs commit: httpd-2.0/modules/http http_protocol.c
On Sun, 26 Aug 2001, Marc Slemko wrote: > hang on, is this about keepalives or chunked encoding? both. the check always fails because ap_content_length_filter has set content length before ap_set_keepalive is called. the right fix would probably be to check http/1.1-oneness eariler and remove (or not add) the ap_content_length_filter if r->chunked.
Re: cvs commit: httpd-2.0/modules/http http_protocol.c
On 26 Aug 2001 [EMAIL PROTECTED] wrote:
> dougm 01/08/26 11:57:16
>
> Modified:modules/http http_protocol.c
> Log:
> ap_content_length_filter has already set Content-Length
> before ap_set_keepalive is called. need to remove this check
> in order for keepalives to work.
hang on, is this about keepalives or chunked encoding?
Exactly what situation doesn't work the way you think it should without
this patch?
>
> Revision ChangesPath
> 1.359 +7 -1 httpd-2.0/modules/http/http_protocol.c
>
> Index: http_protocol.c
> ===
> RCS file: /home/cvs/httpd-2.0/modules/http/http_protocol.c,v
> retrieving revision 1.358
> retrieving revision 1.359
> diff -u -r1.358 -r1.359
> --- http_protocol.c 2001/08/26 04:59:37 1.358
> +++ http_protocol.c 2001/08/26 18:57:16 1.359
> @@ -136,7 +136,14 @@
> && ((r->status == HTTP_NOT_MODIFIED)
> || (r->status == HTTP_NO_CONTENT)
> || r->header_only
> +#if 0
> +/* this was right in 1.x, but in 2.x
> + * ap_content_length_filter has already set Content-Length
> + * before this function is called
> + * XXX: should there be a different check in place of this?
> + */
> || apr_table_get(r->headers_out, "Content-Length")
> +#endif
> || ap_find_last_token(r->pool,
> apr_table_get(r->headers_out,
> "Transfer-Encoding"),
> @@ -1234,7 +1241,6 @@
>if (r->chunked) {
>apr_table_mergen(r->headers_out, "Transfer-Encoding", "chunked");
>apr_table_unset(r->headers_out, "Content-Length");
> -
>}
>
>apr_table_setn(r->headers_out, "Content-Type", ap_make_content_type(r,
>
>
>
>
Re: cvs commit: httpd-2.0/modules/http http_protocol.c
If someone's feeling ambitious, they could add more tests for this sort of
thing... with bucket brigades being as they are, many of the cases that
could fail do so with the "odd" bucket types like pipe and socket.
It just occurred to me that by the time we arrive at this particular
block, the byterange filter should have normalized all such buckets
because it calls apr_brigade_length(), which reads in all buckets of
indeterminate length. So this block is probably never reached, but it's
worthwhile to have it right anyway.
Anyhow, it's entirely possible that there are other places in the code
that ARE reached and that break when they get pipe and socket buckets...
--Cliff
On 24 Aug 2001 [EMAIL PROTECTED] wrote:
> jwoolley01/08/24 13:27:40
>
> Modified:modules/http http_protocol.c
> Log:
> Fix a double-free condition when byterange requests are made on brigades
> containing any bucket that cannot be copied natively (ie, pipe or socket
> buckets).
>
> Before, we were reading that bucket to morph it to a heap bucket and then
> taking the str that heap bucket points to and placing it in a second,
> completely separate heap bucket. That means we'd have two apr_bucket/
> apr_bucket_heap pairs each with a refcount of 1 (rather than two apr_buckets
> and a single apr_bucket_heap with a refcount of 2). str would then be
> doubly-freed when the second of those two buckets was destroyed.
>
> Revision ChangesPath
> 1.355 +6 -1 httpd-2.0/modules/http/http_protocol.c
>
> Index: http_protocol.c
> ===
> RCS file: /home/cvs/httpd-2.0/modules/http/http_protocol.c,v
> retrieving revision 1.354
> retrieving revision 1.355
> diff -u -d -u -r1.354 -r1.355
> --- http_protocol.c 2001/08/16 03:58:16 1.354
> +++ http_protocol.c 2001/08/24 20:27:40 1.355
> @@ -2468,8 +2468,13 @@
>apr_size_t len;
>
>if (apr_bucket_copy(ec, &foo) != APR_SUCCESS) {
> +/* we assume here that if copy failed we can morph
> + * the bucket into a copyable one by reading it... normally
> + * copy won't return anything but APR_SUCCESS or APR_ENOTIMPL
> + */
> +/* XXX: check for failure? */
>apr_bucket_read(ec, &str, &len, APR_BLOCK_READ);
> -foo = apr_bucket_heap_create(str, len, 0, NULL);
> +apr_bucket_copy(ec, &foo);
>}
>APR_BRIGADE_INSERT_TAIL(bsend, foo);
>ec = APR_BUCKET_NEXT(ec);
>
>
>
>
--
Cliff Woolley
[EMAIL PROTECTED]
Charlottesville, VA
Re: cvs commit: httpd-2.0/modules/http http_protocol.c
On Fri, Aug 24, 2001 at 04:39:32PM -0400, Cliff Woolley wrote: >... > It just occurred to me that by the time we arrive at this particular > block, the byterange filter should have normalized all such buckets > because it calls apr_brigade_length(), which reads in all buckets of > indeterminate length. So this block is probably never reached, but it's > worthwhile to have it right anyway. It should be right. It would be very easy for a bucket to be based on a pipe or a socket, thus being uncopyable, but it knows the length of data to be read from that pipe/socket. Imagine if you have a particular protocol running over some socket to a backend server. You know the next chunk of data is 1000 bytes. You insert your custom socket bucket, saying " socket, 1000 bytes". It can be read, but it can't be copied. Cheers, -g -- Greg Stein, http://www.lyra.org/
