0

In a project I am currently doing in group, we have to build a card game from scratch that uses sockets (Linux). We also have to build a chat room that every player can use.

So far so good. The chat is implemented using three separate threads, one that receives incoming connections (up to 50) and stores them in a client list, one that constantly waits for messages from all connected clients, and one that is created each time a client sends a message, sending that message to all clients in the client list. All of this works, except when a single client disconnects.

I managed to keep the server alive (with a sig handler for SIGPIPE) when a client disconnects, but now, when a client disconnects, I keep getting the error Bad file descriptor. But that's not the only problem, since the server keeps receiving empty messages and sends them to the remaining clients, effectively flooding the whole chat in a matter of milliseconds with empty messages.

I believe that if I can fix the problem on the server side, there won't be any problems on the client side.

So my question is: What is the right way (or any way) to manage a Bad file descriptor in my case. I've already tried closing the socket FD and setting the value to -1 in the client list, but that created even more problems and didn't fix the initial ones.

Here is the code, if necessary. The most important function (for the chat) are reception_thread, chat_thread, receive_string, send_string and connect_to_chat on the client side.

Here is the client:

//includes

const int PORT = 2477;
const int CHAT_PORT = 2478;
#define DEBUG

//error()

// Sets up the connection to the server.

//connect_to_server()

int connect_to_chat(char * hostname)
{

#ifdef DEBUG
    printf("[DEBUG] Initiating connection to chat server.\n");
#endif 


    struct sockaddr_in serv_addr;
    struct hostent *server;

    // Get a socket.
    int sockfd = socket(AF_INET, SOCK_STREAM, 0);

    if (sockfd < 0)
        error("Error opening socket for server.");

    // Get the address of the server.
    server = gethostbyname(hostname);

    if (server == NULL) {
        fprintf(stderr, "ERROR, no such host\n");
        exit(0);
    }

    // Zero out memory for server info.
    memset(&serv_addr, 0, sizeof (serv_addr));

    // Set up the server info.
    serv_addr.sin_family = AF_INET;
    memmove(server->h_addr, &serv_addr.sin_addr.s_addr, server->h_length);
    serv_addr.sin_port = htons(CHAT_PORT);

    // Make the connection.
    if (connect(sockfd, (struct sockaddr *) &serv_addr, sizeof (serv_addr)) < 0)
        error("Error connecting to chat server");

#ifdef DEBUG
    printf("[DEBUG] Connected to server.\n");
#endif 

    return sockfd;
}

//-------------------------------- Messages ------------------------------------

// Bunch of send/recv functions that are not important to chat

int send_string(int sockfd, std::string myString)
{
#ifdef DEBUG
    printf("[DEBUG] Sending string: %s.\n", myString.c_str());
#endif 

    //send size
    uint32_t stringLen = myString.size();
    uint32_t sendLen = htonl(stringLen);
    int n = send(sockfd, &sendLen, sizeof (uint32_t), 0);
    if (n < 0) {
        error("Error sending message (string size). Removing client from list.");
        return -1;
    }
    //send string
    n = send(sockfd, myString.c_str(), stringLen, 0);
    if (n < 0) {
        error("Error sending message (string). Removing client from list.");
        return -1;
    }
    return 0;
}

std::string receive_string(int sockfd)
{
    //get string length
    uint32_t stringLen;
    int n = recv(sockfd, &stringLen, sizeof (uint32_t), 0);
    if (n < 0) {
        perror("Error receiving message(string size).");
    }
    stringLen = ntohl(stringLen);
    std::vector<uint8_t> buffer;
    buffer.resize(stringLen, 0x00);

    //get string
    n = recv(sockfd, &(buffer[0]), stringLen, 0);
    if (n < 0) {
        perror("Error receiving message(string).");
    }


    std::string returnString;
    returnString.assign(reinterpret_cast<const char*> (&(buffer[0])), buffer.size()); //might be a bad idea, but it works

#ifdef DEBUG
    printf("[DEBUG] Received message: %s\n", returnString.c_str());
#endif

    return returnString;
}


//----------------------------- Printing functions------------------------------

void print_menu_guest()
{
    // some visual function
}

