Problem with winsock recv() and accept()

Question

I'm having trouble with a socket application I'm programming in C++. I'm doing my programming with Bloodshed Dev-Cpp on Windows XP. I made a class for handling all the message transfers and have a client and server program that both use that class for handling their services. The application itself is very basic, the only intent I have for it is to get all this to work.

The client, which is for sending messages, works like I expect it to. If my server is running, it doesn't have any errors when sending a message. If it's not running it'll pass an error. But my server continuously accepts weird gibberish. It's always the same data. When it receives the message there is no effect. If I have my client try to identify the server, it gets back gibberish.

I have included my source code here. The linker also brings in two extra parameters: -lwsock32 and an inclusion of the library libws2_32.a, which came with Dev-Cpp.

Here's the header for my Messager class:

#ifndef MESSAGER
#define MESSAGER

#include <string>

class Messager{
    private:
     int sendSocket;
     int listenSocket;

    public:
     void init(void);
     bool connect(std::string ip, std::string port);
     bool bind(std::string port);
     void listen(void);
     void send(std::string message);
     std::string receive(void);
};
#endif

These are my definitions for the Messager class:

#include "Messager.h"
#include <winsock2.h>
#include <sys/types.h>
#include <ws2tcpip.h>
#include <windows.h>

void Messager::init(void){
    WSADATA wsaData;

    WSAStartup(MAKEWORD(1,1), &wsaData);
}

bool Messager::connect(std::string ip, std::string port){
    struct addrinfo hints;
    struct addrinfo *res;
    bool success = false;

    memset(&hints, 0, sizeof hints);
    hints.ai_family = AF_UNSPEC;
    hints.ai_socktype = SOCK_STREAM;

    getaddrinfo(ip.c_str(), port.c_str(), &hints, &res);

    sendSocket = socket(res->ai_family, res->ai_socktype, res->ai_protocol);

    success = ::connect(sendSocket, res->ai_addr, res->ai_addrlen) != -1;

    freeaddrinfo(res);

    return success;
}

bool Messager::bind(std::string port){
    struct addrinfo hints, *res;

    memset(&hints, 0, sizeof hints);
    hints.ai_family = AF_UNSPEC;
    hints.ai_socktype = SOCK_STREAM;
    hints.ai_flags = AI_PASSIVE;

    getaddrinfo(NULL, port.c_str(), &hints, &res);

    listenSocket = socket(res->ai_family, res->ai_socktype, res->ai_protocol);

    if(listenSocket == INVALID_SOCKET){
     return false;
    }

    if(::bind(listenSocket, res->ai_addr, res->ai_addrlen) == -1){
     return false;
    }

    return true;
}

void Messager::listen(void){
    ::listen(listenSocket, 10);
}

int Messager::send(std::string message){
    const std::string terminator = "\r\n";
    std::string realMessage;
    int size = 0;
    int totalSent = 0;

    realMessage = message;
    realMessage += terminator;

    size = realMessage.size();

    totalSent = ::send(sendSocket, realMessage.c_str(), size, 0);

    if(totalSent == 0 || totalSent == -1){
     return 0; // There must be an error, 0 means it is an error
    }

    // This statement keeps adding the results of ::send to totalSent until it's the size of the full message
    for(totalSent = 0; totalSent < size; totalSent += ::send(sendSocket, realMessage.c_str(), size, 0));

    return totalSent;
}

// This function has been updated a lot thanks to @Luke
std::string Messager::receive(void){
    const int bufferSize = 256;
    const std::string terminator = "\r\n";
    char buffer[bufferSize];
    int i = 0;
    int received = 0;
    std::string tempString;
    size_t term = 0;

    for(i = 0; i < bufferSize; i++){
     buffer[i] = 0;
    }

    received = ::recv(listenSocket, buffer, bufferSize, 0);
    tempString = buffer;
    term = tempString.find(terminator);

    if(term != -1){ // Already have line
     line = tempString;
    }

    while(received != -1 && received != 0){ // While it is receiving information...
     // Flush the buffer
     for(i = 0; i < bufferSize; i++){
      buffer[i] = 0;
     }

     ::recv(listenSocket, buffer, bufferSize, 0);
     tempString += buffer;
     term = tempString.find(terminator);

     if(term != -1){ // Found terminator!
      return tempString;
     }
    }

    throw 0; // Didn't receive any information.  Throw an error
}

Any ideas about what might be going on would be really appreciated. If necessary I can post the code the server and client use, but I can give a general outline:

Server:

• messager.init()
• messager.bind()
• messager.listen()
• messager.receive() <-- includes accept()

Client:

• messager.init()
• messager.connect()
• messager.send()

Thanks in advance.

Answer 1

I'd be a bit concerned about your receive code. It creates a char* buffer to receive the data but doesn't actually allocate any memory for it.

Now I can't tell whether you're calling the WinSock recv there since you don't explicitly say ::recv but I think you would need to either:

• allocate some space with malloc first (id recv wants a buffer); or
• pass the address of the buffer pointer (if recv allocates its own buffer).

I'm actually surprised that the doesn't cause a core dump since the value of buffer could be set to anything when you call recv.

Something like this may be better:

char *Messager::receive(void){
    int newSocket = 0;
    struct sockaddr_storage *senderAddress;
    socklen_t addressSize;
    char *buffer;

    addressSize = sizeof senderAddress;
    newSocket = accept(listenSocket, (struct sockaddr *)&senderAddress,
        &addressSize);

    buffer = new char[20];
    recv(newSocket, buffer, 20, 0);

    return buffer;

}

But you need to remember that the client of this function is responsible for freeing the buffer when it's finished with it.

Answer 2

Two suggestions:

• check the return values of all the socket functions you call
• ditch DevC++ - it is buggy as hell & is no longer being developed - use http://www.codeblocks.org/ instead.

Answer 3

In your receive function, the buffer local is never initialized to anything, so you end up reading your message into some random memory and probably causing corruption or crashing. You probably want char buffer[MAX_MSG_LENGTH]; instead of char *buffer

Answer 4

I see two concerns.

1. You can't safely use the string assignment operator in Message::receive(). The assignment operator relies on the character array being NULL-terminated, and in this case it is not. It's probably filling it up with a bunch of garbage data. You should get the number of characters actually received (i.e. the return value of recv()) and use the string::assign() method to fill the string object.
2. There is no code to ensure all the data has been sent or received. recv() is going to return as soon as any data is available; you really need to loop until you have received the entire message. For plain-text data, typically people use a CR-LF pair to indicate the end of a line. You keep calling recv() and buffering the results until you see that CR-LF pair and then return that line to the caller. You should also loop on send() until your entire buffer has been sent.

Typically this looks something like the following (this is all from memory so there are probably a few minor errors, but this is the gist of it):

bool Message::Receive(std::string& line)
{
    // look for the terminating pair in the buffer
    size_t term = m_buffer.find("\r\n");
    if(term != -1)
    {
        // already have a line in the buffer
        line.assign(m_buffer, 0, term); // copy the line from the buffer
        m_buffer.erase(0, term + 2); // remove the line from the buffer
        return true;
    }
    // no terminating pair in the buffer; receive some data over the wire
    char tmp[256];
    int count = recv(m_socket, tmp, 256);
    while(count != -1 && count != 0)
    {
        // successfully received some data; buffer it
        m_buffer.append(tmp, count);
        // see if there is now a terminating pair in the buffer
        term = m_buffer.find("\r\n");
        if(term != -1)
        {
            // we now have a line in the buffer
            line.assign(m_buffer, 0, term); // copy the line from the buffer
            m_buffer.erase(0, term + 2); // remove the line from the buffer
            return true;
        }
        // we still don't have a line in the buffer; receive some more data
        count = recv(m_socket, tmp, 256);
    }
    // failed to receive data; return failure
    return false;
}