Winsock Error After Client Connection 10038

Question

I'm new to Winsock. I have an error that appears at some point mentioned in Main.cpp.

While testing with PuTTy, the client connects to the server, and then the PuTTy window immediately closes.

WSAGetLastError() says 10038 (i.e. an action on a non-socket). For some reason, it appears after the accept() call, when I try to recv() data from the socket returned by accept().

Here the source code:

(it will compile, I'm using PuTTy as the client for testing, also add WS2_32.LIB as input)

Main.cpp

#include "Server.h"

#include <iostream>
using namespace Nikson;

void main() {
    Sock listen(PF_INET,SOCK_STREAM,0);
    Sock client;

    hint.sin_port = htons(54000);
    hint.sin_addr.S_un.S_addr = INADDR_ANY;*/
    SockAddr sa(AF_INET, 53000, INADDR_ANY),bb;

    listen.Bind(sa);
    listen.Listen();    

    client = listen.Accept(bb);
    // **AFTER THIS CLIENT CONNECTS BUT THEN DISCONNECTS**

    listen.Close();
    std::cout << "Connected " << client.GetID() << WSAGetLastError();

    char buf[4096];
    int size;
    while (true) {

        size = client.Recv(buf, sizeof buf, 0);

        std::cout << WSAGetLastError();
        std::cin.get();
        for (int i = 0; i < size; ++i)
            std::cout << buf[i];
    }
}

The program uses a library written by me, which wraps a Winsock SOCKET as a class:

Sock.h

#pragma once
#include <WS2tcpip.h>
#include "SockAddr.h"

namespace Nikson {
    class Sock
    {
    public:
        Sock();
        Sock(int af, int type, int protocol);
        Sock(const SOCKET& base);
        ~Sock();
        int Bind(const SockAddr& sa);

        int Send(const char* buf,
            int len,
            int flags);
        int Recv(char* buf,
            int len,
            int flags);

        int Listen();
        Nikson::Sock Accept(const SockAddr& sa);

        int Connect(const SockAddr& sa);
        int Close();
        int Shutdown(int how);
        SOCKET GetID();
    private:
        SOCKET m_NativeID;
        static inline WSAData s_WSAData;
        static inline int s_SockCount=0;
    };
}

Sock.cpp

#include "Sock.h"
#include <iostream>

//int Nikson::Sock::s_SockCount = 0;

Nikson::Sock::Sock()
{
    if (s_SockCount == 0) 
    {
        WORD ver = MAKEWORD(2, 2);
        if ( WSAStartup(ver, &s_WSAData)!= 0) {
            std::cout << "Cant Init winsock! Quiting" << std::endl;
            return;
        }
    }
    ++s_SockCount;
}

Nikson::Sock::Sock(int af,int type, int protocol)
{
    if (s_SockCount == 0)
    {
        WORD ver = MAKEWORD(2, 2);
        if (WSAStartup(ver, &s_WSAData) != 0) {
            std::cout << "Cant Init winsock! Quiting" << std::endl;
            return;
        }
    }
    ++s_SockCount;
    m_NativeID = socket(af, type, protocol);

    if (m_NativeID == INVALID_SOCKET) {
        std::cout << "Invalid socket";
    }
}

Nikson::Sock::Sock(const SOCKET& base)
{
    if (s_SockCount == 0)
    {
        WORD ver = MAKEWORD(2, 2);
        if (WSAStartup(ver, &s_WSAData) != 0) {
            std::cout << "Cant Init winsock! Quiting" << std::endl;
            return;
        }
    }
    ++s_SockCount;
    m_NativeID = base;
}

Nikson::Sock::~Sock()
{
    closesocket(m_NativeID);
    --s_SockCount;
    if (s_SockCount == 0) {
        WSACleanup();
    }
}

int Nikson::Sock::Bind(const SockAddr& sa)
{
    return bind(m_NativeID, (sockaddr*)&sa.data, sizeof(sa.data));
}

int Nikson::Sock::Send(const char* buf, int len, int flags)
{
    return send(m_NativeID,buf,len,flags);
}

int Nikson::Sock::Recv(char* buf, int len, int flags)
{
    return recv(m_NativeID,buf,len,flags);
}

int Nikson::Sock::Listen()
{
    return listen(m_NativeID, SOMAXCONN);
}

Nikson::Sock Nikson::Sock::Accept(const SockAddr& sa)
{
    int size = sizeof(sa);
    return Nikson::Sock(accept(m_NativeID,(sockaddr*)&sa.data,&size));
}

int Nikson::Sock::Connect(const SockAddr& sa)
{
    return connect(m_NativeID, (sockaddr*)&sa.data, sizeof(sa));
}

int Nikson::Sock::Close()
{
    return closesocket(m_NativeID);
}

int Nikson::Sock::Shutdown(int how)
{
    return shutdown(m_NativeID,how);
}

SOCKET Nikson::Sock::GetID()
{
    return m_NativeID;
}

SockAddr.h

#pragma once
#include <WS2tcpip.h>
#include <string>

namespace Nikson {
    struct SockAddr
    {
        SockAddr() = default;
        SockAddr(int family, int port, const std::string& ip);
        SockAddr(int family, int port, unsigned long ip);
        sockaddr_in data;
    };
}

SockAddr.cpp

#include "SockAddr.h"

Nikson::SockAddr::SockAddr(int family, int port, const std::string& ip)
{
    ZeroMemory(this, sizeof * this);

    data.sin_family = family;
    data.sin_port = htons(port);
    inet_pton(family, ip.c_str(), &(data.sin_addr));
}

Nikson::SockAddr::SockAddr(int family, int port, unsigned long ip)
{
    /*ZeroMemory(this, sizeof * this);*/
    data.sin_family = family;
    data.sin_port = htons(port);
    data.sin_addr.S_un.S_addr = ip;
}

EDIT:

This code does not use my library classes, and works fine:

#include <iostream>
#include <string>
#include <WS2tcpip.h>
#include <thread>
#pragma comment (lib,"ws2_32.lib")

using namespace std;

void main() 
{
    //Initialize windsock
    WSADATA wsData;
    WORD ver = MAKEWORD(2, 2);
    int wsok = WSAStartup(ver, &wsData);
    if (wsok != 0) {
        cerr << "Cant Init winsock! Quiting" << endl;
        return;
    }

    //Create a socket
    
    SOCKETlistening = socket(AF_INET, SOCK_STREAM, 0);
    if (listening == INVALID_SOCKET) {
        cerr << "Cant crerate a socket, Quiting" << endl;
    }

    //Bind the  an ip adress and port to socket
    sockaddr_in hint;
    hint.sin_family = AF_INET;
    hint.sin_port = htons(54000);
    hint.sin_addr.S_un.S_addr = INADDR_ANY;//could also use inet_pton.....

    bind(listening, (sockaddr*)&hint, sizeof(hint));

    // Tell winsock the socket is for listening
    listen(listening, SOMAXCONN);

    // wait for connection
    sockaddr_in client;
    int clientSize = sizeof(client);
    SOCKET clientSocket = accept(listening, (sockaddr*)&client, &clientSize);
    if (clientSocket == INVALID_SOCKET){}

    //char host[NI_MAXHOST];
    //char service[NI_MAXHOST];
    //ZeroMemory(host, NI_MAXHOST);
    //ZeroMemory(service, NI_MAXHOST);

    //if (getnameinfo((sockaddr*)&client, sizeof(client), host, NI_MAXHOST, service, NI_MAXSERV, 0) == 0) {
    //  cout << host << " connected on port" << service << std::endl;
    //}
    //else {
    //  inet_ntop(AF_INET, &client.sin_addr, host, NI_MAXHOST);
    //  cout << host<<" connected on port "<<ntohs(client.sin_port)<<endl;
    //}// close listening socket

    closesocket(listening);

    // while loop: accept and echo message back to client
    bool running = true;
    auto reciver = [&clientSocket,&running]() {
        char buf[4096];
        while(running) {
            ZeroMemory(buf, 4096);
            int byteRecived = recv(clientSocket, buf, 4096, 0);
            for (int i = 0; i < byteRecived; ++i) {
                cout << buf[i];
            }
            cout << "\n";
            if (byteRecived == SOCKET_ERROR) {
                cerr << "recv() error";
                break;
            }
            if (byteRecived == 0) {
                cerr << "CLient disconnected";
                break;
            }
        }
        running = false;
    };

    string letter;
    
    thread listener(reciver);
    while (running){
        letter = "";
        cin >> letter;
        letter += "\n";
        send(clientSocket, letter.data(), letter.size(), 0);

    }

    //close the socket
    closesocket(clientSocket);

    //Shutdown winsock
    WSACleanup();
}

Answer 1

Your Sock class lacks proper copy/move semantics.

When copying a Sock object to another Sock, you are not managing your s_SockCount refcount, and more importantly you are not duplicating the socket that m_NativeID refers to.

Since m_NativeID refers to an OS-allocated SOCKET resource, the default compiler-generated copy/move semantics are NOT sufficient to manage the socket correctly.

Sock::Accept() returns a temporary Sock object. When that object goes out of scope, its destructor closes the socket that m_NativeID refers to.

The statement client = listen.Accept(bb); will copy/move that temporary Sock object to client, and then immediately destroy the temporary object. Without valid copy/move semantics implemented in Sock, the default behavior will cause both Sock objects to refer to the same socket.

So, when the temporary Sock object is destroyed, it closes the socket, leaving client referring to an invalid socket. That is why you are getting the 10038 (WSAENOTSOCK) error on operations performed on client.

To fix this, you need to add copy/move semantics to your Sock class, eg:

class Sock
{
public:
    Sock();
    Sock(int af, int type, int protocol);
    Sock(SOCKET base); // <-- remove const reference!
    Sock(const Sock &src); // <-- add this!
    Sock(Sock&& src); // <-- add this!
    ...
    Sock& operator=(Sock rhs); // <-- add this!
    ...
};

Nikson::Sock::Sock()
    : m_NativeID(INVALID_SOCKET) // <-- add this!
{
    // init WSA, increment s_SockCount ...
}

Nikson::Sock::Sock(int af, int type, int protocol)
    : m_NativeID(INVALID_SOCKET) // <-- add this!
{
    // init WSA, increment s_SockCount ...
    // create socket ...
}

Nikson::Sock::Sock(SOCKET base)
    : m_NativeID(base) // <-- add this!
{
    // init WSA, increment s_SockCount ...
    //m_NativeID = base; // <-- remove this!
}

// add this!
Nikson::Sock::Sock(const Sock& base)
    : m_NativeID(INVALID_SOCKET)
{
    // init WSA, increment s_SockCount ...

    if (base.m_NativeID != INVALID_SOCKET) {
        WSAPROTOCOL_INFO info = {};
        if (WSADuplicateSocket(base.m_NativeID, GetCurrentProcessId(), &info) == SOCKET_ERROR) {
            std::cout << "Cant duplicate socket!" << std::endl;
            return;
        }
        m_NativeID = WSASocket(FROM_PROTOCOL_INFO, FROM_PROTOCOL_INFO, FROM_PROTOCOL_INFO, &info, 0, 0);
        if (m_NativeID == INVALID_SOCKET) {
            std::cout << "Invalid socket";
        }
    }
}

// add this!
Nikson::Sock::Sock(Sock&& base)
    : m_NativeID(INVALID_SOCKET)
{
    // init WSA, increment s_SockCount ...

    m_NativeID = base.m_NativeID;
    base.m_NativeID = INVALID_SOCKET;
}

Nikson::Sock::~Sock()
{
    //closesocket(m_NativeID); // <-- replace this...
    Close();                   // <-- with this!

    // decrement s_SockCount, cleanup WSA ...
}

...

int Nikson::Sock::Close()
{
    int ret = closesocket(m_NativeID);
    m_NativeID = INVALID_SOCKET; // <-- add this!
    return ret;
}

...

// add this!
Nikson::Sock& Nikson::Sock::operator=(Sock rhs)
{
    std::swap(m_NativeID, rhs.m_NativeID);
    return *this;
}

---

On a side note, there are some other problems with your code:

• PF_INET should be AF_INET.

• Your Sock constructors are not initializing m_NativeID if something goes wrong. See fixes above.

• Sock::Bind() and Sock::Connect() are casting away constness they shouldn't be. bind() and connect() expect a const sockaddr*. You are also passing the wrong size value:

  int Nikson::Sock::Bind(const SockAddr& sa)
  {
     return bind(m_NativeID, (const sockaddr*)&sa.data, sizeof(sa.data));
  }
  
  int Nikson::Sock::Connect(const SockAddr& sa)
  {
    return connect(m_NativeID, (const sockaddr*)&sa.data, sizeof(sa.data));
  }
  
  

• Sock::Accept() is modifying a const object, which is undefined behavior. accept() takes a non-const sockaddr*, because it modifies the sockaddr it is given, but you have that wrapped inside a const SockAddr object. You need to drop the const, so the caller can actually receive the client's info. You are also setting size to the wrong value:

  Nikson::Sock Nikson::Sock::Accept(SockAddr& sa)
  {
    int size = sizeof(sa.data);
    return Sock(accept(m_NativeID, (sockaddr*) &sa.data, &size));
  }
  

  You might also consider overloading Sock::Accept() to let the SockAddr be optional, if the caller doesn't want it:

  Nikson::Sock Nikson::Sock::Accept()
  {
    return Sock(accept(m_NativeID, NULL, NULL));
  }
  

• Sock::Close() is not setting the m_NativeID member to INVALID_SOCKET. You are allowing a Sock to be closed before it is destroyed, but you need to make sure the destructor can still act properly. Calling closesocket() on a SOCKET that is not INVALID_SOCKET but doesn't refer to a valid socket is undefined behavior. See fix above.

• Your SockAddr class only supports sockaddr_in (AF_INET), so it doesn't really make sense to let callers set its family value. Just hard-code it instead. On the other hand, if you want to support both AF_INET (IPv4) and AF_INET6 (IPv6) IP addresses, you need to change the data member to sockaddr_storage instead, and then cast it to sockaddr_in or sockaddr_in6 as needed.

• I would strongly suggest moving the calls to WSAStartup() and WSACleanup() into a separate class, and then create an instance of that class at the top of main() before doing anything else with your Sock class. Those function calls really don't belong in the Sock class itself.