TCP server in C++

Question

I am building a TCP server in C++, and right now it's a test version without logic to handle server-client packet transactions. Please check if there are any memory leak related issues or anything else I can do better.

Program.cpp

#include <iostream>
#include "Network/Server.cpp"

int main()
{
    srand(time(NULL));
    Server serv;
    serv.start_binding();
    serv.start_listening();
    return 0;
}

Server.cpp

#define SERVER

#include <sys/socket.h>
#include <arpa/inet.h>
#include <unistd.h>
#include <list>

#include "./Connection.cpp"
#include "../Utility/Helper.cpp"

#define PORT 9339
#define MAX_CLIENTS_COUNT 1000
#define TIME_TO_SLEEP 0 * 1000000

class Server
{
public:
    int                    fdmax;
    int                    newfd;
    int                    listener;
    int                    connections_count;
    fd_set                 master;
    fd_set                 read_fds;
    socklen_t              address_length;
    struct sockaddr_in     server_address;
    struct sockaddr_in     remote_address;
    std::list<Connection*> connections;
    ByteStream             stream;

    void start_binding()
    {
        std::cout << "Binding..." << std::endl;
        int sockopt = 1;
        server_address.sin_family = AF_INET;
        server_address.sin_addr.s_addr = INADDR_ANY;
        server_address.sin_port = htons(PORT);
        memset(&(server_address.sin_zero), '\0', 8);

        FD_ZERO(&master);
        FD_ZERO(&read_fds);

        if ((listener = socket(PF_INET, SOCK_STREAM, 0)) == -1) 
        {
            Helper::handle_error(true);
        }
    
        if (setsockopt(listener, SOL_SOCKET, SO_REUSEADDR, &sockopt, sizeof(int)) == -1)
        {
            Helper::handle_error(true);
        }

        if (bind(listener, (struct sockaddr *)&server_address, sizeof(server_address)) == -1) 
        {
            Helper::handle_error(true);
        }

        if (listen(listener, 10) == -1)
        {
            Helper::handle_error(true);
        }

        FD_SET(listener, &master);
        fdmax = listener;
    }

    void start_listening()
    {
        std::cout << "Listening..." << std::endl;
        while (true)
        {
            read_fds = master;

            if (select(fdmax + 1, &read_fds, NULL, NULL, NULL) == -1)
            {
                Helper::handle_error(true);
            }
            handle_accepting();
            handle_receiving();
            usleep(TIME_TO_SLEEP);
        }
    }

    void handle_accepting()
    {
        if (FD_ISSET(listener, &read_fds))
        {
            address_length = sizeof(remote_address);
            newfd = accept(listener, (struct sockaddr *)&remote_address, &address_length);
            if (newfd != -1 && connections_count < MAX_CLIENTS_COUNT)
            {
                FD_SET(newfd, &master);
                if (newfd > fdmax)
                {
                    fdmax = newfd;
                }
                connections.emplace_back(new Connection(newfd));
                connections_count++;
                std::cout << "Новое соединение: " << get_ip_by_fd(newfd) << std::endl;
            }
        }
    }

    void handle_receiving()
    {
        for (auto const& connect: connections)
        {
            if (!(FD_ISSET(connect->fd, &read_fds)))
            {
                continue;
            }

            if (connect->read_header)
            {
                int nbytes = recv(connect->fd, connect->header, 7, 0);
                if (nbytes <= 0 || nbytes != 7)
                {
                    close_connection(connect);
                    break;
                }
                connect->read_header = false;
                continue;
            }

            if (connect->decode_packet() == -1)
            {
                close_connection(connect);
                break;
            }

            int nbytes = recv(connect->fd, connect->buffer, connect->packet_length, 0);
            if (nbytes <= 0 || nbytes != connect->packet_length)
            {
                close_connection(connect);
                break;
            }

            if (connect->handle_packet() == -1)
            {
                close_connection(connect);
                break;
            }
            connect->reloadStream();
        }
    }

    void close_connection(Connection *connect)
    {
        std::cout << "Закрыто соединение: " << get_ip_by_fd(connect->fd) << std::endl;
        connect->reloadStream();
        connections_count--;
        FD_CLR(connect->fd, &master);
        close(connect->fd);
        connections.remove(connect);
        delete(connect);
    }

    char *get_ip_by_fd(int fd)
    {
        char *ip = new char[20];
        struct sockaddr_in addr;
        socklen_t addr_len = sizeof(struct sockaddr_in);
        getpeername(fd, (struct sockaddr *)&addr, &addr_len);
        strcpy(ip, inet_ntoa(addr.sin_addr));
        return ip;
    }
};

#endif

Connection.cpp

#ifndef CONNECTION
#define CONNECTION

#include "Protocol/Packet.cpp"
#include "DataStream/ByteStream.cpp"

#define MAX_PACKET_LENGTH 5000

class Connection : public ByteStream
{
public:
    int fd = -1;
    char header[7];
    bool read_header = true;
    int packet_id = 0;
    int packet_length = 0;
    int packet_version = 0;

    Connection(int newfd)
    {
        fd = newfd;
    }
    
    ~Connection()
    {
        reloadStream();
        free(buffer);
    }

    void clear_packet()
    {
        read_header = true;
        packet_id = 0;
        packet_length = 0;
        packet_version = 0;
    }

    int decode_packet()
    {
        packet_id = (header[0] << 8);
        packet_id |= (header[1] << 0);
        packet_length = (header[2] << 16);
        packet_length |= (header[3] << 8);
        packet_length |= (header[4] << 0);
        packet_version = (header[5] << 8);
        packet_version |= (header[6] << 0);
        if (packet_length > MAX_PACKET_LENGTH)
        {
            return -1;
        }
        return 1;
    }

    int handle_packet()
    {
        std::cout << fd << std::endl;
        return 1;
    }
};

#endif

ByteStream.cpp

#ifndef BYTESTREAM_H
#define BYTESTREAM_H

class ByteStream
{
public:
    char *buffer = new char;
    unsigned long bufflen = 0;
    unsigned long writeOffset = 0;
    unsigned long readOffset = 0;

    void reloadStream()
    {
        free(buffer);
        buffer = (char*)malloc(1);
        writeOffset = 0;
        readOffset = 0;
    }

    void reallocBuffer(unsigned long length)
    {
        buffer = (char*)realloc(buffer, bufflen + length);
        bufflen += length;
    }

    void setNewBuffer(char *newBuffer)
    {
        reloadStream();
        buffer = newBuffer;
    }

    void writeUInt8(unsigned long number, unsigned long shift = 0)
    {
        reallocBuffer(1);
        buffer[writeOffset] = (number >> shift);
        writeOffset++;
    }

    void writeSInt8(signed long number, unsigned long shift = 0)
    {
        reallocBuffer(1);
        buffer[writeOffset] = (number >> shift);
        writeOffset++;
    }

    void writeUInt16(unsigned short number)
    {
        writeUInt8(number, 8);
        writeUInt8(number, 0);
    }

    void writeSInt16(signed short number)
    {
        writeSInt8(number, 8);
        writeSInt8(number, 0);
    }

    void writeUInt32(unsigned long number)
    {
        writeUInt8(number, 24);
        writeUInt8(number, 16);
        writeUInt8(number, 8);
        writeUInt8(number, 0);
    }

    void writeSInt32(signed long number)
    {
        writeSInt8(number, 24);
        writeSInt8(number, 16);
        writeSInt8(number, 8);
        writeSInt8(number, 0);
    }

    void writeBoolean(bool data)
    {
        if (data)
        {
            writeUInt8(1);
        }
        else
        {
            writeUInt8(0);
        }
    }

    void writeVInt(signed long long number, bool rotate = true)
    {
        number = (number << 1) ^ (number >> 31);
        while(number)
        {
            signed long long b = number & 0x7f;
            if (number >= 0x80)
            {
                b |= 0x80;
            }
            if (rotate)
            {
                rotate = false;
                signed long long lsb = b & 0x1;
                signed long long msb = (b & 0x80) >> 7;
                b >>= 1;
                b = (b & ~0xC0);
                b = b | (msb << 7) | (lsb << 6);
            }
            writeSInt8(b);
            number >>= 7;
        }
    }

    void writeString(std::string data)
    {
        if (!data.size())
        {
            writeSInt32(-1);
        }
        else
        {
            writeSInt32(data.size());
            for (int i = 0; i < data.size(); i++)
            {
                writeUInt8(data[i]);
            }
        }
    }

