You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@httpd.apache.org by "Roy T. Fielding" <fi...@kiwi.ICS.UCI.EDU> on 1997/01/25 03:32:53 UTC

[PATCH] Select/accept parameter reinitialization, bugfix

<ad...@virginia.edu> notified us of a problem with children for the httpd
dump core on SIGSEGV regularly, HPUX, version: 10.10, Apache 1.2b4.
Ben mentioned that the symptoms looked like a stack corruption.

I think this is being caused when an initial call to select or accept
fails (for any reason) and the code attempts to reuse the same parameters
even though they were modified by the call.  In any case, we should be
reinitializing the parameters before each call even if this isn't the
cause of the problem.  Patch follows.

......Roy

Index: http_main.c
===================================================================
RCS file: /export/home/cvs/apache/src/http_main.c,v
retrieving revision 1.110
diff -c -r1.110 http_main.c
*** http_main.c	1997/01/24 21:06:33	1.110
--- http_main.c	1997/01/25 02:22:15
***************
*** 1576,1582 ****
  	    exit(0);
  	}
  
- 	clen=sizeof(sa_client);
  	(void)update_child_status (child_num, SERVER_READY, (request_rec*)NULL);
  	
  	accept_mutex_on();  /* Lock around "accept", if necessary */
--- 1576,1581 ----
***************
*** 1592,1598 ****
  #else
                  csd = select(listenmaxfd+1, &fds, NULL, NULL, NULL);
  #endif
! 		if (csd == -1 && errno != EINTR)
  		    log_unixerr("select",NULL,"select error", server_conf);
  
  		/*fprintf(stderr,"%d check(2a) %d %d\n",getpid(),scoreboard_image->global.exit_generation,generation);*/
--- 1591,1597 ----
  #else
                  csd = select(listenmaxfd+1, &fds, NULL, NULL, NULL);
  #endif
! 		if (csd < 0 && errno != EINTR)
  		    log_unixerr("select",NULL,"select error", server_conf);
  
  		/*fprintf(stderr,"%d check(2a) %d %d\n",getpid(),scoreboard_image->global.exit_generation,generation);*/
***************
*** 1604,1644 ****
  		    if (FD_ISSET(sd, &fds)) break;
  		if (sd < 0) continue;
  
! 		clen=sizeof(sa_client);
! 		do csd=accept(sd, &sa_client, &clen);
! 		while (csd == -1 && errno == EINTR);
! 		if (csd != -1) break;
  		log_unixerr("accept", "(client socket)", NULL, server_conf);
  	    }
! 	} else
! 	    {
  	    fd_set fds;
  
! 	    memset(&fds,0,sizeof fds);
! 	    FD_SET(sd,&fds);
! 	    
! 	    do
  #ifdef HPUX
  		csd = select(sd+1, (int*)&fds, NULL, NULL, NULL);
  #else
  		csd = select(sd+1, &fds, NULL, NULL, NULL);
  #endif
! 	    while(csd < 0 && errno == EINTR);
  
! 	    if(csd < 0)
! 		{
  		log_unixerr("select","(listen)",NULL,server_conf);
  		exit(0);
! 		}
  	    /*fprintf(stderr,"%d check(2a) %d %d\n",getpid(),scoreboard_image->global.exit_generation,generation);*/
  	    sync_scoreboard_image();
  	    if(scoreboard_image->global.exit_generation >= generation)
  		exit(0);
  
! 	    while ((csd=accept(sd, &sa_client, &clen)) == -1) 
! 		if (errno != EINTR) 
! 		    log_unixerr("accept",NULL,"socket error: accept failed", server_conf);
! 	    }
  
  	accept_mutex_off(); /* unlock after "accept" */
  
--- 1603,1646 ----
  		    if (FD_ISSET(sd, &fds)) break;
  		if (sd < 0) continue;
  
!                 do {
!                     clen = sizeof(sa_client);
!                     csd  = accept(sd, &sa_client, &clen);
!                 } while (csd < 0 && errno == EINTR);
!                 if (csd < 0) break;
  		log_unixerr("accept", "(client socket)", NULL, server_conf);
  	    }
! 	}
! 	else {
  	    fd_set fds;
  
! 	    do {
!                 FD_ZERO(&fds);
!                 FD_SET(sd,&fds);
  #ifdef HPUX
  		csd = select(sd+1, (int*)&fds, NULL, NULL, NULL);
  #else
  		csd = select(sd+1, &fds, NULL, NULL, NULL);
  #endif
!             } while (csd < 0 && errno == EINTR);
  
!             if (csd < 0) {
  		log_unixerr("select","(listen)",NULL,server_conf);
  		exit(0);
!             }
  	    /*fprintf(stderr,"%d check(2a) %d %d\n",getpid(),scoreboard_image->global.exit_generation,generation);*/
  	    sync_scoreboard_image();
  	    if(scoreboard_image->global.exit_generation >= generation)
  		exit(0);
  
!             do {
!                 clen = sizeof(sa_client);
!                 csd  = accept(sd, &sa_client, &clen);
!             } while (csd < 0 && errno == EINTR);
!             if (csd < 0)
!                 log_unixerr("accept", NULL, "socket error: accept failed",
!                             server_conf);
! 	}
  
  	accept_mutex_off(); /* unlock after "accept" */
  

Re: [PATCH] Select/accept parameter reinitialization, bugfix

Posted by Marc Slemko <ma...@znep.com>.
On Fri, 24 Jan 1997, Roy T. Fielding wrote:

> <ad...@virginia.edu> notified us of a problem with children for the httpd
> dump core on SIGSEGV regularly, HPUX, version: 10.10, Apache 1.2b4.
> Ben mentioned that the symptoms looked like a stack corruption.
> 
> I think this is being caused when an initial call to select or accept
> fails (for any reason) and the code attempts to reuse the same parameters
> even though they were modified by the call.  In any case, we should be
> reinitializing the parameters before each call even if this isn't the
> cause of the problem.  Patch follows.

Looks good to me.  Note my comment below.


> 
> ......Roy
> 
> Index: http_main.c
> ===================================================================
> RCS file: /export/home/cvs/apache/src/http_main.c,v
> retrieving revision 1.110
> diff -c -r1.110 http_main.c
> *** http_main.c	1997/01/24 21:06:33	1.110
> --- http_main.c	1997/01/25 02:22:15
[...]
>   	    /*fprintf(stderr,"%d check(2a) %d %d\n",getpid(),scoreboard_image->global.exit_generation,generation);*/
>   	    sync_scoreboard_image();
>   	    if(scoreboard_image->global.exit_generation >= generation)
>   		exit(0);
>   
> !             do {
> !                 clen = sizeof(sa_client);
> !                 csd  = accept(sd, &sa_client, &clen);
> !             } while (csd < 0 && errno == EINTR);
> !             if (csd < 0)
> !                 log_unixerr("accept", NULL, "socket error: accept failed",
> !                             server_conf);
> ! 	}
>   
>   	accept_mutex_off(); /* unlock after "accept" */
>   
> 

Are you aware that you are changing what the above loop does?

The existing loop does the accept() until it gets something other than -1
as a return code.  This new one does the accept() until it either gets
something other than -1 or it gets an errno other than EINTR.  The
existing version does seem to be a possible infinite loop though; I'm not 
disagreeing with the change, just making sure it is intended.