Odtnhj

I'm currently writing a server and client app that attemps to transfer a screenshot but it's not working properly. I implemented it like this.

SOCKET sock;
char buf[4096];

DWORD WINAPI thread_function()

 bool file_transfer = false;
 bool loop = true;
 while (1)
 
 ZeroMemory(buf, 4096);
 int bytesReceived = recv(sock, buf, 4096, 0);
 if (bytesReceived > 0)
 
 std::string received(buf, 0, bytesReceived);
 if (received == "Sending file.")
 
 file_transfer = true;
 

 if (file_transfer == false)
 
 std::cout << "nSERVER> " << std::string(buf, 0, bytesReceived) << std::endl; 
 std::cout << "> ";
 
 else if (file_transfer == true)
 
 loop = true;
 TCHAR *szfname = "screenshot.bmp";
 FILE* f = fopen(szfname, "wb");
 if (NULL == f)
 
 std::cerr << "Error opening file" << std::endl;
 return 1;
 
 while ((bytesReceived = recv(sock, buf, 4096, 0)) > 0 && loop == true)
 
 received = buf;
 if (received == "File transfer completed !")
 
 loop = false;
 std::cout << "File transfer completed !" << std::endl;
 std::cout << "> ";
 
 else
 
 fwrite(buf, 1, bytesReceived, f);
 
 
 file_transfer = false;
 
 
 

I call the function with this

CreateThread(0, 0, (LPTHREAD_START_ROUTINE)thread_function, 0, 0, 0);

The thing is I believe this is not a very clean way of doing it and also it's not working perfectly. After a file is received I don't correctly receive what the server is sending.

This is the server code wich I think is fine.

 send(clientSocket, TEXT("Attempting to take a screenshot."), sizeof(TEXT("Attempting to take a screenshot...")), 0);
 HWND win = GetDesktopWindow();
 HDC dc = GetDC(win);
 if (HDCToFile("screenshot.bmp", dc, 0, 0, 1920, 1080 ) == true)
 
 send(clientSocket, TEXT("Sending file."), sizeof(TEXT("Sending file.")), 0);
 FILE *fp = fopen("screenshot.bmp", "rb");
 if (fp == NULL)
 
 std::cerr << "Error : Cannot open file." << std::endl;
 return 1;
 
 while (1)
 
 char buff[4096] = 0 ;
 int nread = fread(buff, 1, 4096, fp);
 if (nread > 0)
 
 send(clientSocket, buff, sizeof(buff), 0);
 
 if (nread < 4096)
 
 if (feof(fp))
 
 std::cout << "File transfer completed !" << std::endl;
 send(clientSocket, TEXT("File transfer completed !"), sizeof(TEXT("File transfer completed !")), 0);
 
 if (ferror(fp))
 std::cerr << "Error reading." << std::endl;
 break;
 
 
 
 else
 
 send(clientSocket, TEXT("Screen capture failed...."), sizeof(TEXT("Screen capture failed....")), 0);
 

Thanks for your time and help.

TCP is a streaming protocol. It has no concept of messages, so when the server sends "Sending file." there is no separation between the string and the beginning of the file being sent. Everything just goes into the stream one byte after the next and when the network stack decides it's time, usually because a packet has been filled or it's been too long since data was last added, a packet is sent, possibly containing multiple messages.

So

int bytesReceived = recv(sock, buf, 4096, 0);

very likely reads the full 4096 bytes, Attempting to take a screenshot.Sending file. plus the first four thousand-or-so bytes of the bitmap. The client code consumes the string and discards the rest of the buffer.

You need to establish a communication protocol that sits between the the socket and the writing of the file. There are a whole bunch of different ways to handle this. Common tricks for reading strings are

1. Write the length of the string before writing the string so that the protocol handler knows how many bytes to read ahead of the time

Sender

uint16_t len = str.length(); // size is exactly 16 bits
len = htons(len); // endian is known
int sent = send(sock, (char*)&len, sizeof(len), 0);
// test sent for success (did not fail, sent all the bytes)
sent = send(sock, str.c_str(), len, 0);
// test sent for success (did not fail, sent all the bytes) 
// may need to loop here if the string is super long.

Receiver

uint16_t len;
int recd = recv(sock, (char*)&len, sizeof(len), MSG_WAITALL);
// test recd for success (did not fail, read all the bytes)
// MSG_WAITALL will read exactly the right number of bytes or die trying. 
len = ntohs(len); // ensure correct endian
std::string msg(len, ' '); // allocate a big enough string
char * msgp = &msg[0]; // or msg.data() if C++17 or better. 
 // Never seen &msg[0] fail, but this is not guaranteed by C++
while (len) // sometimes you want an extra exit condition here to bail out early

 recd = recv(sock, msgp, len, 0);
 // test recd for success 
 len -= recd;
 msgp += recd;
 

2. Insert a canary value so that the protocol handler knows when to stop reading. The null terminator works well here. The protocol reads up until it finds the null and preserves the remainder of what's read for later consumption. No code example here because this can be done many, many different ways.


3. Not using strings and sending integer code messages instead. Eg:

enum messageID

 TAKING_SCREENSHOT,
 SENDING_FILE,
 EATING_COOOOOOKIE_OM_NOM_NOM
;

OK! That moves the strings correctly. Assuming I don't have a bug in there. The idea's right, but the actual code is from memory and may contain brainfarts.

What you want to have is a bunch of functions, one for each type of data you send. Each of these functions can and should be be tested separately so that when you get to integrating them into the program, the program looks something like

sendString(sock, "Attempting to take a screenshot.");
if (getBitmap("screenshot.bmp"))

 sendString(sock, "Sending file.");
 sendBitmap(sock, "screenshot.bmp");

or

receiveString(sock);
std::string command = receiveString(sock);
if (command == "Sending file.")

 receiveBitmap(sock, "screenshot.bmp");

else if (command == "Eating coooooookie! Om! Nom! Nom!")

 OmNomNom(sock);

Which is about a close to foolproof as you can get.

Notes:

There is a bug in the server: int nread = fread(buff, 1, 4096, fp); gets the number of bytes read, but send(clientSocket, buff, sizeof(buff), 0); always tries to send a full buffer regardless of how many bytes were read, so garbage will be sent to the client. Also send can fail and this is not being checked. Always check the return codes. People don't put them there unless they're important.