You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@httpd.apache.org by Dean Gaudet <dg...@arctic.org> on 1997/07/31 11:36:40 UTC
[PATCH] some cleanups and bug fixes
I attacked apache with strace tonight, just for the fun of it.
- we call time() many times, when we could just use r->request_time
- we call getpid() way too much
- we re-register the timeout signal handler way too much
- lingering_close() would call bclose without unregistering the cleanup
for the socket, which causes the socket to be close()d twice, which
is a bug. Fixed by making bclose use pclose/pclosesocket ... which
means that the callers have to ensure the cleanups are in the same pool
as the BUFF *.
- added psocket() because none of the places where socket() was called
were properly wrapped by block_alarms()/unblock_alarms()
- mod_proxy now uses bclose() in many cases where it would previously
use pclosesocket().
- fixed a proxy ftp missing return SERVER_ERROR (it would continue going
after closing up sockets)
Dean
Index: CHANGES
===================================================================
RCS file: /export/home/cvs/apache/src/CHANGES,v
retrieving revision 1.373
diff -u -r1.373 CHANGES
--- CHANGES 1997/07/30 18:44:59 1.373
+++ CHANGES 1997/07/31 09:30:47
@@ -1,5 +1,14 @@
Changes with Apache 1.3a2
+ *) Added psocket() which is a pool form of socket(), various places within
+ the proxy weren't properly blocking alarms while registering the cleanup
+ for its sockets. bclose() now uses pclose() and pclosesocket(). There
+ was a bug where the client socket was being close()d twice due a still
+ registered cleanup. [Dean Gaudet]
+
+ *) A few cleanups were made to reduce unneeded time() and getpid()
+ calls. [Dean Gaudet]
+
*) "HostnameLookups double" forces double-reverse DNS to succeed in
order for remote_host to be set (for logging, or for the env var
REMOTE_HOST). The old define MAXIMUM_DNS has been deprecated.
Index: alloc.c
===================================================================
RCS file: /export/home/cvs/apache/src/alloc.c,v
retrieving revision 1.45
diff -u -r1.45 alloc.c
--- alloc.c 1997/07/25 09:41:29 1.45
+++ alloc.c 1997/07/31 09:30:50
@@ -985,6 +985,23 @@
kill_cleanup(p,(void *)(long)sock,socket_cleanup);
}
+API_EXPORT(int) psocket (pool *p, int domain, int type, int protocol)
+{
+ int fd;
+
+ block_alarms();
+ fd = socket (domain, type, protocol);
+ if (fd == -1) {
+ int save_errno = errno;
+ unblock_alarms();
+ errno = save_errno;
+ return -1;
+ }
+ note_cleanups_for_socket (p, fd);
+ unblock_alarms();
+ return fd;
+}
+
API_EXPORT(int) pclosesocket(pool *a, int sock)
{
int res;
Index: alloc.h
===================================================================
RCS file: /export/home/cvs/apache/src/alloc.h,v
retrieving revision 1.31
diff -u -r1.31 alloc.h
--- alloc.h 1997/07/21 05:53:41 1.31
+++ alloc.h 1997/07/31 09:30:51
@@ -220,6 +220,7 @@
API_EXPORT(void) note_cleanups_for_socket (pool *, int);
API_EXPORT(void) kill_cleanups_for_socket (pool *p, int sock);
+API_EXPORT(int) psocket (pool *p, int, int, int);
API_EXPORT(int) pclosesocket(pool *a, int sock);
API_EXPORT(regex_t *) pregcomp (pool *p, const char *pattern, int cflags);
Index: buff.c
===================================================================
RCS file: /export/home/cvs/apache/src/buff.c,v
retrieving revision 1.40
diff -u -r1.40 buff.c
--- buff.c 1997/07/31 09:10:29 1.40
+++ buff.c 1997/07/31 09:30:53
@@ -1175,17 +1175,17 @@
else rc1 = 0;
#ifdef WIN32
if (fb->flags & B_SOCKET) {
- rc2 = closesocket(fb->fd);
+ rc2 = pclosesocket(fb->pool, fb->fd);
if (fb->fd_in != fb->fd) {
- rc3 = closesocket(fb->fd_in);
+ rc3 = pclosesocket(fb->pool, fb->fd_in);
} else {
rc3 = 0;
}
} else {
#endif
- rc2 = close(fb->fd);
+ rc2 = pclosef(fb->pool, fb->fd);
if (fb->fd_in != fb->fd) {
- rc3 = close(fb->fd_in);
+ rc3 = pclosef(fb->pool, fb->fd_in);
} else {
rc3 = 0;
}
Index: http_main.c
===================================================================
RCS file: /export/home/cvs/apache/src/http_main.c,v
retrieving revision 1.191
diff -u -r1.191 http_main.c
--- http_main.c 1997/07/29 06:57:19 1.191
+++ http_main.c 1997/07/31 09:31:01
@@ -219,6 +219,8 @@
pool *pconf; /* Pool for config stuff */
pool *ptrans; /* Pool for per-transaction stuff */
+int APACHE_TLS my_pid; /* it seems silly to call getpid all the time */
+
/* small utility macros to make things easier to read */
#ifdef WIN32
@@ -512,7 +514,9 @@
#else
if(x)
alarm_fn = fn;
- signal(SIGALRM, fn);
+ if (alarm_fn != fn) {
+ signal(SIGALRM, fn);
+ }
old = alarm(x);
#endif
return(old);
@@ -1154,8 +1158,8 @@
sync_scoreboard_image();
new_score_rec = scoreboard_image->servers[child_num];
- new_score_rec.x.pid = getpid();
old_status = new_score_rec.status;
+ new_score_rec.x.pid = my_pid;
new_score_rec.status = status;
#if defined(STATUS)
@@ -1289,7 +1293,6 @@
{
#ifndef MULTITHREAD
int i, status;
- int my_pid = getpid();
sync_scoreboard_image();
for (i = 0; i < max_daemons_limit; ++i) {
@@ -1897,19 +1900,23 @@
int s;
int one = 1;
+ /* note that because we're about to slack we don't use psocket */
+ block_alarms();
if ((s = socket(AF_INET,SOCK_STREAM,IPPROTO_TCP)) == -1) {
log_unixerr("socket", NULL, "Failed to get a socket, exiting child",
server_conf);
+ unblock_alarms();
exit(1);
}
#ifdef SOLARIS2
sock_bind (s, server);
#endif
-
+
s = ap_slack(s, AP_SLACK_HIGH);
note_cleanups_for_socket(p, s); /* arrange to close on exec or restart */
+ unblock_alarms();
#ifndef MPE
/* MPE does not support SO_REUSEADDR and SO_KEEPALIVE */
@@ -2161,6 +2168,7 @@
struct sockaddr sa_client;
listen_rec *lr;
+ my_pid = getpid();
csd = -1;
dupped_csd = -1;
child_num = child_num_arg;
@@ -2619,6 +2627,8 @@
if (!one_process) detach ();
+ my_pid = getpid();
+
do {
copy_listeners(pconf);
if (!is_graceful) {
@@ -3209,6 +3219,8 @@
int requests_this_child = 0;
pool *ppool = NULL;
+ my_pid = getpid();
+
/*
* Only reason for this function, is to pass in
* arguments to child_sub_main() on its stack so
@@ -3283,6 +3295,8 @@
max_jobs_after_exit_request = max_jobs_per_exe/10;
if (!one_process) detach();
+
+ my_pid = getpid();
++generation;
Index: http_protocol.c
===================================================================
RCS file: /export/home/cvs/apache/src/http_protocol.c,v
retrieving revision 1.146
diff -u -r1.146 http_protocol.c
--- http_protocol.c 1997/07/30 18:41:52 1.146
+++ http_protocol.c 1997/07/31 09:31:09
@@ -350,10 +350,9 @@
{
char *etag, weak_etag[MAX_STRING_LEN];
char *if_match, *if_modified_since, *if_unmodified, *if_nonematch;
- time_t now = time(NULL);
+ time_t now;
- if (now < 0)
- now = r->request_time;
+ now = r->request_time;
table_set(r->headers_out, "Last-Modified",
gm_timestr_822(r->pool, (mtime > now) ? now : mtime));
Index: mod_expires.c
===================================================================
RCS file: /export/home/cvs/apache/src/mod_expires.c,v
retrieving revision 1.14
diff -u -r1.14 mod_expires.c
--- mod_expires.c 1997/07/28 18:22:57 1.14
+++ mod_expires.c 1997/07/31 09:31:12
@@ -444,7 +444,7 @@
/* there's been some discussion and it's possible that
* 'access time' will be stored in request structure
*/
- base = time( NULL );
+ base = r->request_time;
additional = atoi( &code[1] );
break;
default:
@@ -456,7 +456,7 @@
};
expires = base + additional;
- ap_snprintf(age, sizeof(age), "max-age=%d", (int)expires - (int)time(NULL));
+ ap_snprintf(age, sizeof(age), "max-age=%d", (int)expires - (int)r->request_time);
table_set( r->headers_out, "Cache-Control", age);
tzset(); /* redundant? called implicitly by localtime, at least
* under FreeBSD
Index: mod_usertrack.c
===================================================================
RCS file: /export/home/cvs/apache/src/mod_usertrack.c,v
retrieving revision 1.15
diff -u -r1.15 mod_usertrack.c
--- mod_usertrack.c 1997/07/28 18:23:05 1.15
+++ mod_usertrack.c 1997/07/31 09:31:13
@@ -151,7 +151,7 @@
mpe_times=times(&mpe_tms);
ap_snprintf(cookiebuf, 1024, "%s%d%ld%ld", rname, (int)getpid(),
- (long)time(NULL), (long)mpe_tms.tms_utime);
+ (long)r->request_time, (long)mpe_tms.tms_utime);
#elif defined(WIN32)
/* We lack gettimeofday() and we lack times(). So we'll use
* a combination of time() and GetTickCount(), which returns
@@ -160,7 +160,7 @@
*/
ap_snprintf(cookiebuf, 1024, "%s%d%ld%ld", rname, (int)getpid(),
- (long)time(NULL), (long)GetTickCount());
+ (long)r->request_time, (long)GetTickCount());
#else
gettimeofday(&tv, &tz);
@@ -173,7 +173,7 @@
static const char *const days[7]=
{"Sun","Mon", "Tue", "Wed", "Thu", "Fri", "Sat"};
struct tm *tms;
- time_t when = time(NULL) + cls->expires;
+ time_t when = r->request_time + cls->expires;
#ifndef MILLENIAL_COOKIES
/* Only two-digit date string, so we can't trust "00" or more.
Index: rfc1413.c
===================================================================
RCS file: /export/home/cvs/apache/src/rfc1413.c,v
retrieving revision 1.13
diff -u -r1.13 rfc1413.c
--- rfc1413.c 1997/07/21 05:53:51 1.13
+++ rfc1413.c 1997/07/31 09:31:14
@@ -193,7 +193,7 @@
result = FROM_UNKNOWN;
- sock = socket(AF_INET, SOCK_STREAM, IPPROTO_TCP);
+ sock = psocket(conn->pool, AF_INET, SOCK_STREAM, IPPROTO_TCP);
if (sock < 0)
{
log_unixerr("socket", NULL, "rfc1413: error creating socket", srv);
@@ -214,7 +214,7 @@
set_callback_and_alarm(NULL, 0);
}
- closesocket(sock);
+ pclosesocket(conn->pool, sock);
conn->remote_logname = result;
return conn->remote_logname;
Index: modules/proxy/proxy_connect.c
===================================================================
RCS file: /export/home/cvs/apache/src/modules/proxy/proxy_connect.c,v
retrieving revision 1.11
diff -u -r1.11 proxy_connect.c
--- proxy_connect.c 1997/07/27 03:13:33 1.11
+++ proxy_connect.c 1997/07/31 09:31:14
@@ -145,13 +145,12 @@
if (err != NULL)
return proxyerror(r, err); /* give up */
- sock = socket(PF_INET, SOCK_STREAM, IPPROTO_TCP);
+ sock = psocket(r->pool, PF_INET, SOCK_STREAM, IPPROTO_TCP);
if (sock == -1)
{
log_error("proxy: error creating socket", r->server);
return SERVER_ERROR;
}
- note_cleanups_for_fd(r->pool, sock);
j = 0;
while (server_hp.h_addr_list[j] != NULL) {
@@ -162,8 +161,10 @@
break;
j++;
}
- if (i == -1 )
+ if (i == -1) {
+ pclosesocket(r->pool, sock);
return proxyerror(r, "Could not connect to remote machine");
+ }
Explain0("Returning 200 OK Status");
Index: modules/proxy/proxy_ftp.c
===================================================================
RCS file: /export/home/cvs/apache/src/modules/proxy/proxy_ftp.c,v
retrieving revision 1.27
diff -u -r1.27 proxy_ftp.c
--- proxy_ftp.c 1997/07/27 03:13:33 1.27
+++ proxy_ftp.c 1997/07/31 09:31:16
@@ -490,14 +490,13 @@
err = proxy_host2addr(host, &server_hp);
if (err != NULL) return proxyerror(r, err); /* give up */
- sock = socket(PF_INET, SOCK_STREAM, IPPROTO_TCP);
+ sock = psocket(pool, PF_INET, SOCK_STREAM, IPPROTO_TCP);
if (sock == -1)
{
proxy_log_uerror("socket", NULL, "proxy: error creating socket",
r->server);
return SERVER_ERROR;
}
- note_cleanups_for_socket(pool, sock);
if (setsockopt(sock, SOL_SOCKET, SO_REUSEADDR, (void *)&one,
sizeof(one)) == -1)
@@ -529,8 +528,10 @@
j++;
}
#endif
- if (i == -1)
+ if (i == -1) {
+ pclosesocket (pool, sock);
return proxyerror(r, "Could not connect to remote machine");
+ }
f = bcreate(pool, B_RDWR | B_SOCKET);
bpushfd(f, sock, sock);
@@ -674,16 +675,15 @@
}
/* try to set up PASV data connection first */
- dsock = socket(PF_INET, SOCK_STREAM, IPPROTO_TCP);
+ dsock = psocket(pool, PF_INET, SOCK_STREAM, IPPROTO_TCP);
if (dsock == -1)
{
proxy_log_uerror("socket", NULL, "proxy: error creating PASV socket",
r->server);
- pclosesocket(pool, sock);
+ bclose(f);
kill_timeout(r);
return SERVER_ERROR;
}
- note_cleanups_for_socket(pool, dsock);
bputs("PASV\015\012", f);
bflush(f);
@@ -696,7 +696,7 @@
proxy_log_uerror("command", NULL, "PASV: control connection is toast",
r->server);
pclosesocket(pool, dsock);
- pclosesocket(pool, sock);
+ bclose(f);
kill_timeout(r);
return SERVER_ERROR;
} else
@@ -733,8 +733,6 @@
return proxyerror(r, "Could not connect to remote machine");
}
else {
- data = bcreate(pool, B_RDWR | B_SOCKET);
- bpushfd(data, dsock, dsock);
pasvmode = 1;
}
} else
@@ -748,21 +746,20 @@
{
proxy_log_uerror("getsockname", NULL,
"proxy: error getting socket address", r->server);
- pclosesocket(pool, sock);
+ bclose(f);
kill_timeout(r);
return SERVER_ERROR;
}
- dsock = socket(PF_INET, SOCK_STREAM, IPPROTO_TCP);
+ dsock = psocket(pool, PF_INET, SOCK_STREAM, IPPROTO_TCP);
if (dsock == -1)
{
proxy_log_uerror("socket", NULL, "proxy: error creating socket",
r->server);
- pclosesocket(pool, sock);
+ bclose(f);
kill_timeout(r);
return SERVER_ERROR;
}
- note_cleanups_for_fd(pool, dsock);
if (setsockopt(dsock, SOL_SOCKET, SO_REUSEADDR, (void *)&one,
sizeof(one)) == -1)
@@ -770,7 +767,7 @@
proxy_log_uerror("setsockopt", NULL,
"proxy: error setting reuseaddr option", r->server);
pclosesocket(pool, dsock);
- pclosesocket(pool, sock);
+ bclose(f);
kill_timeout(r);
return SERVER_ERROR;
}
@@ -783,8 +780,9 @@
ap_snprintf(buff, sizeof(buff), "%s:%d", inet_ntoa(server.sin_addr), server.sin_port);
proxy_log_uerror("bind", buff,
"proxy: error binding to ftp data socket", r->server);
- pclosesocket(pool, sock);
+ bclose(f);
pclosesocket(pool, dsock);
+ return SERVER_ERROR;
}
listen(dsock, 2); /* only need a short queue */
}
@@ -925,7 +923,7 @@
if (i != DECLINED)
{
pclosesocket(pool, dsock);
- pclosesocket(pool, sock);
+ bclose(f);
return i;
}
cache = c->fp;
@@ -941,7 +939,7 @@
proxy_log_uerror("accept", NULL,
"proxy: failed to accept data connection", r->server);
pclosesocket(pool, dsock);
- pclosesocket(pool, sock);
+ bclose(f);
kill_timeout(r);
proxy_cache_error(c);
return BAD_GATEWAY;
@@ -950,6 +948,9 @@
data = bcreate(pool, B_RDWR | B_SOCKET);
bpushfd(data, csd, -1);
kill_timeout(r);
+ } else {
+ data = bcreate(pool, B_RDWR | B_SOCKET);
+ bpushfd(data, dsock, dsock);
}
hard_timeout ("proxy receive", r);
@@ -998,7 +999,7 @@
bputs("ABOR\015\012", f);
bflush(f);
if (!pasvmode)
- pclosesocket(pool, csd);
+ bclose(data);
Explain0("FTP: ABOR");
/* responses: 225, 226, 421, 500, 501, 502 */
i = ftp_getrc(f);
@@ -1014,10 +1015,9 @@
Explain0("FTP: QUIT");
/* responses: 221, 500 */
- if (!pasvmode)
- pclosesocket(pool, csd);
- pclosesocket(pool, dsock);
- pclosesocket(pool, sock);
+ if (pasvmode)
+ bclose(data);
+ bclose(f);
proxy_garbage_coll(r);
Index: modules/proxy/proxy_http.c
===================================================================
RCS file: /export/home/cvs/apache/src/modules/proxy/proxy_http.c,v
retrieving revision 1.23
diff -u -r1.23 proxy_http.c
--- proxy_http.c 1997/07/27 03:13:33 1.23
+++ proxy_http.c 1997/07/31 09:31:17
@@ -218,14 +218,12 @@
if (err != NULL) return proxyerror(r, err); /* give up */
}
- sock = socket(PF_INET, SOCK_STREAM, IPPROTO_TCP);
+ sock = psocket(pool, PF_INET, SOCK_STREAM, IPPROTO_TCP);
if (sock == -1)
{
log_error("proxy: error creating socket", r->server);
return SERVER_ERROR;
}
- note_cleanups_for_socket(pool, sock);
-
#ifdef SINIX_D_RESOLVER_BUG
{ struct in_addr *ip_addr = (struct in_addr *) *server_hp.h_addr_list;
@@ -295,7 +293,7 @@
len = bgets(buffer, HUGE_STRING_LEN-1, f);
if (len == -1 || len == 0)
{
- pclosesocket(pool, sock);
+ bclose(f);
kill_timeout(r);
return proxyerror(r, "Error reading from remote server");
}
@@ -306,7 +304,7 @@
/* If not an HTTP/1 messsage or if the status line was > 8192 bytes */
if (buffer[5] != '1' || buffer[len-1] != '\n')
{
- pclosesocket(pool, sock);
+ bclose(f);
kill_timeout(r);
return BAD_GATEWAY;
}
@@ -367,7 +365,7 @@
i = proxy_cache_update(c, resp_hdrs, inprotocol, nocache);
if (i != DECLINED)
{
- pclosesocket(pool, sock);
+ bclose(f);
return i;
}
@@ -418,7 +416,7 @@
proxy_cache_tidy(c);
- pclosesocket(pool, sock);
+ bclose(f);
proxy_garbage_coll(r);
return OK;