Re: cvs commit: httpd-2.0/server/mpm/prefork prefork.c

[Jeff Trawick <tr...@attglobal.net>]

gregames@apache.org writes:

> gregames    01/10/11 09:28:23
> 
>   Modified:    server/mpm/prefork prefork.c
>   Log:
>   fix another seg fault.
>   @@ -663,12 +663,14 @@
>    		}
>    		first_lr=lr;
>    		do {
>   -                    apr_os_sock_get(&sockdes, lr->sd);
>   -		    if (FD_ISSET(sockdes, &main_fds))
>   -			goto got_listener;
>   -		    lr = lr->next;
>   -		    if (!lr)
>   -			lr = ap_listeners;
>   +                    if (lr->active) {
>   +                        apr_os_sock_get(&sockdes, lr->sd);
>   +		        if (FD_ISSET(sockdes, &main_fds))
>   +			    goto got_listener;
>   +                        lr = lr->next;
>   +                        if (!lr)
>   +                            lr = ap_listeners;
>   +                    }

I think that what needs to happen is to get inactive records out of
the ap_listeners list.

We still have some ugliness and inefficiencies in the other MPMs
caused by exposing inactive ap_listen_recs to the MPMs.  Maybe we
don't care as long as it doesn't segfault.

The only reason I can think of that we keep the inactive records
around is to re-use the storage on the next restart in case that
listener can be initialized.  The storage is from a pool that is never
released, so we have to be careful that we don't leak every time we
restart.

It would be fairly simple to malloc()/free() the ap_listen_rec in
order to avoid the bogosity we force on the MPM.  However, this may
not be the complete issue because this might allow leakage of an
apr_sockaddr_t.

So even though I've looked at it, I don't have a clear picture of what
has to be done to get the code as simple as possible yet avoid a leak
on restart.

Now maybe we don't care if we leak when one of the listeners doesn't
initialize.  That opens up a certain simplification.

Another alternative is that we keep the inactive ones in a different
list.  We can still re-use the ap_listen_rec and apr_sockaddr_t* when
we restart and try again to set up the socket, but because it is in a
different list we don't have to worry the MPM about such details.

*I'm not sure we re-use this currently.  That needs to be looked at.

-- 
Jeff Trawick | trawick@attglobal.net | 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/server/mpm/prefork prefork.c

[Greg Ames <gr...@remulak.net>]

Jeff Trawick wrote:
> 
> gregames@apache.org writes:
> 
> > gregames    01/10/11 09:28:23
> >
> >   Modified:    server/mpm/prefork prefork.c
> >   Log:
> >   fix another seg fault.
> >   @@ -663,12 +663,14 @@
> >               }
> >               first_lr=lr;
> >               do {
> >   -                    apr_os_sock_get(&sockdes, lr->sd);
> >   -               if (FD_ISSET(sockdes, &main_fds))
> >   -                   goto got_listener;
> >   -               lr = lr->next;
> >   -               if (!lr)
> >   -                   lr = ap_listeners;
> >   +                    if (lr->active) {
> >   +                        apr_os_sock_get(&sockdes, lr->sd);
> >   +                   if (FD_ISSET(sockdes, &main_fds))
> >   +                       goto got_listener;
> >   +                        lr = lr->next;
> >   +                        if (!lr)
> >   +                            lr = ap_listeners;
> >   +                    }
> 
> I think that what needs to happen is to get inactive records out of
> the ap_listeners list.

For sure.  I was thinking the same when I hit this seg fault, but took
the easy way out so I could move on to other bugs.

Greg