Socket library based on winsock

Question

I wrote a socket library in C++, to wrap Winsock. I found it a bit tiring using winsock2 - I'd say it's a bit complex, compared to, for instance, the Python socket library.

My code, header file:

#pragma once
#pragma comment(lib,"WS2_32.lib")
#include <winsock2.h>
#include <Ws2tcpip.h>
#include <windows.h>
#include <string>
#include <iostream>

enum flags{
    TCPType = 0,
    UDPType = 1
};

inline flags operator|(const flags& x, const flags& y){
    return static_cast<flags>(static_cast<int>(x)|static_cast<int>(y));
}

class Socket
{
private:
    std::string ip;
    std::string port;

    SOCKET* currentSocket = nullptr;

    bool isTcp;

public:
    Socket(flags f = TCPType);
    ~Socket();

    bool Connect(const std::string& address, const std::string& port);
    bool BindToPort(int p);
    bool Listen();
    bool isTcpType();

    int ReceiveAll(void* buffer, int size);
    int Receive(void* buffer, int size);
    int Sendall(void* buffer, int size);
    int Send(void* buffer, int size);

    std::string  getAddress() const;
    std::string  getPort() const;

    Socket* AcceptConnection();
    void close();

    static bool InitWinsock();
};

And the source file:

#include "pch.h"
#include "Socket.h"

Socket::Socket(flags f)
{
    isTcp = !f;

    auto connectionType = SOCK_DGRAM;
    auto b = 0;

    if (isTcp) {
        connectionType = SOCK_STREAM;
        b = IPPROTO_TCP;
    }

    SOCKET mainSocket = socket(AF_INET, connectionType, b);
    if (mainSocket == INVALID_SOCKET)
    {
        std::cerr << "Error creating socket: " << WSAGetLastError() << '\n';
        WSACleanup();
    }

    currentSocket = new SOCKET(mainSocket);
}

Socket::~Socket()
{
    if(currentSocket != nullptr)
        delete currentSocket;
}

bool Socket::Connect(const std::string& address, const std::string& port){
    addrinfo hints, *result = nullptr;

    ZeroMemory(&hints, sizeof(hints));
    hints.ai_family = AF_INET;
    hints.ai_socktype = SOCK_STREAM;
    hints.ai_protocol = IPPROTO_TCP;

    int iResult = getaddrinfo(address.c_str(), port.c_str(), &hints, &result);
    if (iResult != 0) {
        std::cerr << "getaddrinfo failed with error: " << iResult << '\n';
        WSACleanup();
        return false;
    }

    addrinfo *ptr = result;

    iResult = connect(*currentSocket, ptr->ai_addr, (int)ptr->ai_addrlen);
    if (iResult == SOCKET_ERROR) {
        closesocket(*currentSocket);
        *currentSocket = INVALID_SOCKET;
    }

    return 0;
}

bool Socket::BindToPort(int p){
    if (currentSocket == nullptr)
        return false;

    sockaddr_in service;
    memset(&service, 0, sizeof(service));
    service.sin_family = AF_INET;
    InetPton(AF_INET, (PCWSTR)("127.0.0.1"), &service.sin_addr.s_addr);
    service.sin_port = htons(p);

    if(bind(*currentSocket, (SOCKADDR *)& service, sizeof(sockaddr)) == SOCKET_ERROR)
    {
        std::cerr << "Error during binding socket: " << WSAGetLastError() << '\n';
        closesocket(*currentSocket);
        return false;
    }

    this->port = p;

    return true;
}

bool Socket::Listen() {
    if (currentSocket == nullptr)
        return false;

    if(listen(*currentSocket, 1) == SOCKET_ERROR)
        std::cerr << "Error during trying to listen on socket: " << WSAGetLastError() << '\n';

    ip = "127.0.0.1";

    return true;
}

bool Socket::isTcpType(){
    return isTcp;
}

int Socket::ReceiveAll(void* buffer, int size) {
    int bytesLeft = size, current = 0;

    while (bytesLeft > 0) {
        int bytesRecv = recv(*currentSocket, static_cast<char*>(buffer), size, 0);

        if (bytesRecv < 0)
            return -1;

        bytesLeft -= bytesRecv;
    }

    return size;
}


int Socket::Receive(void* buffer, int size){
    if (currentSocket == nullptr)
        return -1;

    int bytesRecv = SOCKET_ERROR;

    while(bytesRecv == SOCKET_ERROR)
    {
        bytesRecv = recv(*currentSocket, static_cast<char*>(buffer), size, 0);

        if(bytesRecv == WSAECONNRESET)
            break;
    
        if (bytesRecv < 0)
            return -1;
    }

    return bytesRecv;
}

int Socket::Sendall(void* buffer, int size) {
    if (currentSocket == nullptr)
        return -1;

    int bytesSent = 0, counter = 0;

    while (counter < size) {
        bytesSent = send(*currentSocket, static_cast<const char*>(buffer), size, 0);
        counter += bytesSent;
    }

    return size;
}

int Socket::Send(void* buffer, int size){
    if (currentSocket == nullptr)
        return -1;

    int bytesSent, bytesRecv = SOCKET_ERROR;
    bytesSent = send(*currentSocket, static_cast<const char*>(buffer), size, 0);

    return bytesSent;
}

