Sign in to follow this  

[MUD] Server code bug

This topic is 2953 days old which is more than the 365 day threshold we allow for new replies. Please post a new topic.

If you intended to correct an error in the post then please contact us.

Recommended Posts

Hello folks, The following server code has a bug resulting a runtime error because erasing iterator from a vector makes the iterator invalid (which is then decremented with itr--):
#include "winsock2.h"
#include "Ws2tcpip.h"
WSADATA g_wsadata;      // winsock data holder
#define CloseSocket closesocket
#define GetSocketError WSAGetLastError
#define StartSocketLib WSAStartup( MAKEWORD( 2, 2 ), &g_wsadata );
#define CloseSocketLib WSACleanup();

#ifndef socklen_t
      typedef int socklen_t;
#endif

int main() {
int err;                    
    int lsock;                 
    vector<int> socketlist;    
    StartSocketLib;
    lsock = socket( AF_INET, SOCK_STREAM, IPPROTO_TCP );
    if( lsock == -1 )
    {
        cout << "Socket creation error!" << endl;
        return 0;
    }
    cout << "Socket created!" << endl;
    struct sockaddr_in socketaddress;
    socklen_t sa_size = sizeof( struct sockaddr_in );
    socketaddress.sin_family = AF_INET;
    socketaddress.sin_port = htons( 4000 );
    socketaddress.sin_addr.s_addr = htonl( INADDR_ANY );
    memset( &(socketaddress.sin_zero), 0, 8 );
    err = bind( lsock, (struct sockaddr*)&socketaddress, sa_size );
    if( err == -1 )
    {
        cout << "Socket binding error!" << endl;
        return 0;
    }
    cout << "Socket bound!" << endl;
    err = listen( lsock, 16 );
    if( err == -1 )
    {
        cout << "Socket listening error!" << endl;
        return 0;
    }
    cout << "Socket listening, waiting for connection..." << endl;
    fd_set rset;
    int i;
    struct timeval zerotime;
    zerotime.tv_usec = 0;
    zerotime.tv_sec = 0;
    char buffer[128];           
    bool done = false;          
    vector<int>::iterator itr;
    while( !done )
    {
        FD_ZERO( &rset );
        FD_SET( lsock, &rset );
        for( itr = socketlist.begin(); itr != socketlist.end(); itr++ )
        {
            FD_SET( *itr, &rset );
        }
        i = select( 0x7FFFFFFF, &rset, NULL, NULL, &zerotime );
        if( i > 0 )
        {
            if( FD_ISSET( lsock, &rset ) )
            {
                // incomming connection
                cout << "Incomming connection..." << endl;
                int dsock = accept( lsock, 
                                    (struct sockaddr*)&socketaddress, 
                                    &sa_size );
                if( dsock == -1 )
                {
                    cout << "Socket accepting error!" << endl;
                    return 0;
                }
                cout << "Socket " << dsock << " accepted." << endl;
                
                // add the socket to the list
                socketlist.push_back( dsock );
            }
            for( itr = socketlist.begin(); itr != socketlist.end(); itr++ )
            {
                if( FD_ISSET( *itr, &rset ) )
                {
                    cout << "receiving data from socket ";
                    cout << *itr << "..." << endl;
                    err = recv( *itr, buffer, 128, 0 );

                    if( err == -1 )
                    {
                        cout << "Socket receiving error!" << endl;
                        return 0;
                    }
                    if( err == 0 )
                    {
                        cout << "Socket " << *itr << " closed" << endl;
                        shutdown( *itr, 2 );
                        CloseSocket( *itr );
                        socketlist.erase( itr );  // BUG WITH RUNTIME ERROR
                        itr--;
                    }
                    else
                    {
                        cout << "Data: " << buffer << endl;
                        if( strcmp( buffer, "servquit" ) == 0 )
                            done = true;
                    }
                }
            }
        }
    }
    shutdown( lsock, 2 );
    CloseSocket( lsock );
    for( i = 0; i < socketlist.size(); i++ )
    {
        shutdown( socketlist[i], 2 );
        CloseSocket( socketlist[i] );
    }
    CloseSocketLib;
    return 0;
}


