Aborted (core dumped) removing element from vector in c++

-1

I'm trying to learn C++ using "MUD Game Programming" and I am working through the examples, but when I try to erase a connection from a vector I get an error: "Aborted (core dumped)." This usually happens when erasing the last one from the vector. I have tried solutions such as checking to see if the iterator != connlist.end() before deleting, and trying to use connlist.clear() if there is only one element left in the list, but these don't seem to help.

I am using g++ to compile and working on Cygwin.

The error happens at connlist.erase(), which you can see is called when the user types "quit."

void NMudServer::StartListening() {

    fd_set read_set;
    std::vector<SocketLib::Connection>::iterator itr;       
    std::vector<SocketLib::Connection>::iterator itr2;  
    TIMEVAL zerotime;
    zerotime.tv_usec = 0;
    zerotime.tv_sec = 0;
    char buffer[buf_len];
    int err;
    lsock.Listen( default_port );

    if( lsock.IsListening() ) {
        std::cout << "Telnet listening on port " << default_port << "." << std::endl;
    } else {
        std::cout << "Could not start Telnet listening socket! - Last error code: " << WSAGetLastError() << std::endl;
        return;
    }

    while( lsock.IsListening() ) {
        FD_ZERO( &read_set );
        FD_SET( lsock.GetSock(), &read_set );

        for( itr = connlist.begin(); itr != connlist.end(); ++itr ) {
            FD_SET( itr->GetSock(), &read_set );
        }
    
        int sel = select( 0x7FFFFFFF, &read_set, NULL, NULL, &zerotime );           

        if( sel > 0 ) {

            if( FD_ISSET( lsock.GetSock(), &read_set ) ) {
                SocketLib::DataSocket dsock = lsock.Accept();
                SocketLib::Connection conn( dsock, buf_len );           
                connlist.push_back( conn );
                conn.Send( "Hello!\r\n", 8 );
            }           

            for( itr = connlist.begin(); itr != connlist.end(); ++itr ) {

                if( FD_ISSET( itr->GetSock(), &read_set ) ) {

                    err = itr->Receive();

                    if( err == -1 ) {
                        std::cout << "Socket receiving error!" << std::endl;
                        std::cout << "Error code: " << WSAGetLastError() << std::endl;
                        std::cout << "Exiting due to error." << std::endl;
                        CloseAllConnections(); // This is when the connection is closed. Need to only close the one connection.
                        break;
                    } else if( err == 0 ) {
                        itr->Close();
                        connlist.erase( itr );
                        --itr;
                    } else if( itr->IsReady() ) {
                        int size;
                        size = itr->GetData( buffer );

                        if( strcmp( buffer, "servquit\r\n" ) == 0 ) {
                            CloseAllConnections();
                        } else if( strcmp( buffer, "quit\r\n" ) == 0 ) {
                            itr->Close();
                            connlist.erase( itr ); // When you go to erase the last element, it errors
                            --itr;
                        } else {
                            // Echo back the data to all connections
                            for( itr2 = connlist.begin(); itr2 != connlist.end(); ++itr2 ) {                                        

                                if( itr2->GetSock() != itr->GetSock() ) {

                                    int err2;
                                    itr2->Send( buffer, size );

                                    if( err2 == -1 ) {
                                        std::cout << "Socket sending error: " << WSAGetLastError() << std::endl;
                                    }
                                } else {
                                    itr2->Send( "\r\n", 2 );
                                }

                            } // end for
                        }// end if-else

                        itr->Reset();

                    } // end if-else-else

                } // end if
            } // end for
        } // end if sel
    } // end while
}

void NMudServer::CloseAllConnections() {
    lsock.Close();

    std::vector<SocketLib::Connection>::iterator itr;
    for( itr = connlist.begin(); itr != connlist.end(); ++itr ) {
        itr->Close();
    }

    WSACleanup();
}

For reference, the book is: Penton, Ron. MUD Game Programming. Boston, MA, USA: Course Technology / Cengage Learning, 2003.

c++
vector
berkeley-sockets
asked on Stack Overflow Nov 5, 2020 by noworld

1 Answer

2

You are iterating a vector and erasing elements from it inside the loop. That's never a good idea. vector::erase invalidates the iterator you then use to iterate further.

See: https://en.cppreference.com/w/cpp/container/vector/erase

Reason for this is that a vector might reallocate and/or move elements when you erase an element.

Possible solution:

Just close those connections inside that loop and perform an erase-remove afterwards:

connlist.erase(std::remove_if(connlist.begin(), connlist.end(), is_closed), connlist.end());

with is_closed being something like:

is_closed = [](const SocketLib::Connection& c) {return !c.open();};

PS: I don't know that SocketLib so you might change that is_closed a bit

PPS: use range-based for loops like for(auto& connection : connlist) {...}

Edit:

As mentioned in a comment, the vector will not reallocate in this case, the other points still stand.

answered on Stack Overflow Nov 5, 2020 by Stefan Riedel • edited Nov 5, 2020 by Stefan Riedel

User contributions licensed under CC BY-SA 3.0