4
\$\begingroup\$

I've been experimenting with TCP clients in C++ and I really wanted to get some feedback on my code since I know it is bad. I would really appreciate some feedback on the class structure I have and the ways I can improve it. I also would love some feedback on any other simple bugs I have (or things in general I can fix).

//FOR CODE REVIEW PLEASE GIVE FEEDBACK

#include <winsock2.h>
#include <ws2tcpip.h>
#include <stdlib.h>
#include <stdio.h>
#include <iostream>
#include <string>
#include <vector>
#include <thread>

#undef UNICODE
#pragma comment (lib, "Ws2_32.lib")
#define WIN32_LEAN_AND_MEAN
#define port 0001;



std::vector<std::string> split(std::string mystring, std::string delimiter)
{
    std::vector<std::string> subStringList;
    std::string token;
    while (true)
    {
        size_t findfirst = mystring.find_first_of(delimiter);
        if (findfirst == std::string::npos) //find_first_of returns npos if it couldn't find the delimiter anymore
        {
            subStringList.push_back(mystring); //push back the final piece of mystring
            return subStringList;
        }
        token = mystring.substr(0, mystring.find_first_of(delimiter));
        mystring = mystring.substr(mystring.find_first_of(delimiter) + 1);
        subStringList.push_back(token);
    }
    return subStringList;
}

class client {
    WSADATA wsaData;
    //SOCKET cSocket = socket(AF_INET, SOCK_STREAM, IPPROTO_TCP);
    char recvBuf[1024];
    PCWSTR ip = L"192.168.86.36";
    public:
        void startWinsock() {
            std::cout << "[+] - Starting our client.\n";
            int rCode = WSAStartup(MAKEWORD(2, 2), &wsaData);
            if (rCode != NO_ERROR) {
                std::cout << "[-] - Error while trying to start winsock.\n";
                exit(0);
            }
        }
        /*
        void prepSocket() {
            SOCKET cSocket = socket(AF_INET, SOCK_STREAM, IPPROTO_TCP);
            if (cSocket == INVALID_SOCKET) {
                std::cout << "[+] - Error: " << WSAGetLastError() << std::endl;
                WSACleanup();
                exit(1);
            }
        }
        */

        void connectClient(SOCKET cSocket) {
            sockaddr_in addrServer;
            addrServer.sin_family = AF_INET;
            InetPton(AF_INET, ip, &addrServer.sin_addr.s_addr);
            addrServer.sin_port = htons(0001);
            memset(&(addrServer.sin_zero), '\0', 8);

            std::cout << "[+] - Connecting to our socket.\n";
            int iResult = connect(cSocket, (SOCKADDR*)&addrServer, sizeof(addrServer));
            if (iResult == SOCKET_ERROR) {
                closesocket(cSocket);
                std::cout << "[-] - Connection Failed: " << WSAGetLastError() << std::endl;
                WSACleanup();
            }
        }

        void sendMessage(SOCKET cSocket,char messageContent[1024]) {

            //char demoMessage[1024] = "[+] - Demo message\n";
            int message = send(cSocket, messageContent, strlen(messageContent), 0);
            if (message == SOCKET_ERROR) {
                std::cout << "[-] - Unable to send demo message: " << WSAGetLastError() << std::endl;
                closesocket(cSocket);
                WSACleanup();
            }
            std::cout << "[+] - Successfully sent our demo message.\n";

        }

        void recieveMessage(SOCKET cSocket) {
            std::cout << "[+] - Awaiting server response" << std::endl;
            int iResult = recv(cSocket, recvBuf, 1024, 0);
            if (iResult > 0) {
                std::string content;
                for (int i = 0; i < iResult; i++) {
                    content += recvBuf[i];
                }
                if (strstr(content.c_str(), "cmd")) {
                    std::string command = split(content, (std::string)"cmd ")[1];
                    std::cout << "COMMAND SHIT NOT IMPLEMENTED\n";
                }
                else if (strstr(content.c_str(), "exit")) {
                    exit(0);
                }
                std::cout << ">  " << content;
            }
            else if (iResult == 0) {
                std::cout << "[-] - Connection closed\n" << std::endl;
            }
            else {
                std::cout << "[-] - An error occured: " << WSAGetLastError() << std::endl;
            }

        }
        void killSocket(SOCKET cSocket) {
            closesocket(cSocket);
            WSACleanup();
        }

};

