0

The code for echo_server and echo_client is posted below. I noticed that when I type a message over a certain length in the echo_client, the server truncates the end of it and echoes back only part of it. The buffer size is 1024 bytes and the message I'm typing is much, much shorter than that length. What's going on here? How do I solve this problem so the server echoes back the complete message provided its under the length limit?

The server code:

#include "stdafx.h"
#include <iostream>
#include <string>
#include <string.h>


#ifndef UNICODE
#define UNICODE
#endif
#define WIN32_LEAN_AND_MEAN

#include <winsock2.h>
#include <ws2tcpip.h>
#pragma comment(lib, "ws2_32.lib")

using namespace std;


static int MAXPENDING = 5;


int main(int argc, char *argv[])

{

WSADATA wsaData;
int iResult;
int optv = 0;
bool connected = false;
char *optval = (char*)&optv;
int optlen = sizeof(optval);
string Q = "quit";
const char *exit =Q.c_str();




iResult = WSAStartup(MAKEWORD(2, 2), &wsaData);
if (iResult != NO_ERROR) {
    wprintf(L"WSAStartup function failed with error: %d\n", iResult);
    return 1;
}


if(argc!=2){
    printf("Error: incorrect number of arguments.  ");
    return 1;
}


SOCKET servSock;
servSock = socket(AF_INET,SOCK_STREAM,IPPROTO_TCP);
if(servSock==INVALID_SOCKET){
    printf("Socket function failed with error: %d\n",GetLastError());
    return 1;
}


u_int servPort = atoi(argv[1]);

sockaddr_in servAddr;
servAddr.sin_family = AF_INET;
servAddr.sin_addr.s_addr = htonl(INADDR_ANY);
servAddr.sin_port = htons(servPort);

int opt = setsockopt(servSock,SOL_SOCKET,SO_REUSEADDR,optval,sizeof(optval));


if(bind(servSock,(sockaddr*)&servAddr,sizeof(servAddr))<0){
    printf("Bind function failed with error: %d\n", GetLastError());
    return 1;
}



for(;;){


    if(listen(servSock,MAXPENDING) < 0){
    printf("Listen function failed with error: %d/n",GetLastError());
    return 1;
    }else{
        char *str = new char[5];

        printf("Server listening on port %d\n",servPort);
    }

    SOCKET clientSock;
    sockaddr_in clientAddr;
    socklen_t caddrlen = sizeof(clientAddr);



    clientSock = accept(servSock,(sockaddr*)&clientAddr,&caddrlen);

    if(clientSock < 0){
       printf("Accept() function failed with error: %d/n", GetLastError());
       goto QUIT;

    }else if(clientSock >=0){
        connected = true;
    }



    char cName[INET_ADDRSTRLEN];

    if(inet_ntop(AF_INET,&clientAddr.sin_addr,cName,sizeof(cName))!=NULL){
        printf("Handling client %s/%d\n", cName,ntohs(clientAddr.sin_port));
    }else{
        printf("Error: Unable to get client address");
    }


    char buffer[1024];


    while(connected==true){



        long nbytesrcvd = recv(clientSock,buffer,sizeof(buffer),0);




        if(nbytesrcvd==0){
            connected = false;
            cout << endl;
            cout << cName << ": client disconnected" << endl;                   
            cout << endl;
            break;
        }



        if(nbytesrcvd < 0){
            printf("Error: recv() failed");
            cout << endl;
            goto QUIT;
        }


         if(nbytesrcvd > 0){

            long nbytessent = send(clientSock,buffer,nbytesrcvd,0);
            if(nbytessent < 0){
                cout << "Error: send() failed" << endl;
                cout << endl;
                goto QUIT;          

            }else if(nbytessent!=nbytesrcvd){
                cout << "send() error: sent unexpected # of bytes" << endl;
                cout << endl;
                goto QUIT;

            }                       
         }

    }



    QUIT:

        int iResult = closesocket(clientSock);
        if (iResult == SOCKET_ERROR) {
            printf("closesocket function failed with error: %d\n",GetLastError());
        }



    }




}

And the client code:

#include "stdafx.h"
#include <iostream>
#include <string>
#include <string.h>


#ifndef UNICODE
#define UNICODE
#endif

#define WIN32_LEAN_AND_MEAN

#include <Windows.h>
#include <winsock2.h>
#include <ws2tcpip.h>


#define BUFFSIZE 1024

#pragma comment(lib, "ws2_32.lib")

using namespace std;


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


WSADATA wsaData;
int result;
bool connected = false;
hostent *rhost;
char buffer[BUFFSIZE];
string Q = "quit";
const char *exit =Q.c_str();






result = WSAStartup(MAKEWORD(2, 2), &wsaData);
if (result != NO_ERROR) {
    printf("WSAStartup function failed with error: %d\n", result);
    return 1;
}




SOCKET connector;
connector = socket(AF_INET, SOCK_STREAM, IPPROTO_TCP);
if (connector == INVALID_SOCKET) {
    wprintf(L"socket function failed with error: %ld\n", WSAGetLastError());
    WSACleanup();
    return 1;
}

string hname;
cout << "Enter host name(URL): ";
cin >> hname;
cout << endl;

string portnum;
cout << "Enter the port number you wish to connect on: " ;
cin >> portnum;
cout << endl;

char *hostname = const_cast<char*>(hname.c_str());
char *hostPort = const_cast<char*>(portnum.c_str());