I revised the while (!done) loop with the following one, but it never goes to shutdown(lsock, 2) when clients data sockets closes. What did I miss? Thanks, Robert
while( !done )
{
        FD_ZERO( &rset );
        FD_SET( lsock, &rset );
        for( itr = socketlist.begin(); itr != socketlist.end(); itr++ )
        {
            FD_SET( *itr, &rset );
        }   
        i = select( 0x7FFFFFFF, &rset, NULL, NULL, &zerotime );
        if( i &gt; 0 )
        {
            if( FD_ISSET( lsock, &rset ) )
            {
                cout &lt;&lt; "Incomming connection..." &lt;&lt; endl;
                int dsock = accept( lsock, 
                                    (struct sockaddr*)&socketaddress, 
                                    &sa_size );
                if( dsock == -1 )
                {
                    cout &lt;&lt; "Socket accepting error!" &lt;&lt; endl;
                    return 0;
                }
                cout &lt;&lt; "Socket " &lt;&lt; dsock &lt;&lt; " accepted." &lt;&lt; endl;
                socketlist.push_back( dsock );
	    }
            itr = socketlist.begin();
            while (itr != socketlist.end() )
	    {
                if( FD_ISSET( *itr, &rset ) )
		{
                    cout &lt;&lt; "receiving data from socket ";
                    cout &lt;&lt; *itr &lt;&lt; "..." &lt;&lt; endl;
                    err = recv( *itr, buffer, 128, 0 );
                    if( err == -1 )
                    {
                        cout &lt;&lt; "Socket receiving error!" &lt;&lt; endl;
                        return 0;
                    }
                    if( err == 0 )
                    {
                        cout &lt;&lt; "Socket " &lt;&lt; *itr &lt;&lt; " closed" &lt;&lt; endl;
                        shutdown( *itr, 2 );
                        CloseSocket( *itr );
//Fix: itr incremented to the elem after the one being erase, no itr++ needed
                        itr = socketlist.erase( itr );	
                    }
                    else
                    {
                        cout &lt;&lt; "Data: " &lt;&lt; buffer &lt;&lt; endl;
                        if( strcmp( buffer, "servquit" ) == 0 )
                            done = true;
			itr++;
                    }
	    }
	    else 
		itr++;
}			// while loop

[Edited by - Robert007 on November 10, 2009 9:42:06 AM]

Share this post


Link to post
Share on other sites
The iterator is invalid as soon as you erase from the vector. So, when you call erase on a vector with the current iterator, set the iterator to equal the return value from erase instead of doing the usual increment you do each time through the loop. That will point to the next valid object in the vector or the end() position.

EDIT: Too slow. That'll teach me for opening several tabs at once.

Share this post


Link to post
Share on other sites
Why does my fix not work? According to doc on erase, when erasing an iterator, it returns an iterator pointing to the element after the element being erased or to end(). The following one should work for the erase, correct?


vector&lt;int&gt;::iterator itr,tmp;
...
if( err == 0 )
{
cout &lt;&lt; "Socket " &lt;&lt; *itr &lt;&lt; " closed" &lt;&lt; endl;
shutdown( *itr, 2 );
CloseSocket( *itr );
tmp = socketlist.erase( itr );
itr = tmp; // implies: itr++
}



If this is the case, can anyone tell me where my fix (it logis) is wrong?

EDIT: after client runs to its completion, the server output "Socket 1912 closed" with listening socket open instead of closing it and exiting.

Thanks again,
Robert

[Edited by - Robert007 on November 10, 2009 9:35:29 AM]

Share this post


Link to post
Share on other sites
My fix as follows works:


vector<int>::iterator itr;
itr = sockelist.begin()
while (itr != socketlist.end() )
{
...
if( err == 0 )
{
cout << "Socket " << *itr << " closed" << endl;
shutdown( *itr, 2 );
CloseSocket( *itr );
itr = socketlist.erase( itr );
}
...
}



When testing on client, I always used "quit". This only closes data sockets on the server. To close both data and listening sockets on the server, I should have entered "servquit".

Now my code works.

Thanks,
Robert


[/source]

[Edited by - Robert007 on November 10, 2009 9:12:21 AM]

Share this post


Link to post
Share on other sites
OK someone correct me if I'm wrong here.

MSDN documentation on recv says

Quote:

If no error occurs, recv returns the number of bytes received. If the connection has been gracefully closed, the return value is zero. Otherwise, a value of SOCKET_ERROR is returned, and a specific error code can be retrieved by calling WSAGetLastError.


It seems to me that the calls to shutdown() and CloseSocket() are unneeded (at least where you are handling them), since recv is already telling you it has been closed.

[source lang="cpp]
if( err == 0 )
{
std::cout << "Socket " << *itr << " closed" << std::endl;
shutdown( *itr, 2 );
CloseSocket( *itr );
itr = socketlist.erase( itr );
}




Are you getting the runtime error from the iterator, or maybe one of these functions since your doing operations on what might be an already closed socket.

Share this post


Link to post
Share on other sites

This topic is 2953 days old which is more than the 365 day threshold we allow for new replies. Please post a new topic.

If you intended to correct an error in the post then please contact us.

Create an account or sign in to comment

You need to be a member in order to leave a comment

Create an account

Sign up for a new account in our community. It's easy!

Register a new account

Sign in

Already have an account? Sign in here.

Sign In Now

Sign in to follow this