Re: Problem with pthreads on socket

[Jan-Benedict Glaw <jbglaw@lug-owl.de>]

[-- Attachment #1: Type: text/plain, Size: 8560 bytes --]

On Sat, 2004-11-06 02:45:41 -0800, Linux Kernel <linuxkrnl@yahoo.com>
wrote in message <20041106104541.53959.qmail@web90004.mail.scd.yahoo.com>:
> int main()	
> 	struct sockaddr_in my_addr;    /* my address
> information */
>         struct sockaddr_in their_addr; /* connector's
> address information */
>         	
> 	static int sockfd;
> 	int th_res=0;
> 	pthread_t thread;
> 	pthread_attr_t attr;
> 	int sin_size,yes=1,new_fd=0;
> 	char* mPath=NULL;
> 	MyARGV* marg;
> 	fd_set fd_read;
> 	void* mRetTh=NULL;
> 	int tCount=0,res_lock;
> 	T_THREAD mThread[10000];
> 	
> 		
> 	mPath=(char*)malloc(256);
> 	marg=(MyARGV*)malloc(sizeof(MyARGV)+256);	    

Why do you allocate a magic number of bytes more than needed? A "MyARGV"
pointer should exactly point to such a struct, not to something that
looks a bit like it :-)

> 	mPath=getenv((const char*)"MY_PATH");	

This is a plain memory leak. Two lines above, you've allocated 256 bytes
(by the way, there's some error checking missing). Here, you throw aray
your pointer to your allocated memory and substitute it for a pointer to
some environment variable. That is, you don't have any chance to ever
free() the first 256 bytes.

> 	
> 	if ((sockfd = socket(AF_INET, SOCK_STREAM, 0)) == -1)
> {
>             perror("socket");
>             exit(1);
>         }
> 	
> 	if
> (setsockopt(sockfd,SOL_SOCKET,SO_REUSEADDR,&yes,sizeof(int))==-1)
> 	{
> 	    perror("SET_SOCK_OPT : \n");
> 	}
> 	
>         my_addr.sin_family = AF_INET;         /* host
> byte order */
>         my_addr.sin_port = htons(MYPORT);     /*
> short, network byte order */
>         my_addr.sin_addr.s_addr = INADDR_ANY; /*
> auto-fill with my IP */
>         bzero(&(my_addr.sin_zero), 8);        /* zero
> the rest of the struct */

Don't ever depend on the order of elements in any structs. Just zero out
the whole struct before you assign any needed values.

>         if (bind(sockfd, (struct sockaddr *)&my_addr,
> sizeof(struct sockaddr)) \
>                                                       
>                == -1) {
>             perror("bind");
>             exit(1);
>         }
> 
>         if (listen(sockfd, 1) == -1) {
>             perror("listen");
>             exit(1);
>         }
> 	
> 		
> 	FD_ZERO(&fd_read);
> 	FD_CLR(sockfd,&fd_read);
> 	FD_SET(sockfd,&fd_read);
> 	
>         while(1) {  /* main accept() loop */
>             sin_size = sizeof(struct sockaddr_in);
>            
> res_lock=select(sockfd+1,&fd_read,NULL,NULL,NULL);

This is another bug. You need to (re-)initialize fd_read each time
before you call select(). By the way -- why do you select() select here
at all? You seem to not use the result afterwards...

> 	    if ((new_fd = accept(sockfd, (struct sockaddr
> *)&their_addr,&sin_size)) == -1) {
>             	perror("accept");
>                 continue;
>             }
>             
>             marg->s=fcntl(new_fd,F_DUPFD,0);
> 	    marg->mPath=mPath;
> 	    marg->their_addr=their_addr;

No need to dup the fd -- just give it to the thread in spe.

> 	    
> 	    pthread_attr_init(&attr);
> 	   
> pthread_attr_setdetachstate(&attr,PTHREAD_CREATE_DETACHED);

This can be done easier: just call pthread_create() and pthread_detach()

> th_res=pthread_create(&thread,&attr,(void*)MyProcess,(void*)marg);

You'd have _one_ "thread" per thread, not one for each attempt to create
one! Basic trick: have a struct per client, like this:

struct client {
	struct pthread_t me;
	int fd;
	struct client *next;
};

or something like that. Write two functions (one to alloc such a struct,
fill in "fd", create thread and put it into a linked list of clients;
one to clean-up a client struct) to handle it.

By the way -- if you have a thread function of correct type, you don't
need to cast it!

> 	    pthread_attr_destroy(&attr);

Not for on-stack attr's IIRC.

> 	    if(th_res==0)
> 	    {
> 		tCount++;
> 		printf("TH COUNT : %d\n",tCount);
> 		mThread[tCount-1].tIDX=tCount;
> 		mThread[tCount-1].tID=thread;		

Statically allocated array with no tCound overflow check. Will bomb out
within 0.01sec if correctly hammered!

> 		pthread_join(thread,&mRetTh);

_Either_ detach, or join.

> 		//close(new_fd); if i close the socket , accept will
> create the same hnd_new_sock like previous...bad
> ideea.

Perfectly okay to get the same fd number a second time. If one
connection to a client is closed, the fd may be re-used for the next
client. This is why even an apache running for hundreds of days
typically doesn't exceed 1000 fds ...


> 	    	
> 	    }
> 	    else
> 	    {
> 	    	perror("CREATE_THREAD");	
> 	    }
> 	    
>         }
>     }
> 
> }
> void MyProcess(MyARGV* MArg){	
> 	int maxtry=0,mTime=0;
> 	int lenrecv=0,lenwrite=0,mCount=1,dCnt=0;
> 	int MyWait=0;
> 	char* mBuff=NULL;
> 	char* realPath=NULL;
> 	unsigned char *mcmd;
> 	fd_set fd_read;
> 	struct timeval tv;
> 	
> 	int msize=BUFF_SIZE,t;
> 	STR_RECV *ss;
> 	MYSQL *myDATA;
> 		
> 		pthread_detach(pthread_self());    

Another detach? You already detached this thread in the main()
function...

> 		t=setsockopt(MArg->s,SOL_SOCKET,SO_SNDBUF &&
> SO_RCVBUF,&msize,sizeof(msize));
> 		mBuff=(char*)malloc(BUFF_SIZE);
> 		mcmd=(unsigned char*)malloc(4);	

Magic constants tend to break:-)

> 		    ss=(STR_RECV*)malloc(sizeof(STR_RECV)+1);		

Why this extra byte?

> 		    strcpy(ss->mSYS_PATH,MArg->mPath);

Missing length check

> 		    for(;;)
> 		    {
> 			maxtry++;
> 			if(maxtry==MAX_TRY)
> 			{
> 			    exit(0);

It's a thread -- just kill it.

> 			}
> 			
> 			    	pthread_mutex_lock(&mmutex);

Haven't seen it anywhere. Keep in mind that it needs initialization
prior first use.

> lenrecv=recv_data(MArg->s,mBuff,mTime,MArg->their_addr,MyWait);
> 			   	pthread_mutex_unlock(&mmutex);
> 				
> 				if(lenrecv>0)
> 			   	{
> 			   		
> 					PutDataInStruct(mBuff,lenrecv,ss);
> 			   	
> 			   	
> 					if(ss->isData==TRUE)
> 			   		{
> 						mCount++;
> 									
> 						lenwrite=mFile(ss->mfData,realPath,lenrecv-4);
> 						if(lenwrite>0)                       
> 						{
> 				    			
> 							ss->TYPE_CMD=CLI_CMD_NEXT_PACK;
> 							ss->USER_CMD=ss->PACKIDX+1;
> 							myAlignInt(ss->TYPE_CMD,ss->USER_CMD,mcmd);
> 							pthread_mutex_lock(&mmutex);
> 							send_cmd(MArg->s,0,mcmd);
> 							pthread_mutex_unlock(&mmutex);
> 				    				
> 						}	
> 				
> 		
> 		    
> 						//close(new_fd);
> 		    				//close(recvsock);	

Keep in mind that a network server typically deals with two types of
file descriptors (with regard to it's clients):

- The socket() file descriptor which basically is your start to work as
  a server at all
- The accept() file descriptors -- one for each client, for each
  connection. However, you may get a given number very ofter: they may
  be re-assigned if they were closed before.

>     		    	    		}
> 					else
> 			    		{
> 						myAlignInt(ss->TYPE_CMD,ss->USER_CMD,mcmd);
> 						pthread_mutex_lock(&mmutex);
> 						send_cmd(MArg->s,10,mcmd);
> 						pthread_mutex_unlock(&mmutex);
> 					}
> 					memset(mBuff,0x0,BUFF_SIZE);
> 					memset(mcmd,0x0,4);
> 					ss->TYPE_CMD=0;
> 					ss->USER_CMD=0;
> 					memset(ss->mfData,0x0,lenrecv);
> 					mTime=0x0;
> 					lenwrite=0;
> 				    	maxtry=0;
> 				}
> 			   	else
> 			   	{
> 					mTime=0;
> 					
> 					FD_CLR(MArg->s,&fd_read);
> 					FD_ZERO(&fd_read);
> 					FD_SET(MArg->s,&fd_read);
> 					tv.tv_sec=60;
> 					tv.tv_usec=0;
> 					pthread_mutex_lock(&mmutex);
> 					select(MArg->s+1,&fd_read,NULL,NULL,&tv);
> 					pthread_mutex_unlock(&mmutex);
> 				}
> 			   	
> 			   	
> 
> 		    }    
> 
>             mBuff=NULL;
> 	    mcmd=NULL;
> 	    free(realPath);
> 	    free(mBuff);
> 	    
> 	    close(MArg->s);
> 	    
> 	    free(MArg);
> 	      
> 	    pthread_exit((void*)1);
> 	   
> }

Since I don't know the rest of the application, I don't comment on the
malloc()/free() usage and the locking about resources...

MfG, JBG

-- 
Jan-Benedict Glaw       jbglaw@lug-owl.de    . +49-172-7608481             _ O _
"Eine Freie Meinung in  einem Freien Kopf    | Gegen Zensur | Gegen Krieg  _ _ O
 fuer einen Freien Staat voll Freier Bürger" | im Internet! |   im Irak!   O O O
ret = do_actions((curr | FREE_SPEECH) & ~(NEW_COPYRIGHT_LAW | DRM | TCPA));

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 189 bytes --]