我写了一个 多线程 + SOCKET 的程序，但存在严重逻辑错误，请大家帮忙说一下

Question

这是一个数据转发程序（TCP/IP方式）
C表示客户端，P表示转发服务器，S表示服务端
现在C向P发送一条消息，再由P把这条消息发给S
P在接收以后，需要跟S建立连接，并把接收到的信息发给S；
代码如下：P的监听按钮
void CtransferDlg::OnBnClickedBtnStart()
{
// TODO: 在此添加控件通知处理程序代码
//创建Tcp服务器socket
SOCKET sockSrv = socket( AF_INET , SOCK_STREAM , 0 );//服务器地址
SOCKADDR_IN addrSrv ;
addrSrv.sin_addr.S_un.S_addr = htonl(INADDR_ANY) ;
addrSrv.sin_family = AF_INET ;
addrSrv.sin_port = htons(7001) ;//将socket与地址绑定在一起
if(SOCKET_ERROR == bind(sockSrv ,(SOCKADDR*)&addrSrv , sizeof(SOCKADDR)))
{
MessageBox("地址邦定失败!");
return;
}//开始监听客户端请求,最大连接数为5
if(SOCKET_ERROR == listen( sockSrv , 5))
{
MessageBox("监听失败!");
return;
}
//创建监听线程
::CreateThread(NULL, 0, listenThread, (LPVOID)sockSrv, 0, NULL);
//IDC_BTN_START是监听按钮的ID号
GetDlgItem(IDC_BTN_START)->EnableWindow(FALSE);
}监听线程
DWORD WINAPI CtransferDlg::listenThread(LPVOID lpParameter)
{
//用于存放客户端地址
SOCKADDR_IN addrClient ;
int len = sizeof( SOCKADDR_IN );
while(TRUE)
{
SOCKET sockConn = accept((SOCKET)lpParameter, (SOCKADDR *)addrClient , &len);//接收数据
char recvBuf[100] ;
recv(sockConn , recvBuf , 100 , 0);
//关闭socket
closesocket(sockConn);
//连接服务器线程
::CreateThread(NULL, 0, ConnectThread, (LPVOID)recvBuf, 0, NULL);
}
return 0;
}转发器连接服务器线程
DWORD WINAPI CtransferDlg::ConnectThread(LPVOID lpParameter)
{
//用于存放客户端地址
SOCKADDR_IN addrClient ;
int len = sizeof( SOCKADDR_IN );char recvBuf[100] ;
strcpy(recvBuf, (char *)lpParameter);
::MessageBox(NULL, recvBuf, "transfer", MB_OK);//建立客户端socket
SOCKET sockClient = socket( AF_INET ,SOCK_STREAM , 0) ;
//服务器地址
SOCKADDR_IN addrSrv ;
//连接局域网中另外一台计算机
addrSrv.sin_addr.S_un.S_addr = inet_addr("192.168.0.16") ;
addrSrv.sin_family = AF_INET ;
addrSrv.sin_port = htons(7002);
//连接服务端
if(SOCKET_ERROR == connect(sockClient , (SOCKADDR*)&addrSrv , sizeof(SOCKADDR)))
{
::MessageBox(NULL, "转发服务器连接服务端失败", "transfer", MB_OK);
return -1;
}
send(sockClient , recvBuf , strlen(recvBuf)+1 , 0);
closesocket(sockClient);
return 0;
}
程序做简单测试的时候可以正常运行，但一个高手看过我的代码后，给了如下评价：“首先是逻辑上的问题，比方说，
DWORD WINAPI CtransferDlg::listenThread(LPVOID lpParameter)
这个函数有问题，当它接收到一个连接，并收取数据到栈上的一片缓冲区（注意，缓冲区在栈上），然后创建一个新线程来处理这片数据。
这个时候，如果listenThread又收到一个连接并又接收一片数据，就会覆盖掉缓冲区中的内容，而这个时候，新线程甚至很可能工作了一半，或没有开始工作，结果是未知的。其次，你用了阻塞式的RECV，如果客户端一直不发数据，你的线程将一直阻塞，直到SOCKET关闭（或有数据来），这种逻辑肯定不对头，如果有一个恶意的客户端，连接后不发数据，你的服务端就死了，不再响应其他客户端的请求。还有，你处理缓冲区的方式也不对，用了strlen，这个函数太危险，万一缓冲区中的数据不包括零字符，那它返回的值就不可知。”
  请问要解决上面的问题，具体该怎么修改代码？

Answer 1

用一个select 全部就解决了， 如果对性能要求不高，就用一个线程。 如果要求比较高，或者连接多，要多线程池。

Answer 2

