Я создаю TCP-сервер на С++, и сейчас это тестовая версия без логики для обработки транзакций между сервером и клиентом. Пожалуйста, проверьте, есть ли какие-либо проблемы, связанные с утечкой памяти, или что-то еще, что я могу сделать лучше.

Программа.cpp

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

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

Сервер.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

Соединение.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

Пакет.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

Файлы заголовков должны заканчиваться на .h или .hpp.

Все, кроме Program.cpp, выглядит как файл заголовка, а не исходный файл. Убедитесь, что имена файлов заголовков заканчиваются на .hpp.

Видеть этот вопрос StackOverflow для получения дополнительной информации о том, почему существует различие между заголовочными и исходными файлами.

Избавиться от srand()

Я не уверен, почему ты звонишь srand() вообще, поскольку вы не используете случайные числа в своем коде. И если вам нужны случайные числа, предпочитайте использовать Генераторы случайных чисел C++а не C-функции.

Не жестко кодируйте номер порта

Жесткое кодирование порта, который прослушивает TCP-сервер, делает этот код очень негибким. Почему бы не сделать его переменной-членом и передать в качестве параметра start_binding()так что вы можете написать:

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

Рассмотрите возможность создания исключений при ошибках

Когда ваш код сталкивается с ошибкой, он выводит сообщение на std::cout (что должно было быть std::cerr), а затем немедленно выходит из программы. Но если ваш код TCP-сервера будет частью более крупной программы, вы можете захотеть иметь возможность обрабатывать эти ошибки другим способом. Вместо того, чтобы самостоятельно обрабатывать ошибки, throw исключение (предпочтительно пользовательский тип исключения, унаследованный от одного из стандартные типы исключений). Это позволяет вызывающей стороне перехватить исключение и сделать с ним что-то полезное. В качестве альтернативы вызывающая сторона может проигнорировать его, и в этом случае программа все равно прервется.

Что он делает со связями?

В main(), я вижу только создаваемый сервер, который слушает какой-то порт. Что происходит со связями? Что-то делается в Server.cpp и Connection.cpp, но main() функция не контролирует происходящее. Является Server предназначен для общего класса TCP-сервера? Если это так, я бы ожидал, что вызывающая сторона каким-то образом настроит то, что произойдет, когда соединение будет установлено, например, путем регистрации некоторых обратных вызовов, когда соединение будет принято и когда будут получены данные.

Не все ошибки фатальны

Многие функции, связанные с сетью, могут возвращать -1, даже если нет фатальной ошибки. Например, select() может вернуться EINTR в случае, если сигнал был получен во время ожидания. Затем код должен просто повторить вызов select(). Также, даже если select() возвращает положительное число, это не означает, что последующий recv() гарантированно увенчается успехом. Убедитесь, что вы понимаете все случаи, когда функции могут возвращать непредвиденные значения, и убедитесь, что вы правильно их обрабатываете.

Не думайте, что заголовки будут полностью получены с использованием одного recv() вызов

TCP-протокол гарантирует, что данные отправляются надежно и по порядку, но не гарантирует, что если одна сторона send()s 7 байтов, что другая сторона получит все эти байты за один вызов recv(). Причина в том, что TCP-пакеты могут быть разделены в любой точке либо отправителем, либо сетевыми устройствами на пути к получателю. Ты должен будьте готовы обработать случай, когда вы не получите весь заголовок за один раз.

Не задавайте размеры жестко

Вы не должны жестко кодировать размеры вещей, где это возможно. Например, призыв к memset() в start_binding() хардкоды, которые server_address.sin_zero составляет 8 байт. Однако такой гарантии нет: это поле может быть больше или меньше в зависимости от операционной системы и архитектуры ЦП, на которой оно работает. Просто используйте sizeof чтобы получить правильный размер:

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

Однако еще лучше ничего не предполагать о struct sockaddr_in отдельно от sin_family, sin_addr а также sin_port полей и просто предварительно очистите всю структуру:

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

Обратите внимание, что компиляторы очень хорошо оптимизируют избыточную запись.

В связи с этим в handle_receivingзаписывать:

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

Используйте правильные типы

Когда вы звоните recv()вы сохраняете результат в переменной типа int. Однако возвращаемое значение recv() имеет тип ssize_tкоторый часто превышает int. Это может вызвать проблемы, если recv() когда-либо вернет значение больше, чем int может хранить. Хотя маловероятно, что это даже возможно в вашем случае, просто убедитесь, что вы используете правильный тип для переменных, чтобы не происходило потенциально проблемных неявных преобразований типов.

Кроме того, не делайте никаких предположений о размере short, int, long а также long long. Они зависят от платформы, где платформа представляет собой комбинацию архитектуры ЦП и операционной системы. Например, на архитектурах AMD64 long 32-битная, если вы используете Windows, и 64-битная, если вы используете Linux. Если вы знаете, что вам нужно целое число определенного размера, используйте один из стандартных целочисленные типы фиксированной ширины. Например, возвращаемый тип readUInt16() должно быть uint16_t.

Используйте независимые от семейства адресов функции поиска имен

Вместо использования inet_ntoa()рассмотрите возможность использования getnameinfo(). Эта функция работает как с IPv6, так и с IPv4, что упрощает перенос вашей программы на IPv6 в дальнейшем.

Использовать std::string вместо строк C, где это возможно

Вместо того, чтобы иметь get_ip_by_fd() выделить память для строки C, просто верните std::string; это позволяет избежать ручного выделения памяти, которое подвержено ошибкам, о чем свидетельствует тот факт, что вы никогда не освобождаете память, которую он выделяет в вашей программе.

Хранить Connections по значению в connections

Контейнеры STL уже выделяют память для хранимых в них элементов, вам часто не нужно делать собственное выделение памяти. В class Serverпросто объявите:

std::list<Connection> connections;

И затем вы можете добавить новое соединение, выполнив:

connections.emplace_back(newfd);

Контейнеры STL знают, сколько элементов хранится в них

Нет необходимости иметь отдельную переменную connections_count; если вы хотите узнать, сколько подключений, просто позвоните connections.size().

Никогда не смешивайте функции распределения памяти C и C++

В ByteStreamвы выделяете память для buffer с использованием newно когда вы хотите вырастить его, вы звоните realloc(). Это не гарантирует работу, и на некоторых платформах это приведет к сбою вашей программы. Никогда не смешивайте функции выделения памяти C и C++ для одного и того же объекта. Что-то выделяется с new должны быть освобождены только с deleteи нет эквивалента realloc() в C++, поэтому вам нужно решить это по-другому.

Опять же, вместо того, чтобы самостоятельно распределять память, используйте контейнер STL для управления памятью. Вы можете использовать std::vector<char> здесь (но даже std::string возможно).

Не спи без надобности

Нет необходимости звонить usleep() внутри start_listening(). На самом деле, вы почти никогда не должны звонить usleep() или аналогичные функции в вашем коде; часто вы либо спите слишком мало, либо слишком долго, и почти всегда есть лучший способ спать в нужное время, если это вообще необходимо.