void print_menu_user()
{
    // some visual function
}

void print_info()
{
    std::cout << " No information available on the game yet." << std::endl;
}


//---------------------------- Account functions -------------------------------

// Not necessary for chat functions

//--------------------------- Chat thread functions ----------------------------

void reception_thread(int sockfd)
{
#ifdef DEBUG
    printf("[DEBUG] Reception thread started.\n");
#endif 
    std::string stringToPrint;
    while (1) {
        stringToPrint = receive_string(sockfd);
        std::cout << stringToPrint << std::endl;
    }
}

void chat_thread(int sockfd, char* host)
{
#ifdef DEBUG
    printf("[DEBUG] Chat thread started.\n");
#endif 
    std::string myString, myUsername, blank;

    std::cout << "Enter your username (NO SPACES): ";
    std::cin >> myUsername;
    myUsername += ": ";

    int chat_sockfd = connect_to_chat(host);
    std::thread reception_thr(reception_thread, chat_sockfd);
    reception_thr.detach();

    while (1) {
        getline(std::cin, myString);
        if (!myString.empty()) {
            if (myString != "/quit") {
                send_string(chat_sockfd, (myUsername + myString));
            }
            else {
                printf("On peut pas encore quitter :( ");
            }
        }
    }


}

//---------------------- Menu management functions -----------------------------

// Main menu function

//---------------------------- Main function -----------------------------------

int main(int argc, char** argv)
{

    /* Make sure host and port are specified. */
    if (true) {

        char* hostname = "localhost";

        /* Connect to the server. */
        int sockfd = connect_to_server(hostname);


#ifdef DEBUG
        printf("[DEBUG] Client ID: Not yet implemented. ");
#endif

        login_prompt(sockfd);
        user_menu_loop(sockfd);
    }

    return 0;
}

And here is the server: Its most important functions (for the chat) are setup_user_fetcher, message_receiver, send_string_to_all, receive_string, send_string, get_chat_user, setup_chat_listener.

// Bunch of includes

const int PORT = 2477;
const int CHAT_PORT = 2478;
const int BACKLOG = 10;
const int MAX_CLIENTS = 20;

int clients_list[50] = {-1};

#define DEBUG

void error(const char *msg)
{
    perror(msg);
}

/* Catch Signal Handler functio */
void signal_callback_handler(int signum){

        printf("Caught signal SIGPIPE %d\n",signum);
}



//-------------------------- Server set-up functions ---------------------------

// Not necessary for chat

//--------------------------- Chat server functions  ---------------------------

int setup_chat_listener()
{
    int sockfd;
    struct sockaddr_in serv_addr;

    // Get a socket to listen on
    sockfd = socket(AF_INET, SOCK_STREAM, 0);
    if (sockfd < 0)
        error("ERROR opening listener socket.");

    // Zero out the memory for the server information
    memset(&serv_addr, 0, sizeof (serv_addr));

    // set up the server info
    serv_addr.sin_family = AF_INET;
    serv_addr.sin_addr.s_addr = INADDR_ANY;
    serv_addr.sin_port = htons(CHAT_PORT);

    // Bind the server info to the listener socket.
    if (bind(sockfd, (struct sockaddr *) &serv_addr, sizeof (serv_addr)) < 0)
        error("Error binding listener socket.");

#ifdef DEBUG
    printf("[DEBUG] Chat listener set.\n");
#endif 

    // Return the socket number.
    return sockfd;
}

int get_chat_user(int sockfd)
{

#ifdef DEBUG
    printf("[DEBUG] Getting chat user.\n");
#endif 

    struct sockaddr_in their_addr;
    socklen_t sin_size;

    if (listen(sockfd, BACKLOG) < 0) {
        perror("Error while listening.");
        exit(EXIT_FAILURE);
    }

    sin_size = sizeof (struct sockaddr_in);

    // Mise a zero de la memoire pour le client.
    memset(&their_addr, 0, sin_size);

    int new_fd = accept(sockfd, (struct sockaddr *) &their_addr, &sin_size);

    if (new_fd < 0)
        error("Error while accepting.");

    printf("Chat server: Connection received from: %s\n",
           inet_ntoa(their_addr.sin_addr));


    return new_fd;
}