bool CProxy::run()
{
fd_set rfd;
// fd_set wfd; timeval   tm;
tm.tv_sec = 0;
tm.tv_usec = 1000;

int maxfd;
maxfd = sock;

int n; dout<<str_time()<<"The server is running ... \n* If you want to stop the server, please press the key \"Esc\"."<<std::endl; for(;/*!(GetKeyState(VK_ESCAPE) & 0x8000) && */!bStop;)
{
if(_kbhit() && getch() == 27)
break; FD_ZERO(&rfd);
// FD_ZERO(&wfd);
FD_SET(sock, &rfd);
{
for(std::list<Client*>::iterator it = wait.begin(); it!=wait.end(); ++it)
{
FD_SET((*it)->socket, &rfd);
}
} {
DWORD dw = ::GetTickCount();
for(std::map<std::string, Client*>::iterator it = id_to_sock.begin(); it!=id_to_sock.end();)
{
if(it->second == NULL)
{
id_to_sock.erase(it++);
continue;
}
if(it->second->is_offline)
{
// test the offline user is or not time out( >180second )
if((dw - it->second->time_offline > 1000 * wait_time) && m_pGameOut != NULL)
{
GNUsrPacket   out;
out << (uint8) ET_OFFLINE_WAIT_END 
<< (uint32)it->second->uid
<< it->first.c_str();
delete  it->second;
id_to_sock.erase(it++);
m_pGameOut->OnPlayerError(out);
continue;
}
}
else
{
FD_SET(it->second->socket, &rfd);
}
++it;
}
} n = select(maxfd + 1, &rfd, NULL, NULL, &tm);

Answer 3

if (n > 0)
{
{
for(std::map<std::string, Client*>::iterator it = id_to_sock.begin(); it!=id_to_sock.end(); ++it)
{
if(it->second == NULL) continue;
if(FD_ISSET(it->second->socket, &rfd))
{
// 读取数据
int ilen = 0;
ilen = recv(it->second->socket, it->second->buf + it->second->buflen, 1024-it->second->buflen, 0); if (ilen == SOCKET_ERROR)
{

int ierr;
switch(ierr=WSAGetLastError())
{
case WSAECONNRESET:
case WSAETIMEDOUT:
case WSAECONNABORTED:
case WSAESHUTDOWN:
case WSAENETDOWN:
{
dout<<str_time()<< "Socket Error : ["<<it->second->socket<<"] , user: ["<< it->first <<"] "<<std::endl;
//std::list<Client>::iterator temp = ;
closesocket(it->second->socket);
it->second->socket = INVALID_SOCKET;
if(m_pGameOut!=NULL)
{
if(it->second->is_player)
{
it->second->is_offline = true;
it->second->time_offline = ::GetTickCount();
GNUsrPacket   out;
out << (uint8) ET_OFFLINE
<< (uint32)it->second->uid
<< it->first.c_str();
m_pGameOut->OnPlayerError(out);
}
else
{
GNUsrPacket   out;
out << (uint8) ET_OFFLINE_WAIT_END 
<< (uint32)it->second->uid
<< it->first.c_str();
delete  it->second;
it->second = NULL;
//it->first = "";
//id_to_sock.erase(it++);
m_pGameOut->OnPlayerError(out); }
}
continue;
}
default:
//continue;
dout<<str_time()<< "Common Socket Error : ["<<it->second->socket<<"] , user: ["<< it->first <<"] error["<<ierr<<"]"<<std::endl;
break;
}

}
else if(ilen == 0)
{
dout<<str_time()<< "Socket was closed : ["<<it->second->socket<<"] , user: ["<< it->first << "] "<<std::endl;
closesocket(it->second->socket);
it->second->socket = INVALID_SOCKET;
if(m_pGameOut!=NULL)
{
if(it->second->is_player)
{
it->second->is_offline = true;
it->second->time_offline = ::GetTickCount();
GNUsrPacket   out;
out << (uint8) ET_OFFLINE
<< (uint32)it->second->uid
<< it->first.c_str();
m_pGameOut->OnPlayerError(out);
}
else
{
GNUsrPacket   out;
out << (uint8) ET_OFFLINE_WAIT_END 
<< (uint32)it->second->uid
<< it->first.c_str();
delete  it->second;
it->second = NULL;
//it->first = "";
//id_to_sock.erase(it++);
m_pGameOut->OnPlayerError(out); }
}
continue; }
else
{
dout<<str_time()<< "Receive "<<ilen<<" bytes : ["<<it->first<<"]" << std::endl;
output_hex(it->second->buf + it->second->buflen, ilen);
//dout<<std::hex<<std::setfill('0')<<std::uppercase;
//for(int i = 0; i < ilen; ++i)
//{
// dout<<std::setw(2)<<(int)(unsigned char)(it->second->buf[i+it->second->buflen]);
// if(i%0x10 == 0xf)
// dout<<std::endl;
// else
// dout<<' ';
//}
//dout<<std::dec<<std::setfill(' ')<<std::endl; it->second->buflen += ilen;
C_RCV: if( it->second->buflen >=sizeof(TCP_HEADER))
{ int   pkl = (int)ntohs(*(u_short*)(it->second->buf));
if(pkl > 1020)  // the packet is too length....error!!!
{
dout<< "* Receive data error(packet length error:"<< pkl <<") : socket["<<it->second->socket<<"] , user: ["<< it->first << "] "<<std::endl;
closesocket(it->second->socket);
it->second->socket = INVALID_SOCKET; 
if(m_pGameOut!=NULL)
{
if(it->second->is_player)
{
it->second->is_offline = true;
it->second->time_offline = ::GetTickCount();
GNUsrPacket   out;
out << (uint8) ET_OFFLINE
<< (uint32)it->second->uid
<< it->first.c_str();
m_pGameOut->OnPlayerError(out);
}
else
{
GNUsrPacket   out;
out << (uint8) ET_OFFLINE_WAIT_END 
<< (uint32)it->second->uid
<< it->first.c_str();
delete  it->second;
it->second = NULL;
//it->first = "";
//id_to_sock.erase(it++);
m_pGameOut->OnPlayerError(out); }
}
continue;
}

Answer 4

if(pkl <= it->second->buflen)
{
TcpInPacket    in(it->second->buf+2,  pkl); if(in.header.ntf == 0xffffffff)
{
/*
prase packet...
*/
switch(in.header.cmd)
{
case 0x3004:
{
dout<<"* Receive a command data packet from client ["<< it->first<<"]..."<<std::endl;
if(m_pGameOut)
{
GNUsrPacket    out;
out << it->first.c_str();
int  l = 0;
uint8* p = in.readData(l);
out.writeData(p,l);
m_pGameOut->OnGameCmd(out);
}

}
break;
case 0x1113:
{
dout<<"* Receive a live packet from client ["<< it->first<<"]..."<<std::endl;
//GNUsrPacket    out; }
break;
default:
dout<<"* Receive a unknow data packet from ["<<it->first<<"], it will be discard..."<<std::endl;
break;
} }
else
{
dout<<"* Receive a error packet from ["<<it->first<<"], header flag error, it will be discard..."<<std::endl;
}
if(it->second != NULL)
{
it->second->buflen -= pkl + 2;
memcpy(it->second->buf, it->second->buf+pkl+2, it->second->buflen);
}
}
}
if(it->second && it->second->buflen >=sizeof(TCP_HEADER)) goto C_RCV;
}
}
//++it;
}
} {
for(std::list<Client*>::iterator it = wait.begin(); it!=wait.end();)
{
if(FD_ISSET((*it)->socket, &rfd))
{
// 读取数据
//dout << "Recv data " << std::endl;
//static char buf[1024];
//static int buflen = 0; int ilen = 0;
ilen = recv((*it)->socket, (*it)->buf+(*it)->buflen, 1024-(*it)->buflen, 0); if (ilen == SOCKET_ERROR)
{

switch(WSAGetLastError())
{
case WSAECONNRESET:
case WSAETIMEDOUT:
case WSAECONNABORTED:
case WSAESHUTDOWN:
case WSAENETDOWN:
{
dout<<str_time()<< "Socket Error : ["<<(*it)->socket<<"]"<<std::endl;
//std::list<Client>::iterator temp = ;
closesocket((*it)->socket);
delete   *it;
wait.erase(it++);
continue;
}
default:
//continue;
break;
}

}
else if(ilen == 0)
{
dout<<str_time()<< "Socket closed : ["<<(*it)->socket<<"]"<<std::endl;
closesocket((*it)->socket);
delete  *it;
wait.erase(it++);
continue; }
else
{
dout<<str_time()<< "Receive ["<<ilen<<"] bytes" << std::endl;
output_hex((*it)->buf+(*it)->buflen,ilen); //dout<<std::hex<<std::setfill('0')<<std::uppercase;
//for(int i = 0; i < ilen; ++i)
//{
// dout<<std::setw(2)<<(int)(unsigned char)((*it)->buf[i+(*it)->buflen]);
// if(i%0x10 == 0xf)
// dout<<std::endl;
// else
// dout<<' ';
//}
//dout<<std::dec<<std::setfill(' ')<<std::endl;
(*it)->buflen += ilen;
C_RCW: if((*it)->buflen>sizeof(TCP_HEADER))
{ int   pkl = (int)ntohs(*(u_short*)((*it)->buf));
if(pkl > 1020)  // the packet is too length....error!!!
{
dout<< "* Receive data error(packet length error:"<< pkl <<") : socket["<<(*it)->socket<<"]"<<std::endl;
closesocket((*it)->socket);
delete  *it;
wait.erase(it++);
continue; }
if(pkl <= (*it)->buflen)
{
TcpInPacket    in((*it)->buf+2,  pkl);
if(in.header.ntf == 0xffffffff && in.header.cmd == 0x3003)
{
GN_STR  id, password;
in >> id >> password;
dout<< "* Receive user login command: user id :["<<id.text
<<"], password ["<<password.text<<"]"<<std::endl; (*it)->buflen -= pkl + 2;
memcpy((*it)->buf, (*it)->buf+pkl+2, (*it)->buflen); std::string  user_name(id.text);
enum emUserType  ut = check_user(id.text, password.text);
switch(ut)
{
case UT_PLAYER:
case UT_LOOKER:
{
TcpOutPacket  out;
InitPacket(out, 0x3004);
out.send((*it)->socket);
} (*it)->is_player = (ut == UT_PLAYER)?1:0;  if(id_to_sock.find(user_name) != id_to_sock.end() && id_to_sock[user_name] != NULL)
{
if(id_to_sock[user_name]->is_offline)
{
dout << "* The user ["<< user_name << "] is relink successed."<<std::endl;
}
else
{
dout<<"* The user ["<< user_name <<"] already logined !!! "<<std::endl;
if(m_pGameOut!=NULL)
{
GNUsrPacket   out;
out << (uint8) ET_PLAYER_REENTER
<< (uint32)id_to_sock[user_name]->uid
<< user_name.c_str();
m_pGameOut->OnPlayerError(out);
}
}
delete id_to_sock[user_name];
}
else
{
// new user }
id_to_sock[user_name] = *it;
wait.erase(it++);
if(m_pGameOut!=NULL)
{
if(ut == UT_PLAYER)
{
m_pGameOut->OnPlayerEnter(find_player(id.text)+1);
}else{
GNUsrPacket   out;
int idir = ::GetPrivateProfileInt("looker", id.text, -1, inf_file.c_str());
if(idir<0 || idir >= players.size()) idir = rand()%players.size();
out << (uint8) idir;
get_player_info(id.text, out);
m_pGameOut->OnPlayerLook(out);
}
}
break;
default:
dout << "* Login falid ! the user is no exsit or password error !"<<std::endl; {
TcpOutPacket  out;
InitPacket(out, 0x3005);
out.send((*it)->socket);
} closesocket((*it)->socket);
delete  *it;
wait.erase(it++);
break;
}
continue; }
else
{
(*it)->buflen -= pkl + 2;
memcpy((*it)->buf, (*it)->buf+pkl+2, (*it)->buflen);
}
}
} if((*it)->buflen>sizeof(TCP_HEADER)) goto C_RCW;
}
}
++it;
}
}
if (FD_ISSET(sock, &rfd))
{
Client*   cn = new Client;
int il = sizeof(cn->s_in);
if((cn->socket = accept(sock,(sockaddr*)&cn->s_in, &il)) != INVALID_SOCKET)
{
dout<<str_time()<< "come from IP : "<< inet_ntoa(cn->s_in.sin_addr) << std::endl;
wait.push_back(cn);
}else
delete cn;

} }
else
if (n < 0)
{
dout<<str_time()<<"A internal error was happend !"<<std::endl;

break;

}
}
if(!bStop)
{
dout << str_time() << "The server was force broken by user!" << std::endl;
if(m_pGameOut!=NULL)
{
m_pGameOut->OnServerShutdown();
m_pGameOut->Release();
m_pGameOut = NULL;
}
} return !bStop;
}

Answer 5

1、线程同步问题
2、krh2001(边城浪子) 所说：使用select函数可以解决，可以自定义超时
3、用一个变量记录 数据(收到的）长度

Answer 6

这是我以前写的一个测试用游戏代理服务器中的关键代码部分.不是临时写的.上面的代码只使用了一个线程. 通过select完成了SOCKET监听 与客户端通讯等所有的功能.  单线程. 也没有同步问题. 很安全的模式. 适用于连接数不多. 对性能要求不高的场合.
如果性能要求高的话. 应该使用单独的一个线程去做监听.用一个线程池去完成与客户端SOCKET的数据收发.用另一个线程池去完成 对接收到的数据的处理.用第三个线程池去完成与远程服务器的交互操作.

Answer 7

１　ｌｉｓｔｅｎ放在你的ＬｉｓｔｅｎＴｈｒｅａｄ里面进行，就是先开启线程，然后再线程里Ｌｉｓｔｅｎ
２　ConnectThread里完成Ｒｅｃｖ的工作
３　ｓｔｒｌｅｎ　可以自己实现，传入参数时添加一个缓冲区大小参数，这个你可以参考一下
ｖｓ.ｎｅｔ２００５　，已经有相应库函数出现