int main() {
    client client;
    client.startWinsock();
    //client.prepSocket();
    SOCKET cSocket = socket(AF_INET, SOCK_STREAM, IPPROTO_TCP);
    if (cSocket == INVALID_SOCKET) {
        std::cout << "[+] - Error: " << WSAGetLastError() << std::endl;
        WSACleanup();
        exit(1);
    }
    client.connectClient(cSocket);
    client.sendMessage(cSocket,(char *)"works kek :happy_pepe:\n");
    client.recieveMessage(cSocket);
    client.killSocket(cSocket);

}

If you would rather have a pastebin link here's one:

\$\endgroup\$
3
2
\$\begingroup\$

Code Review

Do you really want to use C functions?

#include <stdio.h>

Sure you can but you may want to use the correct version of this header file for C++ which is <cstdio>.


Using port 1 is not recommended for normal socket work.

#define port 0001;

Ports below 1024 are reserved for the OS (and usually specifically allocated to standard services used by the OS).

https://www.webopedia.com/quick_ref/portnumbers.asp

But since you are writing TCP connection 1 is the correct port.


To prevent excessive copying pass complex parameters by const reference rather than value (unless you are going to copy them anyway).

std::vector<std::string> split(std::string mystring, std::string delimiter)

I think we can improve the split a bit:

std::vector<std::string> split(std::string const& mystring, std::string const& delimiter)
{
    std::vector<std::string> subStringList;

    std::string::size_t start = 0;
    std::string::size_t end   = mystring.find_first_of(delimiter);

    while (end != std::string::npos) {
        subStringList.emplace_back(mystring.substr(start, end - start));
        start = end + delimiter.size();
    }
    // if the delimiter is the last item in the string
    // then the following will add a blank string to subStringList.
    // If that is not what you want just test to see if start ==  mystring.size()
    subStringList.emplace_back(mystring.substr(start));

    return subStringList;
}

Remove commented out code from your source.

    //SOCKET cSocket = socket(AF_INET, SOCK_STREAM, IPPROTO_TCP);

        /*
        void prepSocket() {
            SOCKET cSocket = socket(AF_INET, SOCK_STREAM, IPPROTO_TCP);
            if (cSocket == INVALID_SOCKET) {
                std::cout << "[+] - Error: " << WSAGetLastError() << std::endl;
                WSACleanup();
                exit(1);
            }
        }
        */

This is what source control systems are for. Check out github.


You only ever want to connect to this specific host?

    PCWSTR ip = L"192.168.86.36";

I would expect that you want to pass a hostname as a parameter to the client.


Is this really part of the client class?

        void startWinsock() {
            std::cout << "[+] - Starting our client.\n";
            int rCode = WSAStartup(MAKEWORD(2, 2), &wsaData);
            if (rCode != NO_ERROR) {
                std::cout << "[-] - Error while trying to start winsock.\n";
                exit(0);
            }
        }

I would make the start up/shotdown of the windowing socket code a separate class TCPInit (probably a singelton). Then the constructor of client will simply just make sure that the socket stuff has been initialized by getting an instance of TCPInit.


You don't check the result of this call:

            InetPton(AF_INET, ip, &addrServer.sin_addr.s_addr);

Did you not define a port macro above?

            addrServer.sin_port = htons(0001);

Don't assume the size of this object is 8.

            memset(&(addrServer.sin_zero), '\0', 8);

That is what sizeof is used for.