int send_string(int sockfd, std::string myString)
{
#ifdef DEBUG
    printf("[DEBUG] Sending string to client %d.\n", sockfd);
#endif 
    uint32_t stringLen = myString.size();
    uint32_t sendLen = htonl(stringLen);
    int n = send(sockfd, &sendLen, sizeof (uint32_t), 0);
    if (n < 0) {
        error("Error sending message (string size). Removing client from list.");
        return -1;
    }
    //send string
    n = send(sockfd, myString.c_str(), stringLen, 0);
    if (n < 0) {
        error("Error sending message (string). Removing client from list.");
        return -1;
    }
    return 0;
}

std::string receive_string(int sockfd)
{
#ifdef DEBUG
    printf("[DEBUG] Receiving string.\n");
    printf("Current chat user sockfd: %d\n", sockfd);
#endif 
    uint32_t stringLen;
    int n = recv(sockfd, &stringLen, sizeof (uint32_t), 0);
    #ifdef DEBUG
    printf("[DEBUG] String size received: %d.\n", stringLen);
#endif 
    if (n < 0) {
        perror("Error receiving message(string size).");
    }
    stringLen = ntohl(stringLen);
    std::vector<uint8_t> buffer;
    buffer.resize(stringLen, 0x00);

    //get string
    n = recv(sockfd, &(buffer[0]), stringLen, 0);
    if (n < 0) {
        perror("Error receiving message(string).");
        close(sockfd);
    }


    std::string returnString;
    returnString.assign(reinterpret_cast<const char*> (&(buffer[0])), buffer.size()); //might be a bad idea, but it works

#ifdef DEBUG
    printf("[DEBUG] Received message: %s\n", returnString.c_str());
#endif

    return returnString;
}

void send_string_to_all(std::string myString)
{
#ifdef DEBUG
    printf("[DEBUG] Sending string to all clients.\n");
#endif 
    int n;
    for (int i = 0; i < 50; ++i) {
        if (clients_list[i] != -1) {
            n = send_string(clients_list[i], myString);
            if (n < 0) {
                close(clients_list[i]);
                clients_list[i] = -1;
            }
        }
    }
}

void message_receiver(int sockfd)
{
#ifdef DEBUG
    printf("[DEBUG] Setting up message receiver.\n");
    printf("Current chat user sockfd: %d", sockfd);
#endif 
    std::string message;
    int n;
    while (1) {
        message = receive_string(sockfd);
        std::thread t1(send_string_to_all, message);
        t1.detach();
    }
}




//------------------------------------------------------------------------------

// Bunch of send/recv functions, not necessary to chat


//----------------------------Account Functions---------------------------------

// Not necessary to chat

//------------------------------------------------------------------------------

// Main menu function

void setup_user_fetcher(int lis_chat_sockfd)
{
#ifdef DEBUG
    printf("[DEBUG] Gotta catch'em all.\n");
#endif 
    while (1) {
        int chat_user_sockfd = get_chat_user(lis_chat_sockfd);
        for (int i = 0; i < 50; ++i)
            if (clients_list[i] == -1) {
                clients_list[i] = chat_user_sockfd;
                break;
            }
        std::thread message_receiver_thread(message_receiver, chat_user_sockfd);
        message_receiver_thread.detach();
    }
}

int main(int argc, char** argv)
{
    signal(SIGPIPE, signal_callback_handler);

    int lis_sockfd = setup_listener();
    int lis_chat_sockfd = setup_chat_listener();

    std::thread chat_thread(setup_user_fetcher, lis_chat_sockfd);
    chat_thread.detach();
    while (1) {
        int user_sockfd = get_user(lis_sockfd);

        int* user_sockfd_ptr = (int*) malloc(sizeof (int));
        memset(user_sockfd_ptr, 0, sizeof (int));

        user_sockfd_ptr[0] = user_sockfd;

#ifdef DEBUG
        printf("[DEBUG] Starting main menu...\n");
#endif

        pthread_t thread;
        int result = pthread_create(&thread, NULL, main_menu,
                                    (void *) user_sockfd_ptr);
        if (result) {
            printf("Thread creation failed with return code %d\n", result);
            exit(-1);
        }

#ifdef DEBUG
        printf("[DEBUG] New main menu thread started.\n");
#endif
    }

    close(lis_sockfd);
    pthread_exit(NULL);

    return 0;
}