std::string  Socket::getAddress() const{
    return ip;
}

std::string  Socket::getPort() const{
    return port;
}

Socket* Socket::AcceptConnection(){
    if (currentSocket == nullptr)
        return false;

    SOCKET acceptSocket = SOCKET_ERROR;

    while (acceptSocket == SOCKET_ERROR)
    {
        acceptSocket = accept(*currentSocket, NULL, NULL);
    }

    Socket* result = new Socket(*this);
    result->currentSocket = new SOCKET(acceptSocket);

    return result;
}

void Socket::close() {
    closesocket(*currentSocket);
    delete currentSocket;
    WSACleanup();
}

bool Socket::InitWinsock() {
    WSADATA wsaData;

    int result = WSAStartup(MAKEWORD(2, 2), &wsaData);
    if (result != NO_ERROR) {
        std::cerr << "Error during initializing WinSock: " << WSAGetLastError() << '\n';
        return false;
    }

    return true;
}

Can you add some more context about how you want this code to be reviewed? Why did you choose to do things the way you did, what do you think can be improved? — Ben A
– Ben A, Commented May 9, 2020 at 0:27

Loki Astari · Accepted Answer · 2020-05-09 00:05:38Z

2

No need for this in modern C++:

Socket* AcceptConnection();

You should not be returning RAW pointers.

answered May 9, 2020 at 0:05

Loki Astari

97.7k5 gold badges126 silver badges341 bronze badges

Add a comment |

Toby Speight · Accepted Answer · 2022-01-07 15:12:30Z

This enum is poorly named:

enum flags{
    TCPType = 0,
    UDPType = 1
};

I'd expect something called flags to be a bitmask of independent values that can be combined using |.

This would be better as an enum class:

enum class protocol : char {
    TCPType,
    UDPType,
};

Or just use the underlying library's SOCK_STREAM and SOCK_DGRAM.

Why should isTcpType() modify the object? I think it should be declared const.

Why does getPort() return a string? A number is more useful, and can easily be converted to string by the caller if needed.

Naming is inconsistent: ReceiveAll(), but Sendall(); and isTcpType() but Listen().

There's no need to use platform-specific ZeroMemory() when standard library function is sufficient:

std::memset(&hints, 0, sizeof hints);

It's unclear to return a boolean from a function with pointer return type:

Socket* Socket::AcceptConnection(){
    if (currentSocket == nullptr)
        return false;

Although standard conversions will promote false to a null pointer, it's much easier for readers if you write return nullptr;.

A better approach altogether would be to arrange that a Socket object never has null currentSocket (perhaps use the Null Object pattern to provide a suitable object to represent the unconnected state).

In Listen(), the error handling is just:

if(listen(*currentSocket, 1) == SOCKET_ERROR)
    std::cerr << "Error during trying to listen on socket: " << WSAGetLastError() << '\n';

Firstly, a library has no business writing to std::cerr - it should leave such reporting to the calling application, which may be able to correct the situation without bothering the user.

Secondly, we should not continue to return a success indication in this case.

The constructor is flawed:

Socket::~Socket()
{
    if(currentSocket != nullptr)
        delete currentSocket;
}

Firstly, there's nothing wrong with deleting a null pointer, so the test is pointless. On the other hand, what should be inside that test is closing the socket. As it is, the destructor is likely leaking resources.

Socket::~Socket()
{
    if (currentSocket) {
        close();
    }
}

close() also needs fixing so that it doesn't retain a dangling pointer when it deletes the socket. If close() is called again, we have UB.

void Socket::close() {
    if (!currentSocket) {
        return;
    }

    closesocket(*currentSocket);
    delete currentSocket;
    WSACleanup();
    currentSocket = nullptr;  // or point to "null socket" object
}

With this fix, the destructor can unconditionally call close() without problems.

The logic of Sendall() looks broken. After a partial write, we should be attempting to send only the remaining data, rather than retransmitting the whole buffer.

JDługosz · Accepted Answer · 2022-01-07 17:26:21Z

Why does CurrentSocket need to be a pointer? It looks like the SOCKET handle type already has a representation for a closed or invalid socket, so just store that directly in the class. All you're doing is providing an additional nullable state. If you really do need that, use std::optional and avoid the overhead of a block of dynamic memory.

Your destructor should just be:
~Socket() { close(); }

Use the std::error_code for defining errors and their associated message strings. You can return them from operations where the caller needs to check for errors, or throw them in a special exception type that goes with it, for functions that should just work.

Your Accept function returns a raw pointer to a dynamically allocated instance of your class. Why not just return by value? If you do need a pointer, use a unique_ptr or something.

The buffer parameter for sending should be const. For example, s->Send("END\r\n",5); will not compile!

What happens if you copy the Socket object? It will crash and burn when closing since you copied a pointer allocated with new. Remember the rule of three ! If you had to define any of the special members (destructor in your case), you need to provide all of them.

Stack Exchange Network

Socket library based on winsock

3 Answers 3

You must log in to answer this question.

Hot Network Questions

Socket library based on winsock

3 Answers 3

You must log in to answer this question.

Related

Hot Network Questions