    void writeStringReference(std::string data)
    {
        writeSInt16(0);
        writeVInt(data.size());
        for (int i = 0; i < data.size(); i++)
        {
            writeUInt8(data[i]);
        }
    }

    unsigned long readUInt8(int shift = 0)
    {
        unsigned int result = 0;
        result = (buffer[readOffset] << shift);
        readOffset++;
        return result;
    }

    signed long readSInt8(int shift = 0)
    {
        return static_cast<int8_t>(readUInt8(shift));
    }

    unsigned short readUInt16()
    {
        unsigned short result = 0;
        result |= readUInt8(8);
        result |= readUInt8(0);
        return result;
    }

    signed short readSInt16()
    {
        return static_cast<signed short>(readUInt16());
    }

    unsigned long readUInt32()
    {
        unsigned long result = 0;
        result |= readUInt8(24);
        result |= readUInt8(16);
        result |= readUInt8(8);
        result |= readUInt8(0);
        return result;
    }

    signed long readSInt32()
    {
        return static_cast<signed long>(readUInt32());
    }

    bool readBoolean()
    {
        return (bool)readUInt8(1);
    }

    signed long long readVInt(bool rotate = true)
    {
        signed long long result = 0;
        signed long shift = 0;
        while (true)
        {
            signed int byte = readSInt8();
            if (rotate && !shift)
            {
                signed long long seventh = (byte & 0x40) >> 6;
                signed long long msb = (byte & 0x80) >> 7;
                signed long long n = byte << 1;
                n = n & ~0x181;
                byte = n | (msb << 7) | seventh;
            }
            result |= (byte & 0x7f) << shift;
            shift += 7;
            if (!(byte & 0x80))
            {
                break;
            }
        }
        return (result >> 1) ^ (-(result & 1));
    }

    std::string readString()
    {
        std::string result = "";
        long length = readSInt32();
        if (length == -1)
        {
            return "";
        }
        for (int i = 0; i < length; i++)
        {
            result += readUInt8();
        }
        return result;
    }

    std::string readStringReference()
    {
        std::string result = "";
        readSInt16();
        long length = readVInt();
        for (int i = 0; i < length; i++)
        {
            result += readUInt8();
        }
        return result;
    }
};

#endif

Packet.cpp

#ifndef PACKET
#define PACKET

class Packet
{
public:
    bool read_header = true;
    char header[7];
    


    int hello()
    {
        return 1;
    }
};

#endif

Helper.cpp

#ifndef HELPER
#define HELPER

#include <random>

class Helper
{
public:
    static void handle_error(bool finish)
    {
        std::cout << strerror(errno) << std::endl;
        if (finish) { exit(1); }
    }
};

#endif

Answer 1

Header files should end in .h or .hpp

Everything except Program.cpp looks like a header file, not a source file. Make sure the filenames of the header files end in .hpp.

See this StackOverflow question for more information about why there is a distinction between header and source files.

Get rid of srand()

I'm not sure why you call srand() at all, as you don't use any random numbers in your code. And if you would need random numbers, prefer to use C++'s random number generators, not C functions.

Don't hardcode the port number

Hardcoding the port on which the TCP server is listening makes this code very inflexible. Why not make it a member variable, and pass it as a parameter to start_binding(), so you can write:

Server serv;
serv.start_binding(12345);
serv.start_listening();

Consider throwing exceptions on errors

When your code encounters an error, it prints a message to std::cout (which should have been std::cerr) and then exits the program immediately. But if your TCP server code would be part of a larger program, you might want to be able to handle those errors in a different way. Instead of doing the error handling yourself, throw an exception (preferrably a custom exception type inherited from one of the standard exception types). This allows the caller to catch the exception and do something useful with it. Alternatively, the caller can choose to ignore it, in which case the program will abort anyway.

What does it do with connections?

In main(), I only see a server being created that listens on some port. What happens with the connections? There is something being done in Server.cpp and Connection.cpp, but the main() function has no control over what is happening. Is Server meant to be a generic TCP server class? If so, I would expect some way for the caller to customize what will happen when a connection is established, for example by registering some callbacks when a connection is accepted and when data is received.

Not all errors are fatal

Many network related functions can return -1 even if there is no fatal error. For example, select() can return EINTR in case a signal was received while waiting. The code should then simply retry calling select(). Also, even if select() returns a positive number, it doesn't mean that a subsequent recv() is guaranteed to succeed. Make sure you understand all the cases where functions might return unexpected values, and make sure you handle them correctly.

