mod_include strangeness
Seems that mod_include can pass empty bucket brigades down the filter chain. For instance if the first line in an SSI is !--#include... then SPLIT_AND_PASS_PRETAG_BUCKETS will pass an empty brigade as the first brigade. This causes problems with the logic in mod_deflate which attempts to bypass deflate when a response has zero body length (e.g. proxied 304's) What happens is that this empty brigade gets passed down the filter chain and ap_http_header_filter is called before the Content-Encoding header has been set (mod_deflate is holding off setting this until it knows it has some content). As a defensive move, mod_deflate could just check for APR_BRIGADE_EMPTY and immediately return APR_SUCCESS. Or we could put checks in mod_include to prevent passing empty buckets in the first place (I think there was such a check at one time but were taken out for performance improving reasons?). The latter might avoid problems with other modules but being defensive is probably a good idea also. Comments? Allan
Re: mod_include strangeness
Justin Erenkrantz wrote: You should really take a look at the trunk. =) This has been fixed for many months. -- justin Ah, I see it now! Thanks for the quick feedback Justin. Allan
Re: 2.2 roadmap with respect to APR was Re: [NOTICE] CVS to SVN migration complete
man, how did I get so far behind on my email... I'd like to see us get this into httpd 2.2 for the reasons previously outlined and think we need to get the work underway as quickly as possible to determine how extensive the changes are going to be and how fast progress can be made. First order of business now that we are on SVN is to focus on the APR changes that are needed. It's not clear to me though, now that we have an APR 1.0 branch, is the trunk open for API-breaking changes or do we need a separate branch for that work? If we can make good progress towards a stable 64 bit APR 2.0 then moving httpd 2.1/2.2 to it could make sense. The question is whether there is enough feature freeze pressure to say that 64 bit does not warrant the wait... I'd say let's see if we can make some progress first. Any help that can be offered on this endeavour will be greatly appreciated! Allan Bill Stoddard wrote: William A. Rowe, Jr. wrote: At 08:23 AM 11/20/2004, Jim Jagielski wrote: On Nov 20, 2004, at 12:03 AM, Justin Erenkrantz wrote: So, my opinion is that we let Allen branch apr off now and let him go at it at a measured pace, but we shouldn't intend to hold httpd 2.2 for that. -- justin +1. Of course, I am assuming that his 64bit fixes will likely break binary compatibility. It does - that's the rub. And, for 2.2, this was always the plan. And that's precisely the reason we should attack the 64 bit problem for 2.2. This will give the 2.2 series a much longer life than if we push off the 64 bit work to 2.4. Bill
Re: 2.2 roadmap with respect to APR was Re: [NOTICE] CVS to SVN migration complete
William A. Rowe, Jr. wrote: At 01:29 PM 11/24/2004, Cliff Woolley wrote: On Wed, 24 Nov 2004, Justin Erenkrantz wrote: I'm sick of all talk and no action. We tried this last year when we were almost ready to branch APR 1.0 and all action on that front ceased entirely for a YEAR. This time it's one or the other. I'll wait 24 hours at most to hear opinions. Whichever route gets the most support wins. So far we have: trunk remains 1.1, 64-bit is sandboxed: jwoolley, jerenkrantz wrowe: (conditional on committing to head once it's reviewed, and branch 1.1 if we want to keep 1.1 alive.) That is fine by me. I agree that the extent of the changes will probably be significant so at least starting with a sandbox branch may be judicious, at least until we get a feel for the extent of the changes, and working on a branch may actually help accelerate the work. Allan
Re: cvs commit: httpd-2.0/server core.c protocol.c request.c scoreboard.c util.c util_script.c
Brad Nicholes wrote: -1 as well. This is now causing compiler errors on NetWare. Please revert this patch! Can you provide an indication of exactly what broke so we will know what to avoid in future. Or was the breakage actually due to the the mod_cache problem reported last night? Thanks, Allan
Re: cvs commit: httpd-2.0/server core.c protocol.c request.c scoreboard.c util.c util_script.c
Roy T. Fielding wrote: whoa! -1 Was this even discussed on the list? You just changed the entire module API and introduced a dozen potential security holes. The precursor to this patch [PATCH] WIN64: httpd API changes was posted 10/7 so I thought we had had suitable time for discussion. I have addressed the one issue that was raised. There have also been several other threads on the httpd apr lists and the feedback I had received indicated the it was appropriate to sanitize the 64 bit compile even if it incurred httpd API changes. However if there are specific security issues that this has brought up I am more than anxious to address them. Are you opposed to changing the API to fix 64 bit warnings or are there specific issues that I can address and continue to move forward rather that back out the entire patch? Why on earth is it changing nvec to apr_size_t and then downcasting Fixing some of the warnings (below) without resorting to casts ripples through some API's, but changing APR API's at this point is not possible so I had to stop there, with the implication that we have to complete this in APR 2.0. If exceeding 2Gig elements is the security hole you want plugging I can add code to check for that and error out. its use? Why is any of this even needed? These are the 64bit compile warnings I am addressing with this patch .\server\core.c(3959) : warning C4018: '' : signed/unsigned mismatch .\server\core.c(4291) : warning C4267: '=' : conversion from 'size_t' to 'int', possible loss of data .\server\core.c(4296) : warning C4267: '=' : conversion from 'size_t' to 'int', possible loss of data .\server\core.c(4336) : warning C4267: 'function' : conversion from 'size_t' to 'int', possible loss of data .\modules\http\http_protocol.c(665) : warning C4267: 'initializing' : conversion from 'size_t' to 'int', possible loss of data .\modules\http\http_protocol.c(1922) : warning C4267: 'return' : conversion from 'size_t' to 'long', possible loss of data .\server\protocol.c(1400) : warning C4267: 'return' : conversion from 'size_t' to 'int', possible loss of data .\server\protocol.c(1464) : warning C4244: 'initializing' : conversion from '__int64' to 'int', possible loss of data .\server\protocol.c(1473) : warning C4267: 'return' : conversion from 'size_t' to 'int', possible loss of data .\server\protocol.c(1520) : warning C4267: 'return' : conversion from 'size_t' to 'int', possible loss of data .\server\request.c(1231) : warning C4267: '=' : conversion from 'size_t' to 'int', possible loss of data .\server\util.c(461) : warning C4244: '=' : conversion from '__int64' to 'int', possible loss of data .\server\util.c(600) : warning C4244: '=' : conversion from '__int64' to 'int', possible loss of data .\server\util.c(633) : warning C4244: '=' : conversion from '__int64' to 'int', possible loss of data .\server\util.c(663) : warning C4244: '=' : conversion from '__int64' to 'int', possible loss of data .\server\util.c(758) : warning C4244: 'function' : conversion from '__int64' to 'int', possible loss of data .\server\util.c(768) : warning C4244: 'function' : conversion from '__int64' to 'int', possible loss of data .\server\util.c(894) : warning C4267: 'function' : conversion from 'size_t' to 'int', possible loss of data .\server\util.c(1195) : warning C4244: '=' : conversion from '__int64' to 'int', possible loss of data .\server\util.c(1435) : warning C4244: '=' : conversion from '__int64' to 'int', possible loss of data .\server\util.c(1492) : warning C4267: '=' : conversion from 'size_t' to 'int', possible loss of data .\server\util.c(1493) : warning C4267: '=' : conversion from 'size_t' to 'int', possible loss of data .\server\util.c(1978) : warning C4244: 'return' : conversion from '__int64' to 'int', possible loss of data .\server\util.c(1987) : warning C4244: 'return' : conversion from '__int64' to 'int', possible loss of data .\server\util.c(2082) : warning C4267: 'initializing' : conversion from 'size_t' to 'int', possible loss of data .\server\util_script.c(288) : warning C4267: 'initializing' : conversion from 'size_t' to 'int', possible loss of data .\server\util_script.c(289) : warning C4267: 'initializing' : conversion from 'size_t' to 'int', possible loss of data .\server\util_script.c(435) : warning C4267: '=' : conversion from 'size_t' to 'int', possible loss of data .\server\util_script.c(666) : warning C4244: '=' : conversion from '__int64' to 'int', possible loss of data .\server\scoreboard.c(109) : warning C4267: 'return' : conversion from 'size_t' to 'int', possible loss of data Allan
Re: [PATCH] WIN64: httpd API changes
Joe Orton wrote: The ap_r* changes are not safe: these functions return negative values on error. Each and every int-size_t conversion needs to be carefully checked for this kind of issue. Good point. I will review closely this weekend. Thanks, Allan
Re: cvs commit: httpd-2.0/support/win32 ApacheMonitor.c
Mladen Turk wrote: [snip] Both apr and libhttpd has more then 100 of those when /Wp64 is used, so IMO we would just hide the problems under carpet if just casting every 64-32 warning found. Agreed, hiding underlying problems is not acceptable. I am trying to address the many warnings that a Win64 build spits out in the correct way, not just for the sake of suppressing warnings. For example inside http_protocol.c you have: (i.e. in the current http_protocol.c code) 'int len = strlen(method)', that is wrong by all means, but I wouldn't write that as 'int len = (int)strlen(method)' just to make the compiler happy, but rather use 'size_t len = strlen(method)'. Agreed. In the patch just posted you'll see mod_win32.c takes the approach you suggest, as does a patch I have been working on that addresses http_protocol.c and other warnings. I will be posting this shortly. Well, that one is probably OK. I was talking about the concept of not casting just for the sake of making compiler happy. Agree. Thanks, Allan
[PATCH] WIN64: httpd API changes
This set of changes gets rid of most of the libhttpd 64bit warnings on Windows at the cost of several httpd API changes. I defined AP_INT_TRUNC_CAST for use where there are size mismatches with APR API's (those APR API's will need to be updated in APR 2.0) Comments before I commit? Allan - Index: include/ap_mmn.h === RCS file: /home/cvs/httpd-2.0/include/ap_mmn.h,v retrieving revision 1.69 diff -U3 -r1.69 ap_mmn.h --- include/ap_mmn.h4 Jun 2004 22:40:46 - 1.69 +++ include/ap_mmn.h7 Oct 2004 20:48:55 - @@ -84,14 +84,15 @@ * changed ap_add_module, ap_add_loaded_module, * ap_setup_prelinked_modules, ap_process_resource_config * 20040425.1 (2.1.0-dev) Added ap_module_symbol_t and ap_prelinked_module_symbols + * 20041007 (2.1.0-dev) API changes to clean up 64bit compiles */ #define MODULE_MAGIC_COOKIE 0x41503230UL /* AP20 */ #ifndef MODULE_MAGIC_NUMBER_MAJOR -#define MODULE_MAGIC_NUMBER_MAJOR 20040425 +#define MODULE_MAGIC_NUMBER_MAJOR 20041007 #endif -#define MODULE_MAGIC_NUMBER_MINOR 1 /* 0...n */ +#define MODULE_MAGIC_NUMBER_MINOR 0 /* 0...n */ /** * Determine if the server's current MODULE_MAGIC_NUMBER is at least a Index: include/http_protocol.h === RCS file: /home/cvs/httpd-2.0/include/http_protocol.h,v retrieving revision 1.92 diff -U3 -r1.92 http_protocol.h --- include/http_protocol.h 18 Jul 2004 20:06:38 - 1.92 +++ include/http_protocol.h 7 Oct 2004 20:48:55 - @@ -340,7 +340,7 @@ * @return The number of bytes sent * @deffunc int ap_rputs(const char *str, request_rec *r) */ -AP_DECLARE(int) ap_rputs(const char *str, request_rec *r); +AP_DECLARE(apr_size_t) ap_rputs(const char *str, request_rec *r); /** * Write a buffer for the current request @@ -359,7 +359,7 @@ * @return The number of bytes sent * @deffunc int ap_rvputs(request_rec *r, ...) */ -AP_DECLARE_NONSTD(int) ap_rvputs(request_rec *r,...); +AP_DECLARE_NONSTD(apr_size_t) ap_rvputs(request_rec *r,...); /** * Output data to the client in a printf format @@ -369,7 +369,7 @@ * @return The number of bytes sent * @deffunc int ap_vrprintf(request_rec *r, const char *fmt, va_list vlist) */ -AP_DECLARE(int) ap_vrprintf(request_rec *r, const char *fmt, va_list vlist); +AP_DECLARE(apr_size_t) ap_vrprintf(request_rec *r, const char *fmt, va_list vlist); /** * Output data to the client in a printf format @@ -379,7 +379,7 @@ * @return The number of bytes sent * @deffunc int ap_rprintf(request_rec *r, const char *fmt, ...) */ -AP_DECLARE_NONSTD(int) ap_rprintf(request_rec *r, const char *fmt,...) +AP_DECLARE_NONSTD(apr_size_t) ap_rprintf(request_rec *r, const char *fmt,...) __attribute__((format(printf,2,3))); /** * Flush all of the data for the current request to the client @@ -445,7 +445,7 @@ * if EOF, or -1 if there was an error * @deffunc long ap_get_client_block(request_rec *r, char *buffer, apr_size_t bufsiz) */ -AP_DECLARE(long) ap_get_client_block(request_rec *r, char *buffer, apr_size_t bufsiz); +AP_DECLARE(apr_size_t) ap_get_client_block(request_rec *r, char *buffer, apr_size_t bufsiz); /** * In HTTP/1.1, any method can have a body. However, most GET handlers Index: include/httpd.h === RCS file: /home/cvs/httpd-2.0/include/httpd.h,v retrieving revision 1.212 diff -U3 -r1.212 httpd.h --- include/httpd.h 12 Aug 2004 05:22:59 - 1.212 +++ include/httpd.h 7 Oct 2004 20:48:55 - @@ -1091,7 +1091,7 @@ /** Pathname for ServerPath */ const char *path; /** Length of path */ -int pathlen; +apr_size_t pathlen; /** Normal names for ServerAlias servers */ apr_array_header_t *names; @@ -1244,7 +1244,7 @@ * address of field is shifted to the next non-comma, non-whitespace * character. len is the length of the item excluding any beginning whitespace. */ -AP_DECLARE(const char *) ap_size_list_item(const char **field, int *len); +AP_DECLARE(const char *) ap_size_list_item(const char **field, apr_size_t *len); /** * Retrieve an HTTP header field list item, as separated by a comma, @@ -1587,7 +1587,7 @@ * @param c The character to search for * @return The index of the first occurrence of c in str */ -AP_DECLARE(int) ap_ind(const char *str, char c); /* Sigh... */ +AP_DECLARE(apr_size_t) ap_ind(const char *str, char c);/* Sigh... */ /** * Search a string from right to left for the first occurrence of a @@ -1596,7 +1596,7 @@ * @param c The character to search for * @return The index of the first occurrence of c in str */ -AP_DECLARE(int) ap_rind(const char *str, char c); +AP_DECLARE(apr_size_t) ap_rind(const char *str, char c); /** * Given a string, replace any bare
Re: cvs commit: httpd-2.0/server/mpm/winnt child.c
Jeff Trawick wrote: possible heresy The first parameter to select() on Windows is actually ignored. We should zap the logic that does the Unix-style computation of listenmaxfd and just pass in INVALID_SOCKET or 0, and add a comment that the value is ignored. /possible heresy No heresy, MSDN doc clearly says the first parm is ignored so it doesn't matter what we set it to. For clarification I can add a comment and set it to zero. Thanks, Allan
Re: cvs commit: httpd-2.0/support/win32 ApacheMonitor.c
Mladen Turk wrote: That's true, but the strlen can never be int (negative). The API is DWORD meaning 32 bit unsigned integer, so either cast to API or no cast. You are correct that for WriteConsole the cast should have been DWORD - I will fix that, thanks For TextOut and lstrcpyn the parameter is in fact int so we either have to cast to int and assmume the string length will never by 2Gig (seems reasonable), or we need to explicitly check for strlen 2Gig and fail if it is, or use a different API (if there is one). What do you suggest? Allan
Re: cvs commit: apr/network_io/win32 sendrecv.c
Joe Orton wrote: On Wed, Sep 22, 2004 at 06:21:31PM -, [EMAIL PROTECTED] wrote: --- core.c 20 Sep 2004 20:12:20 - 1.286 +++ core.c 22 Sep 2004 18:21:29 - 1.287 @@ -2298,7 +2298,7 @@ -cmd-server-pathlen = strlen(arg); +cmd-server-pathlen = (int)strlen(arg); It would surely be better to make pathlen an apr_size_t than add casts like this. Did you try that? This question is going to come up again so it's good to get agreement before I make many more commits. The IA64 Windows httpd build spits out a large number of warnings, over 500, and a fair number of these come from int/size_t mismatches. Apparently other 64 bit compilers aren't as picky as the MS compiler, since I haven't seen anyone else complaining about this. They seem to reasonably assume that we aren't going to have strings 4Gig long. The general approach I have been taking is changing locally declared int's to apr_size_t where feasible but I have avoided changing int's to apr_size_t in public structures. Changing public structures may ripple through other code and brings up concerns about MMN bumps. In this case we would incur a major bump I believe. Unless a particular field is resulting in warnings in multiple parts of the code I'm generally going to opt for the cast. Sound reasonable? BTW, if you commit stuff to both httpd and apr at the same time the CVS commit notifications don't go to all the right places, so it's best to do each separately. I noticed that after I committed, next time I'll split the commit. Thanks, Allan
Re: cvs commit: apr/network_io/win32 sendrecv.c
William A. Rowe, Jr. wrote: At 02:37 PM 9/23/2004, Allan Edwards wrote: aren't as picky as the MS compiler, since I haven't seen anyone else complaining about this. They seem to reasonably assume that we aren't going to have strings 4Gig long. However, they have qword ints and qword size_t's. For Linux IA64 int is 4 bytes, size_t is 8 - no? There are no concerns with MMN bumps until we consider backporting this support to 2.0. Because IA64/win64 support is new, please don't dwell on httpd-2.0, but let's focus on making 2.1-dev typesafe. OK, will revisit from that standpoint If we have a structure member change in APR 1.0 that's a little bit more serious, we can discuss it when it comes up. Thanks, Allan
Re: DWORD_MAX
NormW wrote:
Compiling network_io/win32/sendrecv.c
### mwccnlm Compiler:
#File: network_io\win32\sendrecv.c
#
# 110: while (cur_len DWORD_MAX) {
# Error: ^
# undefined identifier 'DWORD_MAX'
That would be my recent update and I assume it
means that the definition for DWORD_MAX needs to
be copied from apr.hw to apr.hnw. Could someone from
the NW side please confirm that is the correct thing
to do I will update it.
Thanks, Allan
Re: Windows IA64 builds
-AP_DECLARE(long) ap_get_client_block(request_rec *r, char *buffer, apr_size_t bufsiz); +AP_DECLARE(apr_ssize_t) ap_get_client_block(request_rec *r, char *buffer, apr_size_t bufsiz); Don't know why long was used here, but it causes a warning for Windows IA64 build since long is 32 bits. I'm wondering however if there are any other 64 bit platforms that also have 32 bit longs that have ignored this warning. Hopefully not and therefore this would be safe for backport to 2.0. Anyone have information to the contrary? Allan
Re: Windows 2003 IA64 builds?
I posted a companion patch to the apr list last night.
This patch against HEAD along with the apr patch get
the three core dlls cross compiling with Visual Studio 6
+ IA64 SDK. It addresses most /W2 warnings and fixes a
couple of bugs. I haven't looked at many of the modules
yet.
As I mentioned in the other note there are a lot more
warnings that are thrown with /W3 that need to be
investigated furher.
In addition to fixing compiler warnings there is the
question of the build process. I'm currently running
setenv /SRV64 /RETAIL from the SDK to set the IA64
environment, then running msdev with the /USEENV option.
This requires a separate set of .dsp files that have
been munged to specify /machine:IA64 among other things.
Keeping the .dsp's separate rather than folding into
the existing ones if for convenience and maintainability
at least in the short term, maybe the long term too.
One problem that comes up is that there are 3 executables
built that are used to generate tables at compile time, but
since everything is cross compiled IA64 that doesn't work.
I'll have to hack around that somehow, maybe store the prebuilt
files in CVS and copy to the appropriate locations.
I'm open to suggestions if anyone has a better idea.
If this sounds reasonable I'll proceed along this path.
Allan
--
Index: os/win32/ap_regkey.c
===
RCS file: /home/cvs/httpd-2.0/os/win32/ap_regkey.c,v
retrieving revision 1.11
diff -U3 -r1.11 ap_regkey.c
--- os/win32/ap_regkey.c9 Feb 2004 20:40:49 - 1.11
+++ os/win32/ap_regkey.c17 Aug 2004 21:02:48 -
@@ -236,7 +236,7 @@
*/
valuelen = (size - 1) * 3 + 1;
*result = apr_palloc(pool, valuelen);
-rv = apr_conv_ucs2_to_utf8(wvalue, size, *result, valuelen);
+rv = apr_conv_ucs2_to_utf8(wvalue, (apr_size_t *)size, *result, valuelen);
if (rv != APR_SUCCESS)
return rv;
else if (size)
@@ -309,7 +309,7 @@
wvallen = alloclen = size;
wvalue = apr_palloc(pool, alloclen * 2);
-rv = apr_conv_utf8_to_ucs2(value, size, wvalue, wvallen);
+rv = apr_conv_utf8_to_ucs2(value, (apr_size_t *)size, wvalue, wvallen);
if (rv != APR_SUCCESS)
return rv;
else if (size)
@@ -363,7 +363,7 @@
return APR_ENAMETOOLONG;
/* Read to NULL buffer to determine value size */
rc = RegQueryValueExW(key-hkey, wvalname, 0, resulttype,
- NULL, resultsize);
+ NULL, (LPDWORD)resultsize);
if (rc != ERROR_SUCCESS) {
return APR_FROM_OS_ERROR(rc);
}
@@ -371,7 +371,7 @@
/* Read value based on size query above */
*result = apr_palloc(pool, *resultsize);
rc = RegQueryValueExW(key-hkey, wvalname, 0, resulttype,
- (LPBYTE)*result, resultsize);
+ (LPBYTE)*result, (LPDWORD)resultsize);
}
#endif /* APR_HAS_UNICODE_FS */
#if APR_HAS_ANSI_FS
@@ -379,14 +379,14 @@
{
/* Read to NULL buffer to determine value size */
rc = RegQueryValueEx(key-hkey, valuename, 0, resulttype,
- NULL, resultsize);
+ NULL, (LPDWORD)resultsize);
if (rc != ERROR_SUCCESS)
return APR_FROM_OS_ERROR(rc);
/* Read value based on size query above */
*result = apr_palloc(pool, *resultsize);
rc = RegQueryValueEx(key-hkey, valuename, 0, resulttype,
- (LPBYTE)*result, resultsize);
+ (LPBYTE)*result, (LPDWORD)resultsize);
if (rc != ERROR_SUCCESS)
return APR_FROM_OS_ERROR(rc);
}
@@ -452,7 +452,7 @@
char *buf;
char *tmp;
DWORD type;
-DWORD size = 0;
+apr_size_t size = 0;
rv = ap_regkey_value_raw_get(value, size, type, key, valuename, pool);
if (rv != APR_SUCCESS) {
Index: server/mpm/winnt/child.c
===
RCS file: /home/cvs/httpd-2.0/server/mpm/winnt/child.c,v
retrieving revision 1.37
diff -U3 -r1.37 child.c
--- server/mpm/winnt/child.c14 Aug 2004 10:57:13 - 1.37
+++ server/mpm/winnt/child.c17 Aug 2004 21:02:48 -
@@ -684,7 +684,7 @@
{
int rc;
DWORD BytesRead;
-DWORD CompKey;
+apr_size_t CompKey;
LPOVERLAPPED pol;
mpm_recycle_completion_context(context);
Index: server/mpm/winnt/mpm_winnt.c
===
RCS file: /home/cvs/httpd-2.0/server/mpm/winnt/mpm_winnt.c,v
retrieving revision 1.311
diff -U3 -r1.311 mpm_winnt.c
--- server/mpm/winnt/mpm_winnt.c24 Apr 2004 11:23:14 - 1.311
+++ server/mpm/winnt/mpm_winnt.c17 Aug 2004 21:02:49 -
@@ -416,7 +416,7 @@
HANDLE hDup;
HANDLE
Windows 2003 IA64 builds?
Has anyone attempted to build apr/apache for Windows 2003 Itanium? First attempts to build using 2003 SDK and Visual Studio 6.0 gave a surprising number of type mismatch warnings, about 100 for apr/aprutil and around 80 for libhttpd. Need to comb through these to see how many are real issues, but was wondering if this is pioneering work or has anyone ventured down this path ahead of me? Allan
Re: [PATCH] mod_deflate + mod_proxy bug
Joe Orton wrote: Wouldn't it be simpler to just check for a brigade containing just EOS and do nothing for that case in the first place? Yes I had considered that. The initial patch covered some pathological cases but after having looked over the code some more I think the simpler more efficient way of bailing on just EOS is sufficient. But the fact that the proxy passes such a brigade through the output filters in the first place sounds like the real bug, it doesn't happen for a non-proxied 304 response. Whether or not this is a bug I guess is open for debate. What happens for non proxied error responses is that ap_send_error_response resets r-output_flters to r-proto_output_filters so the deflate filter is taken out of the respone path. In the proxy path there is no such logic for error responses, so error pages would get zipped. But I don't know that this violates the RFC and browsers seem to be able to handle it. Allan
[PATCH] mod_deflate + mod_proxy bug
Running ProxyPass with mod_deflate results in
an extraneous 20 bytes being tacked onto 304
responses from the backend.
The problem is that mod_deflate doesn't handle
the zero byte body, adds the gzip header and
tries to compress 0 bytes.
This patch detects the fact that there was no
data to compress and removes the gzip header
from the bucket brigade.
Any comments before I commit to head?
Allan
--
Index: mod_deflate.c
===
RCS file: /home/cvs/httpd-2.0/modules/filters/mod_deflate.c,v
retrieving revision 1.49
diff -u -d -b -r1.49 mod_deflate.c
--- mod_deflate.c 1 Jun 2004 13:06:10 - 1.49
+++ mod_deflate.c 9 Jun 2004 16:38:30 -
@@ -433,6 +433,8 @@
char *buf;
unsigned int deflate_len;
+if (ctx-stream.total_in != 0) {
+
ctx-stream.avail_in = 0; /* should be zero already anyway */
for (;;) {
deflate_len = c-bufferSize - ctx-stream.avail_out;
@@ -510,6 +512,14 @@
* Time to pass it along down the chain.
*/
return ap_pass_brigade(f-next, ctx-bb);
+}
+else {
+/* this was a zero length response, remove gzip header bucket then
pass down the EOS */
+APR_BUCKET_REMOVE(APR_BRIGADE_FIRST(ctx-bb));
+APR_BUCKET_REMOVE(e);
+APR_BRIGADE_INSERT_TAIL(ctx-bb, e);
+return ap_pass_brigade(f-next, ctx-bb);
+}
}
if (APR_BUCKET_IS_FLUSH(e)) {
Re: [PATCH] mod_deflate + mod_proxy bug
Cliff Woolley wrote: I haven't looked at the entire context of this, but if you remove a bucket (brigade_first(ctx-bb) from a brigade without deleting it and without having any extra pointers to it, you'll leak memory. Thanks for catching that! I'll replace APR_BUCKET_REMOVE with a call to apr_bucket_delete(). Also just realized I need to add a call to deflateEnd(). Also, what happens if e *is* the first bucket in the brigade? Can that occur? I think that by coincidence given the implementation of APR_BUCKET_REMOVE, nothing bad would happen by double-removing a given bucket twice in a row, but in general that seems like a bad idea and should be avoided. e is on the brigade passed into deflate_out_filter and the gzip header bucket is in ctx-bb so that is not a problem. Thanks, Allan
Re: cvs commit: httpd-2.0 libhttpd.dsp
William A. Rowe, Jr. wrote: uh wrong. with /debug incremental yes is the default but you have to pound it into the msdev's head. please fix/revert. -# ... /dll /incremental:no /debug /machine:I386 /base:@os\win32\BaseAddr.ref,libhttpd.dll /opt:ref +# ... /dll /debug /machine:I386 /base:@os\win32\BaseAddr.ref,libhttpd.dll /opt:ref Looks like MSDEV fooness to me. I changed nothing in the project except adding the eoc file but I can't coax MSDEV into including /incremental:no in the dsp even though it *is* there in the Link Project Options box. Since I don't want to manually edit the generated file the only thing I can do is revert the checkin and let someone else (Bill?) add the eoc file back in. Allan
Re: [PATCH] Windows IPv6
William A. Rowe, Jr. wrote:
+1, but which warning does 4163 quiet?
It quiets C4163: '_rotl64' : not available as an intrinsic
function' (see below) that gets generated as a result of
including the platform SDK. Picking up the platform SDK also
results in a couple of other warnings but this was the
major offendor.
Allan
At 09:46 AM 2/26/2004, you wrote:
Here's a patch to enable IPv6 on Windows XP 2003.
In addition we'll need to change the setting of
APR_HAVE_IPV6 in apr.hw - seems like we'll need
some awk magic to do that.
Note that enabling IPv6 drags in the need for
the XP or 2003 platform SDK but I don't see
any way around it. I believe the platform SDK can
be freely downloaded from MS for those who want to
do an IPv6 build.
Any comments before I commit to 2.1?
Allan
Index: server/mpm/winnt/child.c
===
RCS file: /home/cvs/httpd-2.0/server/mpm/winnt/child.c,v
retrieving revision 1.26
diff -u -d -b -r1.26 child.c
--- server/mpm/winnt/child.c9 Feb 2004 20:40:51 -1.26
+++ server/mpm/winnt/child.c25 Feb 2004 16:20:51 -
@@ -314,13 +314,17 @@
int wait_time = 1;
int csd;
SOCKET nsd = INVALID_SOCKET;
-struct sockaddr_in sa_client;
int count_select_errors = 0;
int rc;
int clen;
ap_listen_rec *lr;
struct fd_set listenfds;
SOCKET listenmaxfd = INVALID_SOCKET;
+#if APR_HAVE_IPV6
+struct sockaddr_in6 sa_client;
+#else
+struct sockaddr_in sa_client;
+#endif
/* Setup the listeners
* ToDo: Use apr_poll()
@@ -395,7 +399,13 @@
static PCOMP_CONTEXT win9x_get_connection(PCOMP_CONTEXT context)
{
apr_os_sock_info_t sockinfo;
-int len;
+int len, salen;
+#if APR_HAVE_IPV6
+salen = sizeof(struct sockaddr_in6);
+#else
+salen = sizeof(struct sockaddr_in);
+#endif
+
if (context == NULL) {
/* allocate the completion context and the transaction pool */
@@ -415,7 +425,7 @@
if (context-accept_socket == INVALID_SOCKET) {
return NULL;
}
-len = sizeof(struct sockaddr);
+len = salen;
context-sa_server = apr_palloc(context-ptrans, len);
if (getsockname(context-accept_socket,
context-sa_server, len)== SOCKET_ERROR) {
@@ -423,7 +433,7 @@
getsockname failed);
continue;
}
-len = sizeof(struct sockaddr);
+len = salen;
context-sa_client = apr_palloc(context-ptrans, len);
if ((getpeername(context-accept_socket,
context-sa_client, len)) == SOCKET_ERROR) {
@@ -434,7 +444,7 @@
sockinfo.os_sock = context-accept_socket;
sockinfo.local = context-sa_server;
sockinfo.remote = context-sa_client;
-sockinfo.family = APR_INET;
+sockinfo.family = context-sa_server-sa_family;
sockinfo.type= SOCK_STREAM;
apr_os_sock_make(context-sock, sockinfo, context-ptrans);
@@ -465,9 +475,21 @@
DWORD BytesRead;
SOCKET nlsd;
int rv, err_count = 0;
+#if APR_HAVE_IPV6
+SOCKADDR_STORAGE ss_listen;
+int namelen = sizeof(ss_listen);
+#endif
apr_os_sock_get(nlsd, lr-sd);
+#if APR_HAVE_IPV6
+if (getsockname(nlsd, (struct sockaddr *)ss_listen, namelen) == SOCKET_ERROR) {
+ap_log_error(APLOG_MARK,APLOG_ERR, apr_get_netos_error(), ap_server_conf,
+winnt_accept: getsockname error on listening socket, is IPv6
available?);
+return;
+ }
+#endif
+
while (!shutdown_in_progress) {
if (!context) {
context = mpm_get_completion_context();
@@ -479,6 +501,25 @@
}
/* Create and initialize the accept socket */
+#if APR_HAVE_IPV6
+if (context-accept_socket == INVALID_SOCKET) {
+context-accept_socket = socket(ss_listen.ss_family, SOCK_STREAM,
IPPROTO_TCP);
+context-socket_family = ss_listen.ss_family;
+}
+else if (context-socket_family != ss_listen.ss_family) {
+closesocket(context-accept_socket);
+context-accept_socket = socket(ss_listen.ss_family, SOCK_STREAM,
IPPROTO_TCP);
+context-socket_family = ss_listen.ss_family;
+}
+
+if (context-accept_socket == INVALID_SOCKET) {
+ap_log_error(APLOG_MARK,APLOG_WARNING, apr_get_netos_error(),
ap_server_conf,
+ winnt_accept: Failed to allocate an accept socket.
+ Temporary resource constraint? Try again.);
+Sleep(100);
+continue;
+}
+#else
if (context-accept_socket == INVALID_SOCKET) {
context-accept_socket = socket(AF_INET, SOCK_STREAM, IPPROTO_TCP);
if (context-accept_socket == INVALID_SOCKET) {
@@ -490,7 +531,7 @@
continue;
}
}
-
+#endif
/* AcceptEx on the completion context. The completion context will be
* signaled when a connection is accepted.
*/
@@ -607,7 +648,7 @@
sockinfo.os_sock =
Re: Win32 .pdb symbol support [was: Time...] for 1.3.28
William A. Rowe, Jr. wrote: Ok, the .pdb changes are committed to apache-1.3 head, along with the updated .mak/.dep files. If someone else would mind checking out and validating the command line build (makefile.win), both Release and Debug, I'd be much obliged. Looks good to me. Allan
Re: cvs commit: httpd-2.0/server/mpm/winnt child.c mpm_winnt.c mpm_winnt.h
Rather than WindowsSocketsWorkaround, why not WinUseWinsock1 or ??. It would be better I think if the directive somehow indicated exactly what it was doing (causing the winnt mpm to use the select/accept winsock1 calls rather than AcceptEx, a winsock2 call). We're still usng winsock2 with this directive, it's our use of some of MS winsock extension calls (mswsock.dll) that are biting us. In fact I wrestled with the same question myself before deciding that otherBills suggestion was probably the best. If we were to be more to the point maybe something like WindowsSocketsDontUseAcceptEx, but apart from the length how many webmasters are likely to know what AcceptEx is? I'm open to renaming if we can come up with something more suitable, Allan
Re: mod_cache forward proxy
Paul J. Reder wrote: There is an entry in the STATUS file about adding regular expression support for CacheEnable/CacheDisable, wouldn't regular expressions provide this function and a whole lot more? That could be the next step, did I hear a volunteer step forward ;-)
Re: cvs commit: httpd-2.0/server/mpm/winnt child.c mpm_winnt.c mpm_winnt.h
probably. My opinion in this case isn't strong either. Actually I wanted to exclude an oversight of the INIT_FLAG macro :) OK, point taken, thanks for the feedback Andre! Allan
Re: cvs commit: httpd-2.0/server/mpm/winnt child.c mpm_winnt.c mpm_winnt.h
+AP_INIT_TAKE1(WindowsSocketsWorkaround, set_sockets_workaround, NULL, RSRC_CONF, + Set \on\ to work around buggy Winsock provider implementations of certain VPN or Firewall software), + hey, no need to double code. AP_INIT_FLAG exists ;-) Well I guess that's just a matter of personal preference, I don't have strong feelings either way... took the cue from OtherBill. You might argue there is precedence for his style with Keepalive on|off. Any others think on|off is better dispensed with? Allan
mod_cache forward proxy
Currently CachEnable foo / will configure mod_cache to cache all forward proxy
responses. Seems to me we want to be a little more granular than that.
The following patch will enable the use of e.g. CacheEnable foo http:
to signify that just HTTP content should be cached. It also will allow
scoping to a particular server e.g. CacheEnable foo http://CacheOnlyThisServer/
which is useful for say a branch office proxy, where you want to cache content
from just the corporate server. Does this sound like a good idea? It will
of course break current forward proxy config files that are using CacheEnable foo / ,
but mod_cache *is* still experimental.
Index: mod_cache.c
===
RCS file: /home/cvs/httpd-2.0/modules/experimental/mod_cache.c,v
retrieving revision 1.73
diff -u -d -b -r1.73 mod_cache.c
--- mod_cache.c 3 Feb 2003 17:52:59 - 1.73
+++ mod_cache.c 5 Mar 2003 18:52:13 -
@@ -95,7 +95,7 @@
apr_uri_t uri = r-parsed_uri;
char *url = r-unparsed_uri;
apr_size_t urllen;
-char *path = uri.path;
+char *path = r-uri;
const char *types;
cache_info *info = NULL;
cache_request_rec *cache;
Also, anyone have a good reason why we can't remove these
lines and allow mod_cache to serve default welcome pages?
/* DECLINE urls ending in / ??? EGP: why? */
if (url[urllen-1] == '/') {
return DECLINED;
}
Allan
Re: PR 15282 AcceptEx problem
William A. Rowe, Jr. wrote:
Just to summarize, there are three conditions we need to consider:
1) we hit the TransmitFile recycle bug many times in a row
2) we have encountered an incompatible firewall or VPN
3) the IP address has changed
You seem to have the failcases easily reproduced. Would you tack in
some quick code that simply uses getsockopt(foo) (any option you like)
to see if simply getting socket options for a now-broken listen socket
will fail?
Actually I have not been able to reproduce the AcceptEx error
for 3), however I think the following will address all three
cases and introduces the WindowsSocketsWorkaround directive:
Index: mpm/winnt/child.c
===
RCS file: /home/cvs/httpd-2.0/server/mpm/winnt/child.c,v
retrieving revision 1.13
diff -u -d -b -r1.13 child.c
--- mpm/winnt/child.c 28 Feb 2003 14:02:42 - 1.13
+++ mpm/winnt/child.c 3 Mar 2003 22:31:15 -
@@ -498,7 +498,7 @@
PCOMP_CONTEXT context = NULL;
DWORD BytesRead;
SOCKET nlsd;
-int rv;
+int rv, err_count = 0;
apr_os_sock_get(nlsd, lr-sd);
@@ -538,15 +538,38 @@
rv = apr_get_netos_error();
if ((rv == APR_FROM_OS_ERROR(WSAEINVAL)) ||
(rv == APR_FROM_OS_ERROR(WSAENOTSOCK))) {
-/* Hack alert. Occasionally, TransmitFile will not recycle the
- * accept socket (usually when the client disconnects early).
- * Get a new socket and try the call again.
+/* Hack alert, we can get here because:
+ * 1) Occasionally, TransmitFile will not recycle the accept socket
+ *(usually when the client disconnects early).
+ * 2) There is VPN or Firewall software installed with buggy AcceptEx
implementation
+ * 3) The webserver is using a dynamic address and it has changed
*/
+Sleep(0);
+if (++err_count 1000) {
+apr_int32_t disconnected;
+
+/* abitrary socket call to test if the Listening socket is still
valid */
+apr_status_t listen_rv = apr_socket_opt_get(lr-sd,
APR_SO_DISCONNECTED, disconnected);
+
+if (listen_rv == APR_SUCCESS) {
+ap_log_error(APLOG_MARK,APLOG_ERR, listen_rv, ap_server_conf,
+ AcceptEx error: If this occurs constantly and NO
requests are being served
+ try using the WindowsSocketsWorkaround directive set
to 'on'.);
+err_count = 0;
+}
+else {
+ap_log_error(APLOG_MARK,APLOG_ERR, listen_rv, ap_server_conf,
+ The Listening socket is no longer valid. Dynamic
address changed?);
+break;
+}
+}
+
closesocket(context-accept_socket);
context-accept_socket = INVALID_SOCKET;
ap_log_error(APLOG_MARK, APLOG_DEBUG, rv, ap_server_conf,
- winnt_accept: AcceptEx failed due to early client
- disconnect. Reallocate the accept socket and try again.);
+ winnt_accept: AcceptEx failed, either early client disconnect,
+ dynamic address renewal, or incompatible VPN or Firewall
software.);
+
continue;
}
else if ((rv != APR_FROM_OS_ERROR(ERROR_IO_PENDING))
@@ -558,6 +581,7 @@
Sleep(100);
continue;
}
+err_count = 0;
/* Wait for pending i/o.
* Wake up once per second to check for shutdown .
@@ -701,7 +725,7 @@
ap_update_child_status_from_indexes(0, thread_num, SERVER_READY, NULL);
/* Grab a connection off the network */
-if (osver.dwPlatformId == VER_PLATFORM_WIN32_WINDOWS) {
+if (osver.dwPlatformId == VER_PLATFORM_WIN32_WINDOWS ||
windows_sockets_workaround == 1) {
context = win9x_get_connection(context);
}
else {
@@ -769,7 +793,7 @@
static void create_listener_thread()
{
int tid;
-if (osver.dwPlatformId == VER_PLATFORM_WIN32_WINDOWS) {
+if (osver.dwPlatformId == VER_PLATFORM_WIN32_WINDOWS ||
windows_sockets_workaround == 1) {
_beginthreadex(NULL, 0, (LPTHREAD_START_ROUTINE) win9x_accept,
NULL, 0, tid);
} else {
@@ -840,7 +864,7 @@
* Create the worker thread dispatch IOCompletionPort
* on Windows NT/2000
*/
-if (osver.dwPlatformId != VER_PLATFORM_WIN32_WINDOWS) {
+if (osver.dwPlatformId != VER_PLATFORM_WIN32_WINDOWS
windows_sockets_workaround != 1) {
/* Create the worker thread dispatch IOCP */
ThreadDispatchIOCP =
Re: PR 15282 AcceptEx problem
William A. Rowe, Jr. wrote: This patch can't be applied... it actually introduces a denial of service problem if folks can simply early-disconnect a server some half dozen actually 100 :) times in a row... It isn't hard to work up such a tool. If it is possible for someone to externally tickle the TransmitFile socket recycle bug then I agree. Better; what if we test *which* socket failed. We are sort of helpless when the errors could be either the Listen and Accept socket. If the error is on the Listen socket, we should exit signaling the parent to do a restart with new listeners, if the error is on the accept socket we can just keep going. Based on the IP address renewal scenario you mention below, testing the Listen socket (somehow, tbd) sounds like a good idea. Just to summarize, there are three conditions we need to consider: 1) we hit the TransmitFile recycle bug many times in a row 2) we have encountered an incompatible firewall or VPN 3) the IP address has changed Instead, can we find some patch that will test AcceptEx? Perhaps we create a single local listen and attempt to connect and write to it, test that the AcceptEx succeeds, and otherwise emit some nasty warnings and throw a flag that puts us into the Win9x listener code? Testing AcceptEx is not easy, the failure only occurs when duplicating the socket between processes. But maybe testing the Listen socket provides us with enough information to indicate what the problem might be and suggest or perform corrective action. Does accept() also fail? Can we use the 9x code to work around these sorts of problems? No, accept() is fine. Using the 9x path *may* work but I haven't tested it. The other option Bill S. suggested was to add a directive that forces the 9x path. I tend to think that is preferable than a run time decision because I'm not sure we can reliably determine which path to take at runtime. Note: taking the 9x path is only relevant to case 2) above. I don't as much mind the Sleep(100) or even Sleep(0) so that we relinquish clock cycles. It's the arbitrary foil the server 100 times and it will exit problem. OK, so we can log a msg continue instead of exiting. Since we may not be able to guarantee a false positive maybe we should modify the error message and say that if NO requests are being served it is probably a firewall or VPN problem, but continue the accept loop. However, prior to logging this message we would need to test the Listen socket and, if it is bad, log a message saying that the IP address has probably become invalid, then exit the child and let the parent renew the Listeners. Because those only occur once the listen socket becomes invalidated, due to DHCP or some other change. You can trigger by reconfiguring TCP/IP to switch between two IP addresses. Again, we can recover gracefully if we ask the parent to do a respawn upon recreating all of *it's* listeners. i.e. whenever we hit some threshold of consecutive AcceptEx errors test the Listening socket (tbd somehow), and exit the child if it is bad. Allan
PR 15282 AcceptEx problem
As far as I can tell this is a bug in the Sprint
PCS Connect support for AcceptEx, (they install a
Winsock transport provider called BMI). However, it slips
through our checks and causes the accept thread to
hard loop and consume most of the cpu.
What happens is that in get_listeners_from_parent()
WSASocket *succeeeds* using the WSAProtocolInfo from
the parent however, AcceptEx in winnt_accept() fails
with WSAENOTSOCK.
I don't see what we can do to fix this but we should
at least avoid hogging the cpu and log an informative
message. Unless there is a better idea I'll commit to 2.1
16327 may be related but I haven't been able to recreate
the problem with BlackIce or Norton Personal Firewall.
Allan
Index: child.c
===
RCS file: /home/cvs/httpd-2.0/server/mpm/winnt/child.c,v
retrieving revision 1.12
diff -u -d -b -r1.12 child.c
--- child.c 26 Feb 2003 21:55:54 - 1.12
+++ child.c 27 Feb 2003 16:38:59 -
@@ -498,7 +498,7 @@
PCOMP_CONTEXT context = NULL;
DWORD BytesRead;
SOCKET nlsd;
-int rv;
+int rv, err_count = 0;
apr_os_sock_get(nlsd, lr-sd);
@@ -547,6 +547,14 @@
ap_log_error(APLOG_MARK, APLOG_DEBUG, rv, ap_server_conf,
winnt_accept: AcceptEx failed due to early client
disconnect. Reallocate the accept socket and try again.);
+
+Sleep(100);
+if (++err_count 100) {
+ap_log_error(APLOG_MARK,APLOG_ERR, rv, ap_server_conf,
+ AcceptEx unrecoverable error,
+ possibly incompatible firewall or VPN software is
installed.);
+break;
+}
continue;
}
else if ((rv != APR_FROM_OS_ERROR(ERROR_IO_PENDING))
@@ -558,6 +566,7 @@
Sleep(100);
continue;
}
+err_count = 0;
/* Wait for pending i/o.
* Wake up once per second to check for shutdown .
Index: child.c
===
RCS file: /home/cvs/httpd-2.0/server/mpm/winnt/child.c,v
retrieving revision 1.12
diff -u -d -b -r1.12 child.c
--- child.c 26 Feb 2003 21:55:54 - 1.12
+++ child.c 27 Feb 2003 16:38:59 -
@@ -498,7 +498,7 @@
PCOMP_CONTEXT context = NULL;
DWORD BytesRead;
SOCKET nlsd;
-int rv;
+int rv, err_count = 0;
apr_os_sock_get(nlsd, lr-sd);
@@ -547,6 +547,14 @@
ap_log_error(APLOG_MARK, APLOG_DEBUG, rv, ap_server_conf,
winnt_accept: AcceptEx failed due to early client
disconnect. Reallocate the accept socket and try again.);
+
+Sleep(100);
+if (++err_count 100) {
+ap_log_error(APLOG_MARK,APLOG_ERR, rv, ap_server_conf,
+ AcceptEx unrecoverable error,
+ possibly incompatible firewall or VPN software is
installed.);
+break;
+}
continue;
}
else if ((rv != APR_FROM_OS_ERROR(ERROR_IO_PENDING))
@@ -558,6 +566,7 @@
Sleep(100);
continue;
}
+err_count = 0;
/* Wait for pending i/o.
* Wake up once per second to check for shutdown .
Re: PR 15282 AcceptEx problem
Bill Stoddard wrote: Humm... how do our friends at MS solve this in IIS? It only happens because of our parent-child process model. If you run -X the problem goes away. It's the socket duplication that seems to bite us. Allan
Re: PR 15282 AcceptEx problem
Perhaps we need a winnt mpm directive to force the server to use the Win9* accept code path. Whould be a terrible thing to do on a production level server (for performance reasons) but quite okay for most of the folks that are seeing personal firewalls collide with our use of AcceptEx. mmm... that might work. PCS Connect has no problem with the accept() call. Allan
core.c not handling APR_ENOTIMPL from apr_sendfile
Without this I believe Win98/ME are broken
on HEAD and APACHE_2_0_BRANCH. OK to commit?
Allan
Index: core.c
===
RCS file: /home/cvs/httpd-2.0/server/core.c,v
retrieving revision 1.225.2.1
diff -u -d -b -r1.225.2.1 core.c
--- core.c 9 Jan 2003 16:27:25 - 1.225.2.1
+++ core.c 10 Jan 2003 17:46:16 -
@@ -3972,6 +3972,11 @@
sent */
flags); /* apr_sendfile flags*/
+if (APR_ENOTIMPL == rv) {
+rv = emulate_sendfile(net, fd, hdtr, foffset, flen,
+ bytes_sent);
+}
+
if (logio_add_bytes_out bytes_sent 0)
logio_add_bytes_out(c, bytes_sent);
}
Index: core.c
===
RCS file: /home/cvs/httpd-2.0/server/core.c,v
retrieving revision 1.225.2.1
diff -u -d -b -r1.225.2.1 core.c
--- core.c 9 Jan 2003 16:27:25 - 1.225.2.1
+++ core.c 10 Jan 2003 17:46:16 -
@@ -3972,6 +3972,11 @@
sent */
flags); /* apr_sendfile flags*/
+if (APR_ENOTIMPL == rv) {
+rv = emulate_sendfile(net, fd, hdtr, foffset, flen,
+ bytes_sent);
+}
+
if (logio_add_bytes_out bytes_sent 0)
logio_add_bytes_out(c, bytes_sent);
}
RE: win32 changes from 1.3
I suggest we cwd to the server root on startup. We can do this in the winnt_mpm, or for all platforms in main(). Opinions? +1 - at least for Windows And +1 for the rest, or was that simply +0 to do so in main()? why would we want to chdir() for the other platforms? maybe it wouldn't hurt at this stage (we chdir(/) later in processing on Unix) Well I don't feel comfortable giving the recommendation for Unix etc, i.e. what the side effects might be so unless someone else says +1 for main I would go with winnt_mpm. Allan
win32 changes from 1.3
In 1.3 we did not need to specify a drive letter for ServerRoot and DocumentRoot paths but in 2.0 it appears we must specify the drive letter or Apache will not start as a service. (note: the 2.0.36 .msi install sets the drive letter so this limitation is not immediately apparent to those users). Also, in 1.3 the -k stop/restart directives worked even if you started Apache from the command line. Now it appears that -k is only effective when running as a service. Were these changes intentional or did they slip in inadvertently? Allan
RE: win32 changes from 1.3
If your cwd is on another volume, that's a problem. This hurts services, since the cwd will always be c:\winnt\system32\. Yep, was installed on a non-C: drive I suggest we cwd to the server root on startup. We can do this in the winnt_mpm, or for all platforms in main(). Opinions? +1 - at least for Windows -k preferring a service was a deliberate change. That was my guess, but I had to ask :) Allan
RE: [PATCH] Implement -k option for httpd
I'm not going to finish it because: a) I'm not really sure what to do on Win32. main.c(617) : error C2065: 'SIGHUP' : undeclared identifier main.c(620) : error C2065: 'AP_SIG_GRACEFUL' : undeclared identifier main.c(628) : warning C4013: 'kill' undefined; assuming extern returning int Error executing cl.exe. Maybe the best thing to do is follow the Windows model. The shutdown/restart etc. processing is handled by the mpm (on the rewrite_args hook), which is probably the logical place to do this sort of thing. Allan
RE: [PATCH] Implement -k option for httpd
Doesn't the new function need to be done before we hit ap_mpm_run()? ap_run_rewrite_args gets called before ap_mpm_run in main(). If main() doesn't know what's going on then how would it work? I was talking about putting the bit that didn't compile on windows in the unix mpm, mainly the part inside: if (sendsignal != AP_NONE sendsignal != AP_START) If this is common for all unix mpm's then as Will said it should go in mpm_common.c Allan
RE: cvs commit: httpd-2.0/include ap_release.h
Let me just go on record saying that I don't think we're in a position to release another version. I'll second that based on problems I still see with filters - additional post coming momentarily. Allan
filtering problems fix
A simple SSI file with two #include file directives
will coredump due the fact that we are copying the
filter chain *pointers* from the main request to
the subrequest in make_sub_request. When we add
the subreq_core_filter this corrupts the main
filter chain.
We need to copy the filter chain, not just the
pointers. Attached is a patch to do this. I will
commit if there are no objections.
There is also at least one remaining bug in
add_any_filter handle due to the fact that
r-output_filters and/or r-proto_output_filters
are not getting updated when filters are added.
I'm still looking into that one if no-one beats
me to it.
Allan
Index: include/util_filter.h
===
RCS file: /home/cvs/httpd-2.0/include/util_filter.h,v
retrieving revision 1.67
diff -u -d -b -r1.67 util_filter.h
--- include/util_filter.h 3 Mar 2002 06:04:08 - 1.67
+++ include/util_filter.h 6 Mar 2002 18:24:23 -
-515,6 +515,18
...)
__attribute__((format(printf,3,4)));
+/**
+ * Copy the in/out filter chains from one request to another
+ * param from The request to copy the chains from
+ * param to The request to copy the chains to
+ * param start_filter The filter from which point the output chain copy starts
+ * return void
+ * deffunc void ap_copy_filter_chains(const request_rec *from, request_rec *to,
+const ap_filter_t *start_filter)
+ */
+void ap_copy_filter_chains(const request_rec *from,
+ request_rec *to,
+ const ap_filter_t *start_filter);
+
#ifdef __cplusplus
}
#endif
Index: server/request.c
===
RCS file: /home/cvs/httpd-2.0/server/request.c,v
retrieving revision 1.102
diff -u -d -b -r1.102 request.c
--- server/request.c5 Mar 2002 05:24:21 - 1.102
+++ server/request.c6 Mar 2002 18:24:25 -
-1492,14 +1492,7
/* start with the same set of output filters */
if (next_filter) {
-/* while there are no input filters for a subrequest, we will
- * try to insert some, so if we don't have valid data, the code
- * will seg fault.
- */
-rnew-input_filters = r-input_filters;
-rnew-proto_input_filters = r-proto_input_filters;
-rnew-output_filters = next_filter;
- rnew-proto_output_filters = r-connection-output_filters;
+ap_copy_filter_chains(r, rnew, next_filter);
ap_add_output_filter_handle(ap_subreq_core_filter_handle,
NULL, rnew, rnew-connection);
}
Index: server/util_filter.c
===
RCS file: /home/cvs/httpd-2.0/server/util_filter.c,v
retrieving revision 1.85
diff -u -d -b -r1.85 util_filter.c
--- server/util_filter.c6 Mar 2002 17:29:39 - 1.85
+++ server/util_filter.c6 Mar 2002 18:24:25 -
-615,3 +615,96
va_end(args);
return rv;
}
+
+void ap_copy_filter_chains(const request_rec *from,
+ request_rec *to,
+ const ap_filter_t *start_filter)
+{
+int connection_done = 0, proto_done = 0;
+const ap_filter_t *from_filter;
+ap_filter_t *to_filter, *curr_filter;
+
+from_filter = from-connection-output_filters;
+while (from_filter-next != NULL)
+from_filter = from_filter-next;
+
+curr_filter = NULL;
+while(1) {
+
+to_filter = apr_palloc(to-pool, sizeof(ap_filter_t));
+memcpy(to_filter, from_filter, sizeof(ap_filter_t));
+to_filter-next = curr_filter;
+
+if (NULL != curr_filter)
+ curr_filter-prev = to_filter;
+
+curr_filter = to_filter;
+
+if (!connection_done) {
+if (from_filter == from-connection-output_filters) {
+to-connection-output_filters = curr_filter;
+connection_done = 1;
+}
+}
+else if (!proto_done) {
+if (from_filter == from-proto_output_filters) {
+to-proto_output_filters = curr_filter;
+proto_done = 1;
+}
+}
+
+if (from_filter == start_filter)
+break;
+
+from_filter = from_filter-prev;
+if (NULL == from_filter)
+break;
+}
+
+if (!proto_done)
+to-proto_output_filters = curr_filter;
+if (NULL == to-output_filters)
+to-output_filters = to-proto_output_filters;
+
+from_filter = from-connection-input_filters;
+while (from_filter-next != NULL)
+from_filter = from_filter-next;
+
+connection_done = proto_done = 0;
+curr_filter = NULL;
+
+while(1) {
+
+to_filter = apr_palloc(to-pool, sizeof(ap_filter_t));
+memcpy(to_filter, from_filter, sizeof(ap_filter_t));
+
RE: filtering problems fix
The other thing is that we actually want to use the exact same filters. My patch replaces the ap_filter_t structures from the original chain with exact copies in the subreq chain so 1) we have the exact same filter chain and 2) and adds/removes will only affect the subrequest chain. That is one of the goals of subrequests, so I believe that this is the wrong solution. I will remove the previous filter ASAP and see if it still works. I guess I don't see how this fixes the problem that the subrequest can be adding/subtracting filters and if we are just copying the pointers from the main request we had better be sure that before we return from the subrequest all adds/subtracts are undone, and I don't see how we can guarantee that. Can you provide a config that causes this bug? A simple SSI file with two #include file directives Cheers, Allan
RE: filtering problems fix
I found the source of the bug. The SUB_REQ filter is an HTTP_HEADER filter, but it shouldn't be. That is a resource filter, because it is only added when the resource changes. Yes, that's what I just saw and is the source of at least one of the bugs I've been hitting. I eagerly await your patch! Allan
RE: filtering problems fix
Did that patch fix the bug for everybody? If so, I want to commit it. I have a three hour meeting now, so I'm not going to have time to though. Can somebody else commit it this afternoon if it works? Thanks Ryan, with your patch we longer trap in the SSI testcase but I have a question. In make_sub_request where the filter pointers are copied, why this: rnew-proto_output_filters = r-connection-output_filters; rnew-output_filters is pointing to the remainder of the main chain (next_filter) and rnew-proto_output_filters is not pointing to the proto part of it. Is this just a placeholder in case some subrequest code tries to add a proto filter? My only other observation is that any filters added in the subreq must only be added to the top of the rnew-output_filters chain or must be cleanly removed before the subreq returns otherwise the main chain will get corrupted. I haven't seen this happen so far, so maybe this is just a hypothetical concern. Allan
RE: filtering problems fix
My only other observation is that any filters added in the subreq must only be added to the top of the rnew-output_filters chain or must be cleanly removed before the subreq returns otherwise the main chain will get corrupted. I haven't seen this happen so far, so maybe this is just a hypothetical concern. It isn't hypothetical, but it also isn't a bug we want to protect against. The thing is that the filters added in the subrequest by definition must be resource filters, which means that they must be added above protocol and connection filters. If you are adding either a protocol or connection filter in a sub request, then you are doing something incredibly wrong, and we want the server to fail. Absolutely. The case I was actually thinking about but didn't clearly explain was something like say mod_deflate, which if it were AP_FTYPE_CONTENT_SET would be above the proto chain passed by next_filter. Even so I don't know of any subreq filter that would get placed after this. Allan
RE: HTTP, Internet Explorer, and *large* POSTs
Jerry, I've been trying to nail this one for a while now but have not been able to recreate - do you have a testcase that will generate the questionable POST request? What is catching the POST on the server - a CGI? What platform is the server? Please send any further info to me directly rather than through the list. Thanks, Allan Large POSTs, say of 8K bytes fail. When I've examined this with tcpdump, I've seen that Internet Explorer has not sent out one POST but will sometimes send out two or even three POSTs of the same URL. Only the first POST that is sent out will contain the POST data. And even better, sometimes the third POST transforms itself into a GET. It seems to be a function of the length of the POST, but that length is not repeatable. It happens much more often with long POSTs, say of 8K, but on occasion, I believe I've seen it happen with as few as a 1K byte POST.
RE: client_socket bogosity...
Please don't let two mis-behaved modules color your judgment on this. Both proxy and perchild must be re-written if they are going to be clean, and once that is done the stupid set_module_config can be removed. In fact, the server ran for over a day without the set_module_config, but that broke the proxy, so I added the hack to allow the proxy to continue to work, while I worked to solve the underlying problem. mmm... I'd be interested to know what the solution is for net_time_filter since it is using the ap_get_module_config hack also. Allan
RE: client_socket bogosity...
The real solution is to pass the core_net_rec structure to the NET_TIME filter as user-data for the filter, the same way we do for CORE_IN and CORE_OUT. But you'll have to play some sort of trick to do that since the NET_TIME filter is added from the create_request hook which does not know about core_net_rec, only core_create_conn knows about that. I suppose some core specific magic could be played but that would prevent any other module from running it's own net_time hook. Allan
RE: client_socket bogosity...
prevent any other module from running it's own net_time hook. I meant net_time filter...
Windows cgi problem
This might also be a problem on unix but I haven't tested.
When cgi's are launched a window sometimes pops up, same for
mod_include exec cgi. Is there a reason we are not
launching the cgi as a detached process?
Index: mod_cgi.c
===
RCS file: /home/cvs/httpd-2.0/modules/generators/mod_cgi.c,v
retrieving revision 1.113
diff -u -d -b -r1.113 mod_cgi.c
--- mod_cgi.c 2001/12/13 17:22:20 1.113
+++ mod_cgi.c 2001/12/13 18:49:03
@@ -446,6 +446,7 @@
couldn't set child process attributes: %s, r-filename);
}
else {
+apr_procattr_detach_set(procattr, 1);
procnew = apr_pcalloc(p, sizeof(*procnew));
if (e_info-prog_type == RUN_AS_SSI) {
SPLIT_AND_PASS_PRETAG_BUCKETS(*(e_info-bb), e_info-ctx, e_info-next,
rc);
Allan