rhost = gethostbyname(hostname);
in_addr addr;
addr.s_addr = *(u_long *)rhost->h_addr_list[0];



sockaddr_in clientserv;
clientserv.sin_family = AF_INET;
clientserv.sin_addr.s_addr = addr.s_addr;
clientserv.sin_port = htons(atoi(hostPort));

if(connect(connector,(sockaddr*)&clientserv,sizeof(clientserv))==SOCKET_ERROR){
    printf("connect function failed with error: %d\n", GetLastError());
    if(closesocket(connector)<0){
        printf("closesocket function failed %d\n", GetLastError());
    }
    return 1;
}else{
    connected = true;
    cout << "Connected to host " << hname << " on port " << portnum << endl;
    cout << "Type 'quit' to exit the program " << endl;
}


while(connected==true){

   int nbr = 0;
   string msg;

   cout << ">";
   getline(cin,msg);
   cout << endl;




   if(msg=="quit"){

       connected = false;
       goto quit;
   }


   int len = sizeof(msg);
   const char *message = msg.c_str();
   strncpy_s(buffer,message,sizeof(msg));         
   long nbs = send(connector,buffer,len,0);
   if(nbs < 0){
        printf("send() failed", GetLastError());
        return 1;
   }

   while(nbr < nbs){
        nbr = recv(connector,buffer,len,0);
        if(nbr < 0){
            printf("recv() failed", GetLastError());
            return 1;

        }else if(nbr==0){
            printf("recv() failed: connection closed prematurely", GetLastError());
            return 1;               
        }else if(nbr > 0){
            string str(buffer);
            cout << ">> " << str << endl;
            cout << endl;
        }

   }



}

quit:

    if (closesocket(connector) == SOCKET_ERROR) {
        printf("closesocket function failed with error: %ld\n", GetLastError());
        WSACleanup();
        return 1;
    }


WSACleanup();

return 0;

}

1
  • In the client, don't do the const_cast of the strings returned by the c_str function. The functions you use have their arguments being const char * and so should you. May 7, 2013 at 0:40

2 Answers 2

1

Remember that recv may not receive all of the data at once. This means that the recv call in the server may not get the complete message, but send back what it gets and in turn the client may receive only a part of what is sent from the server.

This would not be a problem, unless you only receive once in the client and then continue doing something else. Just like you do.

There are two solutions to this:

  1. Make a simple protocol that either prepends each message with the message length, or have a special end-of-message marker. In the first case you will always know how much data there is and can read in a loop until all is received, in the second case you receive in aloop until you get the end-of-message marker.

  2. Make the sockets non-blocking, and read in a loop until recv returns an error with the error being WSAEWOULDBLOCK. Then there is no more data to read.


Mistakes in the client:

int len = sizeof(msg);

This line returns the size of the string object, not the string contained inside the object. You should use msg.length() to get the length. You use this in a couple of places.

Using a std::string is quite okay, if you use it properly. E.g.:

long nbs = send(connector, msg.c_str(), msg.length(), 0);

Another thing:

nbr = recv(connector, buffer, len, 0);

You again use the wrong size here, the one provided by the previously and erroneously use of sizeof(msg). Instead you should provide the size of the actual buffer. Since buffer is a proper array you can use e.g. sizeof(buffer) - 1 here. The -1 is because the data you send from the client is not an zero-terminated string, and so you need to reserve a space in the received data to terminate it like a string:

buffer[nbr] = '\0';
3
  • These issues do not really apply in a simple echo system like the OP's code, but they are valid points in more complex systems. May 4, 2013 at 17:49
  • Joachim Pileborg, thanks for your advice but that is actually unnecessary. TCP allows you to prepend messages of arbitrary length using the setsockopt() function and the SO_RCVBUFF/SO_SENDBUFF options. But what worked is taking the client console input as a char array instead of a string.
    – Mr X
    May 6, 2013 at 18:09
  • @MrX Re-read the client code, and added a few points about it. May 7, 2013 at 0:50
0

I see several problems with your code.

Your server is calling listen() inside the accept() loop. It does not belong here. Call listen() only once, after bind().

send() can accept fewer bytes than requested. When sending a buffer, call send() in a loop until the entire buffer is exhausted. Your client and server code both suffer from this.

Your client code is copying the user's input into a buffer and then sending the entire buffer, even if the input is smaller than the buffer. That will also truncate the input if it is larger than the buffer. In both cases,you don't need that buffer at all. You can pass the input string directly to send(), using the string's c_str() and length() methods. That way, the entire string gets sent regardless of its actual length.

Your client code is also not taking the return value of recv() into account when converting the echoed data into a string. The data is not null terminated, so you need to pass the nbr variable as the second parameter of the string constructor.

3
  • Remy, I actually solved this problem by reading console input from the client program as a char array instead of a string.
    – Mr X
    May 6, 2013 at 1:58
  • I used: scanf("%s",buffer) instead of getline(cin,msg). The reason for the listen() function being nested inside of the for(;;) loop is so that the server will continue to listen for incoming client connections as long as the server program is running.
    – Mr X
    May 6, 2013 at 18:06
  • Using scanf() like that has potential for a buffer overflow. You need to tell it the size of your buffer so it won't overflow. And you DO NOT need to call listen() in a loop. Once the server starts listening, it stays listening until it is closed. You only need to call accept() in a loop. May 6, 2013 at 18:52

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.