Don't assume headers will be completely received using one recv() call

The TCP protocol guarantees that data is sent reliably and in-order, but it doesn't guarantee that if one side send()s 7 bytes, that the other side will receive all those bytes in one call to recv(). The reason is that TCP packets can be split at any point, either by the sender, or by network devices along the route to the receiver. You must be ready to handle the case where you don't receive the whole header in one go.

Don't hardcode sizes

You should not hardcode sizes of things where possible. For example, the call to memset() in start_binding() hardcodes that server_address.sin_zero is 8 bytes. However, there is no such guarantee: this field might be bigger or smaller depending on the operating system and CPU architecture it is running on. Just use sizeof to get the right size:

memset(&server_address.sin_zero, 0, sizeof server_address.sin_zero);

However, even better is not to assume anything about struct sockaddr_in apart from the sin_family, sin_addr and sin_port fields, and just clear the whole struct beforehand:

memset(&server_address, 0, sizeof server_address);
server_address.sin_family = AF_INET;
...

Note that compilers are very good at optimizing away the redundant writes.

Related to this, in handle_receiving, write:

ssize_t bytes = recv(connect->fd, connect->header, sizeof connect->header, 0);

Use the correct types

When you call recv(), you store the result in a variable of type int. However, the return value of recv() is of type ssize_t, which is often larger than an int. This can cause problems if recv() would ever return a value larger than an int can store. While it is unlikely this is even possible in your case, just make sure you use the correct type for variables such that potentially problematic implicit type conversions don't happen.

Also, make no assumptions about the size of short, int, long and long long. These are platform-defined, where platform is a combination of CPU architecture and operating system. For example, on AMD64 architectures, long is 32-bits if you are running Windows, but 64-bits if you are running Linux. If you know you want an integer of a specific size, use one of the standard fixed-width integer types. For example, the return type of readUInt16() should be uint16_t.

Use address-family independent name lookup functions

Instead of using inet_ntoa(), consider using getnameinfo(). This function handles IPv6 as well as IPv4, making it easier to port your program to IPv6 later.

Use std::string instead of C strings where possible

Instead of having get_ip_by_fd() allocate memory for a C string, just return a std::string; this avoids the manual memory allocation, which is error-prone, as proven by the fact that you never free the memory it allocates in your program.

Store Connections by value in connections

STL containers already allocate memory for the elements they store, you often don't need to do your own memory allocation. In class Server, just declare:

std::list<Connection> connections;

And then you can add a new connection by doing:

connections.emplace_back(newfd);

STL containers know how many elements are stored in them

There is no need to have a separate variable connections_count; if you want to know how many connections there are, just call connections.size().

Never mix C and C++ memory allocation functions

In ByteStream, you allocate memory for buffer using new, but when you want to grow it, you call realloc(). This is not guaranteed to work, and on some platforms this will cause your program to crash. Never mix C and C++ memory allocation functions on the same object. Something allocated with new must only be freed with delete, and there is no equivalent of realloc() in C++, so you have to solve this in a different way.

Again, instead of doing memory allocations yourself, make use of an STL container to manage memory for you. You can use std::vector<char> here (but even std::string is possible).

Don't sleep unnecessarily

There is no need to call usleep() inside start_listening(). In fact, you almost never should call usleep() or similar functions in your code; often you either sleep too short or too long, and almost always there is a better way to sleep for the right time if that is necessary at all.

Answer 2

• Is struct sockaddr_in remote_address; really a property of the class Server? I don't see it ever used. If you plan to use it, better make it a member of a Connection.

• What is the purpose of Server::ByteStream stream?

• Manual new/deletememory management is C-ism. You'd be in a much better shape with std::list<Connection> connections; (not a list of pointers, buut a list of connections).

• It is very surprising to see

    packet_id = (header[0] << 8);
    packet_id |= (header[1] << 0);
  

  while there already is

    unsigned short readUInt16()
    {
        unsigned short result = 0;
        result |= readUInt8(8);
        result |= readUInt8(0);
        return result;
    }
  

  Buggy as it is, it shall be used.

• realoadStream() seems very off. I don't see what purpose does it serve, and why do you call it in a destructor.