If you wish to reproduce the error, you could compile the code using the following lines

g++ client.cpp -o client -std=c++14 -pthread
g++ server.cpp -o server -std=c++14 -pthread

and run both without any arguments. The client is set to connect on "localhost".

I would be really glad if anyone could help me out with this.

7
  • 2
    Only 900 lines of code? Methinx you need to narrow down your question...
    – John3136
    Feb 26, 2016 at 1:08
  • @John3136 I definitely could. If anything is unclear (because lots of functions use other functions) feel free to ask. I will edit it now. Feb 26, 2016 at 1:10
  • And this is C++, not C code! Different languages. Feb 26, 2016 at 1:11
  • 1
    @Olaf actually, almost all the server/client part is written with the C libraries (same syntax, etc.), so yes, technically it is C++, but the server/client part is C. Feb 26, 2016 at 1:13
  • Edited the question so that only the important parts of the code remain. Feb 26, 2016 at 1:18

2 Answers 2

3

I recomment getting rid of the SIGPIPE signal itself.

signal(SIGPIPE, SIG_IGN);

Now, write()s on killed sockets will simply return -1. It should be easier to deal with that, instead of an asynchronous signal.

If you need SIGPIPE for other reasons, replace write()s with sendto()s with the MSG_NOSIGNAL option. See the sendto(2) manual page for more information.

4
  • I see. That might be a better idea, however will that fix the Bad file descriptor error? I thought it was an entirely different problem. Feb 26, 2016 at 1:20
  • Fairly certain that "Bad file descriptor" is due to a bug somewhere in your code, and is not related to actual socket operations. You should never encounter a bad file descriptor in properly-written code that knows exactly which file descriptors it has open at any given time. Feb 26, 2016 at 1:21
  • Yes, I am fairly sure that's the reason as well. I know that usually happens when the other side has closed the socket, or if it disconnected abruptly. I was trying to figure out a way to handle that disconnection, like a check before every operation to see if the client disconnected or not. I just don't know any way of doing it, so I was seeking an answer from someone more experienced in the matter. Feb 26, 2016 at 1:31
  • @DefinitelyNotMe The bad file descriptor is due to your closing the socket but not removing it from some data structure, for example the readFDs of your selector.
    – user207421
    Feb 26, 2016 at 3:12
1

You have UB. &(buffer[0]) will fail if the number of bytes read is 0 (which I believe will happen if client disconnects). You should test for 0 and return early before building your string.

Also you do not return after finding errors so you build your string from bad data in case of errors.

Maybe something more like:

std::string receive_string(int sockfd)
{
    uint32_t stringLen;
    int n = recv(sockfd, &stringLen, sizeof (uint32_t), 0);

    if (n < 0) {
        close(sockfd);
        // exit early
        throw std::runtime_error("Error receiving message(string size): "
            + std::string(std::strerror(errno)));
    }

    // test for zero
    if(!n)
        return {}; // empty string

    stringLen = ntohl(stringLen);
    std::vector<uint8_t> buffer(stringLen);
    // buffer.resize(stringLen, 0x00);

    //get string
    n = recv(sockfd, &(buffer[0]), stringLen, 0);
    if (n < 0) {
        close(sockfd);
        // exit early
        throw std::runtime_error("Error receiving message(string): "
            + std::string(std::strerror(errno)));
   }

    // only build string if no errors
    return {buffer.begin(), buffer.begin() + n};
}
3
  • Evaluating the address of one past the end of any data structure is not undefined behaviour. The second recv() call should also be tested for zero, not just the first one.
    – user207421
    Feb 26, 2016 at 3:16
  • @EJP The expression buffer[0] actually references the non-existent first element and empty vector, is that not UB?
    – Galik
    Feb 26, 2016 at 4:50
  • Thank you very much! Tweaking what you gave me got it working :) Feb 26, 2016 at 18:14

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

Not the answer you're looking for? Browse other questions tagged or ask your own question.