Weird behaviour on sf::TcpSocket wenn connecting from a connected socket

[Raincode]

Hi,

I came across something very puzzling while working with sockets. Check out the following example:

   
#include <iostream>

#include <SFML/Network.hpp>

int main()
{
    sf::TcpListener l;
    l.listen(2017);

    sf::TcpSocket sock;
    if (sock.connect(sf::IpAddress::LocalHost, 2017) == sf::Socket::Done)
        std::cout << "Connected!\n";

    if (sock.connect({1,2,3,4}, 2017) == sf::Socket::Done)
        std::cout << "???\n";

    char ch;
    std::cin >> ch;
}

This gives me

Connected!
???

I would very much expect the second call to connect to fail. Can anyone explain what's going on? Is this a bug? I understand that further calls to connect disconnect the socket first, but the second connection should obviously fail.

Edit:
Equally confusing behaviour, I would expect the second call to connect to succeed, yet I get no output:

    sf::TcpListener l;
    l.listen(2017);

    sf::TcpSocket s;
    if (s.connect({1,2,3,4}, 2017, sf::milliseconds(200)) == sf::Socket::Done)
        std::cout << "???\n";

    if (s.connect(sf::IpAddress::LocalHost, 2017) == sf::Socket::Done)
        std::cout << "Connected\n";

    char ch;
    std::cin >> ch;

I'm compiling with mingw-w64 GCC 7.2.0 on Windows 10 with SFML from github master


I'm trying to loop through a range of IpAddresses e.g. 192.168.178.2 - 192.168.178.99 and attempt to connect with the same socket for each address. At first I thought I was making some mistake using multiple threads, but then I tried a small example in a single main().

Edit2:
Calling disconnect seems to work for me to "handle" a failed connection, kind of like this:

   
sf::Uint8 b1{192}, b2{168}, b3{178};
socks.push_back(std::make_unique<sf::TcpSocket>());
for (sf::Uint8 b4 = 2; b4 < 100; ++b4) {
    if (socks.back()->connect({b1,b2,b3,b4}, port, sf::milliseconds(100)) == sf::Socket::Done) {
        chat.queue("Connected");
        socks.push_back(std::make_unique<sf::TcpSocket>());
    } else
        socks.back()->disconnect();
}

[eXpl0it3r]

I've kept this thread open since the first time it popped up, as it sounded suspicious. Only now did I have the time to look into it and this indeed seems like a bug.

When the socket is already connected and you try to establish a new connection, the ::connect() actually fails, but then the error code is wrongfully matched (for blocking sockets).

This condition is true and the if-body is entered:

        if (::connect(getHandle(), reinterpret_cast<sockaddr*>(&address), sizeof(address)) == -1)
            return priv::SocketImpl::getErrorStatus();

But the returned status code seems to be WSAEISCONN, I assume since the socket is still connected. Which then gets mapped wrongly to Socket::Done. Even though the comment states that this only applies to non-blocking sockets, there's no check, to ensure that we do have a non-blocking socket.

    switch (WSAGetLastError())
    {
        case WSAEWOULDBLOCK:  return Socket::NotReady;
        case WSAEALREADY:     return Socket::NotReady;
        case WSAECONNABORTED: return Socket::Disconnected;
        case WSAECONNRESET:   return Socket::Disconnected;
        case WSAETIMEDOUT:    return Socket::Disconnected;
        case WSAENETRESET:    return Socket::Disconnected;
        case WSAENOTCONN:     return Socket::Disconnected;
        case WSAEISCONN:      return Socket::Done; // when connecting a non-blocking socket
        default:              return Socket::Error;
    }

Even though the documentation states:

If the socket was previously connected, it is first disconnected.

(Which by the way can be misunderstood when 'disconnected' is understood as adjective instead of verb)
We're not actively doing a disconnect, but the code just tries to run a new connect. I wonder if the assumption that doing a new connection will close the old connection is correct/good enough.

[Laurent]

Isn't there a call to "close()" at the beginning of the function? That one is supposed to disconnet the socket (it kills it and recreates it).

[eXpl0it3r]

No, there isn't. Neither does create() close it or similar.

[Laurent]

Ah, right, I should have looked first.

I guess we can create a github issue to review this code.