You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@httpd.apache.org by Jim Jagielski <ji...@jaguNET.com> on 2007/06/01 16:05:26 UTC
Re: [Fwd: Apache httpd vulenrabilities]
For 1.3, I'm looking at something like this...
Similar approach for 2.x...
Comments and feedback appreciated before I work on
"porting" to the 2.x trees:
Index: main/http_main.c
===================================================================
--- main/http_main.c (revision 543486)
+++ main/http_main.c (working copy)
@@ -354,9 +354,17 @@
char tpf_mutex_key[TPF_MUTEX_KEY_SIZE];
#endif /* TPF */
+/*
+ * Shared memory scoreboard
+ */
scoreboard *ap_scoreboard_image = NULL;
/*
+ * Parent process local storage of child pids
+ */
+static table *pid_table;
+
+/*
* Pieces for managing the contents of the Server response header
* field.
*/
@@ -372,6 +380,26 @@
API_VAR_EXPORT int ap_change_shmem_uid = 0;
/*
+ * Check the pid table to see if the actual pid exists
+ */
+static int in_pid_table(int pid) {
+ char apid[64];
+ const char *spid;
+ snprintf(apid, sizeof(apid), "%d", pid);
+ spid = ap_table_get(pid_table, apid);
+ if (spid && spid[0] == '1' && spid[1] == '\0')
+ return 1;
+ else
+ return 0;
+}
+
+static void set_pid_table(int pid) {
+ char apid[64];
+ snprintf(apid, sizeof(apid), "%d", pid);
+ ap_table_set(pid_table, apid, "1");
+}
+
+/*
* This routine is called when the pconf pool is vacuumed. It
resets the
* server version string to a known value and [re]enables
modifications
* (which are disabled by configuration completion).
@@ -2787,6 +2815,11 @@
if (pid == my_pid || pid == 0)
continue;
+ if (!in_pid_table(pid)) {
+ ap_log_error(APLOG_MARK, APLOG_NOERRNO|APLOG_ERR,
server_conf,
+ "Bad pid (%d) in scoreboard slot %d",
pid, i);
+ continue;
+ }
waitret = waitpid(pid, &status, WNOHANG);
if (waitret == pid || waitret == -1) {
ap_scoreboard_image->parent[i].pid = 0;
@@ -2887,13 +2920,21 @@
for (n = 0; n < max_daemons_limit; ++n) {
ap_sync_scoreboard_image();
- if (ap_scoreboard_image->servers[n].status != SERVER_DEAD &&
- kill((pid = ap_scoreboard_image->parent[n].pid), 0) == -1) {
- ap_update_child_status(n, SERVER_DEAD, NULL);
- /* just mark it as having a successful exit status */
- bzero((char *) status, sizeof(ap_wait_t));
- return(pid);
- }
+ pid = ap_scoreboard_image->parent[n].pid;
+ if (ap_scoreboard_image->servers[n].status != SERVER_DEAD) {
+ if (in_pid_table(pid)) {
+ if (kill(pid, 0) == -1) {
+ ap_update_child_status(n, SERVER_DEAD, NULL);
+ /* just mark it as having a successful exit
status */
+ bzero((char *) status, sizeof(ap_wait_t));
+ return(pid);
+ }
+ }
+ else {
+ ap_log_error(APLOG_MARK, APLOG_NOERRNO|APLOG_ERR,
server_conf,
+ "Bad pid (%d) in scoreboard slot %d",
pid, n);
+ }
+ }
}
return 0;
}
@@ -2916,15 +2957,21 @@
#define MAXWAITOBJ MAXIMUM_WAIT_OBJECTS
HANDLE h[MAXWAITOBJ];
int e[MAXWAITOBJ];
- int round, pi, hi, rv, err;
+ int round, pi, hi, rv, err, pid;
for (round = 0; round <= (HARD_SERVER_LIMIT - 1) / MAXWAITOBJ +
1; round++) {
hi = 0;
for (pi = round * MAXWAITOBJ;
(pi < (round + 1) * MAXWAITOBJ) && (pi < HARD_SERVER_LIMIT);
pi++) {
if (ap_scoreboard_image->servers[pi].status != SERVER_DEAD) {
- e[hi] = pi;
- h[hi++] = (HANDLE) ap_scoreboard_image->parent[pi].pid;
+ e[hi] = pi;
+ pid = ap_scoreboard_image->parent[pi].pid;
+ if (in_pid_table(pid))
+ h[hi++] = (HANDLE) pid;
+ else {
+ ap_log_error(APLOG_MARK, APLOG_NOERRNO|
APLOG_ERR, server_conf,
+ "Bad pid (%d) in scoreboard slot %
d", pid, pi);
+ }
}
}
@@ -4339,6 +4386,7 @@
ap_server_pre_read_config = ap_make_array(pcommands, 1, sizeof
(char *));
ap_server_post_read_config = ap_make_array(pcommands, 1, sizeof
(char *));
ap_server_config_defines = ap_make_array(pcommands, 1, sizeof
(char *));
+ pid_table = ap_make_table(pglobal, 100);
}
#ifndef MULTITHREAD
@@ -4987,6 +5035,7 @@
ap_scoreboard_image->parent[slot].last_rtime = now;
#endif
ap_scoreboard_image->parent[slot].pid = pid;
+ set_pid_table(pid);
#ifdef SCOREBOARD_FILE
lseek(scoreboard_fd, XtOffsetOf(scoreboard, parent[slot]), 0);
force_write(scoreboard_fd, &ap_scoreboard_image->parent[slot],
@@ -5049,6 +5098,7 @@
int i;
int to_kill;
int idle_count;
+ int pid;
short_score *ss;
time_t now = time(NULL);
int free_length;
@@ -5113,8 +5163,15 @@
else if (ps->last_rtime + ss->timeout_len < now) {
/* no progress, and the timeout length has been exceeded */
ss->timeout_len = 0;
- kill(ps->pid, SIG_TIMEOUT_KILL);
- }
+ pid = ps->pid;
+ if (in_pid_table(pid)) {
+ kill(pid, SIG_TIMEOUT_KILL);
+ }
+ else {
+ ap_log_error(APLOG_MARK, APLOG_NOERRNO|
APLOG_ERR, server_conf,
+ "Bad pid (%d) in scoreboard slot %d",
pid, i);
+ }
+ }
}
#endif
}
@@ -5126,11 +5183,18 @@
* while we were counting. Use the define SIG_IDLE_KILL to reflect
* which signal should be used on the specific OS.
*/
- kill(ap_scoreboard_image->parent[to_kill].pid, SIG_IDLE_KILL);
- idle_spawn_rate = 1;
+ pid = ap_scoreboard_image->parent[to_kill].pid;
+ if (in_pid_table(pid)) {
+ kill(pid, SIG_IDLE_KILL);
+ idle_spawn_rate = 1;
#ifdef TPF
- ap_update_child_status(to_kill, SERVER_DEAD, (request_rec *)
NULL);
+ ap_update_child_status(to_kill, SERVER_DEAD,
(request_rec *)NULL);
#endif
+ }
+ else {
+ ap_log_error(APLOG_MARK, APLOG_NOERRNO|APLOG_ERR,
server_conf,
+ "Bad pid (%d) in scoreboard slot %d", pid,
to_kill);
+ }
}
else if (idle_count < ap_daemons_min_free) {
/* terminate the free list */
Re: [Fwd: Apache httpd vulenrabilities]
Posted by Jim Jagielski <ji...@jaguNET.com>.
On Jun 1, 2007, at 3:35 PM, Jim Jagielski wrote:
>
> FWIW, I've created a branch of the work in progress, so
> people can follow along and provide comments and patches :)
>
> http://svn.apache.org/repos/asf/httpd/httpd/branches/httpd-pid-
> table
>
this is based off of trunk, so once we have this where we
want it, we'll backport to 2.2 and 2.0
Re: [Fwd: Apache httpd vulenrabilities]
Posted by Jim Jagielski <ji...@jaguNET.com>.
On Jun 1, 2007, at 2:13 PM, Jim Jagielski wrote:
>
> On Jun 1, 2007, at 11:39 AM, Jim Jagielski wrote:
>
>>
>> I will likely just commit the updated patch, and we
>> can fine-tune via commits rather than having rounds
>> of patches posted :)
>>
>
> I just started on the trunk patches, not sure when they
> will be done... anyway, I was think that in addition to
> the checks, it would also be useful if, instead of
> just logging the issue, maybe the code should also
> set the pid to 0 and maybe a different status (maybe
> SERVER_UNKNOWN ?) Haven't thought this all the way
> through yet.
>
> As one would expect the 2.x patches are much larger,
> since things aren't as centralized...
>
FWIW, I've created a branch of the work in progress, so
people can follow along and provide comments and patches :)
http://svn.apache.org/repos/asf/httpd/httpd/branches/httpd-pid-
table
Re: [Fwd: Apache httpd vulenrabilities]
Posted by Jim Jagielski <ji...@jaguNET.com>.
On Jun 1, 2007, at 11:39 AM, Jim Jagielski wrote:
>
> I will likely just commit the updated patch, and we
> can fine-tune via commits rather than having rounds
> of patches posted :)
>
I just started on the trunk patches, not sure when they
will be done... anyway, I was think that in addition to
the checks, it would also be useful if, instead of
just logging the issue, maybe the code should also
set the pid to 0 and maybe a different status (maybe
SERVER_UNKNOWN ?) Haven't thought this all the way
through yet.
As one would expect the 2.x patches are much larger,
since things aren't as centralized...
Re: [Fwd: Apache httpd vulenrabilities]
Posted by Jim Jagielski <ji...@jaguNET.com>.
On Jun 1, 2007, at 10:45 AM, Colm MacCarthaigh wrote:
> On Fri, Jun 01, 2007 at 10:50:09AM -0400, Jim Jagielski wrote:
>>> Should we get rid of it from the table here? Can we get away without
>>> removing stale pids in general? What if they are recycled by the OS
>>> for something else?
>>>
>>
>> No, that's a good point. We should likely remove the
>> pid from our table once the child goes away.
>
> I think we need to do it with an actual table unset call too, not just
> set the entry to "0" or whatever, so that we don't exhaust our table.
>
Yes, agreed.
> On that point, instead of initialising with ap_make_table(pglobal,
> 100)
> , it should probably use HARD_SERVER_LIMIT (if that's not in scope, it
> may even justify making it in scope) instead of 100 too, to avoid
> potential immediate exhaustion.
>
> Nice simple fix though, using a simple table seems like a way better
> approach than trying to replicate a paralell scoreboard.
>
I will likely just commit the updated patch, and we
can fine-tune via commits rather than having rounds
of patches posted :)
Re: [Fwd: Apache httpd vulenrabilities]
Posted by Colm MacCarthaigh <co...@stdlib.net>.
On Fri, Jun 01, 2007 at 10:50:09AM -0400, Jim Jagielski wrote:
> >Should we get rid of it from the table here? Can we get away without
> >removing stale pids in general? What if they are recycled by the OS
> >for something else?
> >
>
> No, that's a good point. We should likely remove the
> pid from our table once the child goes away.
I think we need to do it with an actual table unset call too, not just
set the entry to "0" or whatever, so that we don't exhaust our table.
On that point, instead of initialising with ap_make_table(pglobal, 100)
, it should probably use HARD_SERVER_LIMIT (if that's not in scope, it
may even justify making it in scope) instead of 100 too, to avoid
potential immediate exhaustion.
Nice simple fix though, using a simple table seems like a way better
approach than trying to replicate a paralell scoreboard.
--
Colm MacCárthaigh Public Key: colm+pgp@stdlib.net
Re: [Fwd: Apache httpd vulenrabilities]
Posted by Jim Jagielski <ji...@jaguNET.com>.
On Jun 1, 2007, at 10:19 AM, Colm MacCarthaigh wrote:
> On Fri, Jun 01, 2007 at 10:05:26AM -0400, Jim Jagielski wrote:
>> - if (ap_scoreboard_image->servers[n].status != SERVER_DEAD &&
>> - kill((pid = ap_scoreboard_image->parent[n].pid), 0) == -1) {
>> - ap_update_child_status(n, SERVER_DEAD, NULL);
>> - /* just mark it as having a successful exit status */
>> - bzero((char *) status, sizeof(ap_wait_t));
>> - return(pid);
>> - }
>> + pid = ap_scoreboard_image->parent[n].pid;
>> + if (ap_scoreboard_image->servers[n].status != SERVER_DEAD) {
>> + if (in_pid_table(pid)) {
>> + if (kill(pid, 0) == -1) {
>> + ap_update_child_status(n, SERVER_DEAD, NULL);
>> + /* just mark it as having a successful exit
>> status */
>> + bzero((char *) status, sizeof(ap_wait_t));
>> + return(pid);
>> + }
>
> Should we get rid of it from the table here? Can we get away without
> removing stale pids in general? What if they are recycled by the OS
> for something else?
>
No, that's a good point. We should likely remove the
pid from our table once the child goes away.
Re: [Fwd: Apache httpd vulenrabilities]
Posted by Colm MacCarthaigh <co...@stdlib.net>.
On Fri, Jun 01, 2007 at 10:05:26AM -0400, Jim Jagielski wrote:
> - if (ap_scoreboard_image->servers[n].status != SERVER_DEAD &&
> - kill((pid = ap_scoreboard_image->parent[n].pid), 0) == -1) {
> - ap_update_child_status(n, SERVER_DEAD, NULL);
> - /* just mark it as having a successful exit status */
> - bzero((char *) status, sizeof(ap_wait_t));
> - return(pid);
> - }
> + pid = ap_scoreboard_image->parent[n].pid;
> + if (ap_scoreboard_image->servers[n].status != SERVER_DEAD) {
> + if (in_pid_table(pid)) {
> + if (kill(pid, 0) == -1) {
> + ap_update_child_status(n, SERVER_DEAD, NULL);
> + /* just mark it as having a successful exit
> status */
> + bzero((char *) status, sizeof(ap_wait_t));
> + return(pid);
> + }
Should we get rid of it from the table here? Can we get away without
removing stale pids in general? What if they are recycled by the OS
for something else?
--
Colm MacCárthaigh Public Key: colm+pgp@stdlib.net