The value returned by send() is not just an error.

            int message = send(cSocket, messageContent, strlen(messageContent), 0);
            if (message == SOCKET_ERROR) {

You should check the result of send() make sure you have sent the whole message. You may need to call send() multiple times to send the whole message.


This looks like a destructor:

        void killSocket(SOCKET cSocket) {
            closesocket(cSocket);
            WSACleanup();
        }
\$\endgroup\$
5
  • \$\begingroup\$ Thank you very much <3 this is exactly the type of feedback I was looking for. \$\endgroup\$ – user222877 Apr 24 '20 at 16:54
  • \$\begingroup\$ I have one question after fully reading your feedback, why do you recommend use sizeof instead of 8 isn't it always going to have an integer value of 8? \$\endgroup\$ – user222877 Apr 24 '20 at 21:43
  • 1
    \$\begingroup\$ @Backslash Can you show me where it says its 8 in the standard? If so then you can use 8. Otherwise it is an implementation details that will depend on platform. Actually this field sin_addr is an ungodly structure struct in_addr that is a combination of several unions depending on platform and age of your OS. You should zero out the whole structure as that is what the API is expecting. \$\endgroup\$ – Martin York Apr 24 '20 at 21:56
  • \$\begingroup\$ While searching for where it says that, i was unable to find that so I now realize that I was wrong. Thank you lol \$\endgroup\$ – user222877 Apr 24 '20 at 23:01
  • 2
    \$\begingroup\$ @Backslash In general using "magic" numbers is bad practice. Even if the value was exactly 8 I would have recomended putting that value into a named constant (e.g. static int const SizeOfAddr = /*Value*/;. This has two benefits 1) Named constants give more meaning to your code as they become self documenting. If the value does change in a new version of the OS then you have. only one place in the code to change the value (at the constant). \$\endgroup\$ – Martin York Apr 24 '20 at 23:05
1
\$\begingroup\$

Prefer using getaddrinfo()

You are using InetPton() to convert an IPv4 address to a sockaddr_in. However, this function has several drawbacks:

  • It does not perform DNS lookups, so you can't use a hostname to connect to a server.
  • You have to know up front which address family to use.
  • It doesn't fill in all of a sockaddr_in, it only provides you with the address part.

If you use getaddrinfo() you do get DNS lookups, and you don't have to deal with address families explicitly anymore. There is only one drawback with getaddrinfo(), and that is that if you provide it with a hostname, you can actually get multiple struct sockaddrs back from it. The correct way to deal with this is to loop over all the sockaddrs, and try to connect to each one in turn until you get a working connection.

Don't write number with leading zeroes

In C++, if you write an integer literal starting with a zero, it will be treated as an octal number. Lucky for you, the octal number 0001 has the same value as the decimal number 1. However, if you would have written 0010, then it would have been the same as 8.

Avoid #define for defining constants

In C++, it is better to declare constants as static constexpr variables. For example:

static constexpr std::uint16_t port = 1;

This has the advantage that you can give a proper type to the constant, and that the compiler can actually check this declaration for correctness. Your definition was actually incorrect, since you added a semicolon to the macro:

#define port 0001;

If you would have used this to fill in sin_port in connectClient() as Martin York suggests in his answer, then the following line:

addrServer.sin_port = htons(port);

Would have expanded to:

addrServer.sin_port = htons(0001;);

Which is clearly a syntax error.

Avoid using C functions on C++ objects

If you use std::string, you should prefer to use the member functions of that class to manipulate strings. For example, instead of writing:

if (strstr(content.c_str(), "cmd")) {

You should write this idiomatic C++ code using find():

if (content.find("cmd") != content.npos) {

It's a bit unfortunate that we have to wait for C++23 for contains().

\$\endgroup\$
1
  • 1
    \$\begingroup\$ That getaddrinfo() looks to be closely modelled on the POSIX function of the same name, so using it is easing the way towards more portability, too. \$\endgroup\$ – Toby Speight Mar 5 at 7:44

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy