Winsock connect call crashing if multiple threads running, works fine in one thread?

Question

I have an application that needs to download a variable list of files (changes based on user, what's changed, etc). The list can be very short or very long (1000's of files). I start X worker threads with a list for each thread to download. If I run 1 thread, it all works fine. If I run >1 thread, it "may" crash in __acrt_lock (I don't call that, however). The problem is on the winsock connect call. If I comment out that call, it works (obviously does not download the file, but it doesn't crash).

THIS USED TO WORK UNDER THE OLD v110 COMPILER CHAIN. I have upgraded to the v141 chain, and now the problem is occurring. I am using the multi-threaded libraries, of course.

I have "shortcut" the code with premature return statements in various places, and have determined that the single line calling the winsock function "connect" is the problem. There are no global variables used (only private local storage to the thread).

bool Socket::connect(const char * adrs, int port) {

    lastErrCode = 0;
    myIP = adrs;
    myPort = port;

    if (inet_addr(adrs) == INADDR_NONE) {
        getHostByName(adrs, myIP);
    }
    else {
        myIP = adrs;
    }

    if ((me = socket(AF_INET, SOCK_STREAM, 0)) == INVALID_SOCKET) {
        lastErrCode = WSAGetLastError();
        return (false);
    }

    SOCKADDR_IN sock;
    sock.sin_family = PF_INET;
    sock.sin_port = htons(port);
    sock.sin_addr.s_addr = inet_addr(myIP.str());
    return(false); //:DEBUG:

    if (::connect(me, (SOCKADDR*)&sock, sizeof(SOCKADDR)) == SOCKET_ERROR) {
        lastErrCode = WSAGetLastError();
        closesocket(me);
        me = INVALID_SOCKET;
        return (false);
    }

    return (true);

}

There's nothing in this question that proves that "the winsock function connect is the problem". Just because a C++ program crashes in one particular function doesn't mean that's where the bug is. C++ does not work this way. The bug can be anywhere in the code that was executed prior to the crash. Which is why stackoverflow.com's [help] tells you that you must prepare a [mcve]. Without a [mcve] that anyone can compile, run, and reproduce the problem, it's unlikely that anyone will be able to help you. — Sam Varshavchik, Dec 20 '18 at 18:24
Are you able to reproduce this with a debug build? Could be some good information to be had. Other than that, I'm not sure `gethostbyname`is reentrant. Wait... That's not the `gethostbyname` I'm used to. Got nothing for you. [mcve], please. — user4581301, Dec 20 '18 at 18:24
Someone has voted to close based on typo. Whoever did that, could you throw the asker and I a bone and let us in on the typo? I'm not seeing it. — user4581301, Dec 20 '18 at 18:29
`gethostbyname()` is indeed not reentrant. It returns a pointer to a static `hostent` structure that is not guaranteed to be safe if `gethostbyname()` is called by multiple threads at the same time (though, in the case of WinSock, it is safe, as the struct is allocated in TLS memory). This is one of many reasons why `gethostbyname()` is deprecated in favor of `getaddrinfo()`, which you should be using anyway. — Remy Lebeau, Dec 20 '18 at 21:41
Also, have no idea why StackOverflow has "lost" my name and now I'm user3382059? — Bret Levy, Dec 21 '18 at 06:48

score 0 · Answer 1 · answered Dec 21 '18 at 06:44

Thanks for the responses everyone. I have solved the issue! My main code did not have a stupid windows message pump (I typically run these types of servers on Linux). I added this to the main code, and now it works perfectly.

#ifdef _WIN32
    MSG msg;
    while (!Stopped && GetMessage(&msg, (HWND)NULL, 0, 0)) {
        TranslateMessage(&msg);
        DispatchMessage(&msg);
    }
#else
    while (!Stopped) {
        Sleep(1000);
    }
#endif

Sorry I didn't put enough code out there to actually run the example, but it did 100% crash in the winsock code in both debug and release builds. Always with a crash in RtlHeapxxx() or __acrt_lock(). With the message pump, I can repeatedly run it no problem.

As for the gethostbyname issue, I was calling my internal GetHostByName method (case change) and it calls the newer getaddrinfo().

UINT32 Socket::getHostByName(const char * name, Data& ip) {
    UINT32 iadrs = getHostByName(name);
    ip.format("%d.%d.%d.%d", (iadrs >> 24) & 0xFF, (iadrs >> 16) & 0xFF, (iadrs >> 8) & 0xFF, iadrs & 0xFF);
    return (iadrs);
}

UINT32 Socket::getHostByName(const char * name) {

    if (lastAddress == name) {
        return(lastResolution);
    }

    struct addrinfo hints;
    memset(&hints, 0, sizeof(hints));
    hints.ai_family = AF_UNSPEC;
    hints.ai_socktype = SOCK_STREAM;
    hints.ai_protocol = IPPROTO_TCP;

    struct addrinfo * result = nullptr;
    if (getaddrinfo(name, "80", &hints, &result) != 0) {
        if (result != nullptr) {
            freeaddrinfo(result);
        }
        return (0);
    }

    for (struct addrinfo * ptr = result; ptr != nullptr; ptr = ptr->ai_next) {
        if (ptr->ai_family == AF_INET) {
            struct sockaddr_in * ip = (struct sockaddr_in *) ptr->ai_addr;
            UINT32 iadrs = (ip->sin_addr.S_un.S_un_b.s_b1 << 24) | (ip->sin_addr.S_un.S_un_b.s_b2 << 16) | (ip->sin_addr.S_un.S_un_b.s_b3 << 8) | (ip->sin_addr.S_un.S_un_b.s_b4);
            freeaddrinfo(result);
            lastAddress = name;
            lastResolution = iadrs;
            return (iadrs);
        }
    }

    freeaddrinfo(result);
    return (0);

}

Winsock connect call crashing if multiple threads running, works fine in one thread?

1 Answers1