Visual Studio release mode blocks code execution that is executed in debug mode. Using WinHTTP and Multi-Threading

Question

I'm using WinHTTP to make a GET request and I'm using WinHttpOpen asynchronously with a callback function.

HINTERNET hSession = WinHttpOpen(L"", WINHTTP_ACCESS_TYPE_NO_PROXY, WINHTTP_NO_PROXY_NAME, WINHTTP_NO_PROXY_BYPASS, WINHTTP_FLAG_ASYNC);
if (hSession) {
    void* phSession_Callback = WinHttpSetStatusCallback(hSession, (WINHTTP_STATUS_CALLBACK)hSession_Callback, WINHTTP_CALLBACK_FLAG_ALL_NOTIFICATIONS, 0);
    if (phSession_Callback == WINHTTP_INVALID_STATUS_CALLBACK) {
        //error handling code
    }
}

At the end of my GET function I use a bitfield to store data. In that data there is a flag that says when the request completed successfully (that is the if check).

while (1) {
        std::this_thread::sleep_for(std::chrono::microseconds(1));
        if ((g_nBitFlags >> 3) & 1) {
            WinHttpCloseHandle(hRequest);
            WinHttpCloseHandle(hConnection);
            WinHttpCloseHandle(hSession);
            if ((g_nBitFlags >> 2) & 1) {
                return -1;
            } else if ((g_nBitFlags >> 1) & 1) {
                return 0;
            } else if ((g_nBitFlags >> 0) & 1) {
                return g_nBitFlags >> 4;
            }
        }
    }

And here is my hSession callback which fires the function (as seperate thread) which sets the flag if its recieved successfully or not. (Note that this thread is not waited to be joined, its completely seperate even if the http request finished before that worker function)

void WINAPI hSession_Callback(
    IN HINTERNET hInternet,
    IN DWORD_PTR dwContext,
    IN DWORD dwInternetStatus,
    IN LPVOID lpvStatusInformation,
    IN DWORD dwStatusInformationLength
) {
    case WINHTTP_CALLBACK_STATUS_DATA_AVAILABLE: {
        std::thread(dataAvail_worker, &httplib::g_nBitFlags, lpvStatusInformation, hInternet).detach();
    }
}

Requested Edit: dataAvail_worker:

    #include "pch.h"

#include "dataAvail.h"

namespace httplib {
    extern char* g_szDataBuffer;
}

void dataAvail_worker(unsigned short* pnBitfield, LPVOID lpvDataLength, HINTERNET hRequest) {
    if (*(DWORD*)lpvDataLength > 0) {
        if (httplib::g_szDataBuffer == nullptr) {
            httplib::g_szDataBuffer = new char[*(DWORD*)lpvDataLength + 1];
            httplib::g_szDataBuffer[*(DWORD*)lpvDataLength] = 0;

            WinHttpReadData(hRequest, httplib::g_szDataBuffer, *(DWORD*)lpvDataLength, NULL);
        } else {
            char* temp_buffer = new char[strlen(httplib::g_szDataBuffer) + 1];
            temp_buffer[strlen(httplib::g_szDataBuffer)] = 0;
            memcpy(temp_buffer, httplib::g_szDataBuffer, strlen(httplib::g_szDataBuffer)); //-V575 (PVS-Studio FalseAlarm)
            delete[] httplib::g_szDataBuffer;

            httplib::g_szDataBuffer = new char[strlen(temp_buffer) + *(DWORD*)lpvDataLength + 1];
            httplib::g_szDataBuffer[strlen(temp_buffer) + *(DWORD*)lpvDataLength] = 0;
            memcpy(httplib::g_szDataBuffer, temp_buffer, strlen(temp_buffer));

            WinHttpReadData(hRequest, httplib::g_szDataBuffer + strlen(temp_buffer), *(DWORD*)lpvDataLength, NULL);

            delete[] temp_buffer;
        }
    } else *pnBitfield += (~(*pnBitfield << 12) & 0b1000);
}

Requested Edit: get.cpp:

    #include "pch.h"

#include "get.hpp"

#include "callbacks/get_Session/Session.h"

void _appendToHeader(std::string& private_member, const char* szValue, const char* szHeaderName);

namespace httplib {
    char* g_szDataBuffer = nullptr;

    GetRequest::GetRequest(FILE* temp_pSTDOUT) {
        *stdout = *temp_pSTDOUT;
        m_szData = &g_szDataBuffer;
    }

    GetRequest::~GetRequest() {
        delete[] *m_szData;
    }

    // XXXXXXXXXXXX0000 = X (12bit): Reserved for HTTP RETURN STATUS CODE
    // XXXXXXXXXXXX0001 = request succsessfully finished + valid http return status code available
    // XXXXXXXXXXXX0010 = return SendRequest() function with 0 (Ivalid URI)
    // XXXXXXXXXXXX0100 = return SendRequest() function with -1 (SSL Certificate error)
    // XXXXXXXXXXXX1000 = is the request finished / should the function return
    USHORT g_nBitFlags = 0b0000'0000'0000'0000;

    RESPONSE GetRequest::SendRequest(Address szURI, Port nPort) {
        char* pStartEndpoint = (char*)strchr(szURI, L'/');

        HINTERNET hSession = WinHttpOpen(L"", WINHTTP_ACCESS_TYPE_NO_PROXY, WINHTTP_NO_PROXY_NAME, WINHTTP_NO_PROXY_BYPASS,     WINHTTP_FLAG_ASYNC);
        if (hSession) {
            void* phSession_Callback = WinHttpSetStatusCallback(hSession, (WINHTTP_STATUS_CALLBACK)hSession_Callback,   WINHTTP_CALLBACK_FLAG_ALL_NOTIFICATIONS, 0);
            if (phSession_Callback == WINHTTP_INVALID_STATUS_CALLBACK) {
                // TODO: Display the error message in the gui/console of DiscordPP
                printf("TEMPORARY: GetLastError() from GetRequest::SendRequest at WinHttpOpen() async at WinHttpSetStatusCallback   () (Cannot install callback function): %u\n", GetLastError()); // This is temporary. Should not be used.
            }
        } else {
            // TODO: Display the error message in the gui/console of DiscordPP
            printf("TEMPORARY: GetLastError() from GetRequest::SendRequest at WinHttpOpen() async: %u\n", GetLastError()); // This   is temporary. Should not be used.
        }

        wchar_t* szwConnectionUri;

        if (pStartEndpoint != nullptr) {
            szwConnectionUri = new wchar_t[pStartEndpoint - szURI + 1];
            ZeroMemory(szwConnectionUri, (pStartEndpoint - szURI + 1) * sizeof(wchar_t));
            mbstowcs(szwConnectionUri, szURI, pStartEndpoint - szURI);
        } else {
            szwConnectionUri = new wchar_t[strlen(szURI) + 1];
            ZeroMemory(szwConnectionUri, (strlen(szURI) + 1) * sizeof(wchar_t));
            mbstowcs(szwConnectionUri, szURI, strlen(szURI));
        }


        HINTERNET hConnection = WinHttpConnect(hSession, szwConnectionUri, nPort, 0);
        if (!hConnection) {
            // TODO: Display the error message in the gui/console of DiscordPP
            printf("TEMPORARY: GetLastError() from GetRequest::SendRequest at WinHttpConnect() async: %u\n", GetLastError()); //    This is temporary. Should not be used.
            if (GetLastError() == 12005)
                return 0;
        }
        delete[] szwConnectionUri;

        wchar_t* szwEndpointUri;
        if (pStartEndpoint != nullptr) {
            szwEndpointUri = new wchar_t[(strlen(szURI) - (pStartEndpoint - szURI)) + 1];
            ZeroMemory(szwEndpointUri, ((strlen(szURI) - (pStartEndpoint - szURI)) + 1) * sizeof(wchar_t));
            mbstowcs(szwEndpointUri, szURI + (pStartEndpoint - szURI), (strlen(szURI) - (pStartEndpoint - szURI)));
        } else {
            szwEndpointUri = (wchar_t*)L"/";
        }

        const wchar_t* szDataAcceptTypes[] = {L"", 0};
        HINTERNET hRequest = WinHttpOpenRequest(hConnection, L"GET", szwEndpointUri, L"HTTP/2.0", WINHTTP_NO_REFERER,   szDataAcceptTypes, WINHTTP_FLAG_SECURE);
        if (!hRequest) {
            // TODO: Display the error message in the gui/console of DiscordPP
            printf("TEMPORARY: GetLastError() from GetRequest::SendRequest at WinHttpOpenRequest() async: %u\n", GetLastError   ()); // This is temporary. Should not be used.
        }
        if (pStartEndpoint != nullptr) delete[] szwEndpointUri;

        wchar_t* szwHeaders = strcmp(m_szHeadersOut.c_str(), "") ? new wchar_t[m_szHeadersOut.length() + 1] : nullptr;
        if (szwHeaders != nullptr) {
            ZeroMemory(szwHeaders, (m_szHeadersOut.length() + 1) * sizeof(wchar_t));
            mbstowcs(szwHeaders, m_szHeadersOut.c_str(), m_szHeadersOut.length());
        }

        if (strcmp(m_szHeadersOut.c_str(), "") != 0 && szwHeaders != nullptr) {
            if (!WinHttpAddRequestHeaders(hRequest, szwHeaders, wcslen(szwHeaders), WINHTTP_ADDREQ_FLAG_ADD |   WINHTTP_ADDREQ_FLAG_REPLACE))
                // TODO: Display the error message in the gui/console of DiscordPP
                printf("TEMPORARY: GetLastError() from GetRequest::SendRequest at WinHttpOpenRequest() async: %u\n", GetLastError   ()); // This is temporary. Should not be used.
        }

        if (szwHeaders != nullptr) {
            if (!WinHttpSendRequest(hRequest, szwHeaders, wcslen(szwHeaders), WINHTTP_NO_REQUEST_DATA, NULL, 0, 0))
                // TODO: Display the error message in the gui/console of DiscordPP
                printf("TEMPORARY: GetLastError() from GetRequest::SendRequest at WinHttpOpenRequest() async: %u\n", GetLastError   ()); // This is temporary. Should not be used.
        } else {
            if (!WinHttpSendRequest(hRequest, WINHTTP_NO_ADDITIONAL_HEADERS, 0, WINHTTP_NO_REQUEST_DATA, NULL, 0, 0))
                // TODO: Display the error message in the gui/console of DiscordPP
                printf("TEMPORARY: GetLastError() from GetRequest::SendRequest at WinHttpOpenRequest() async: %u\n", GetLastError   ()); // This is temporary. Should not be used.
        }

        DWORD dwOption = WINHTTP_DISABLE_REDIRECTS;
        WinHttpSetOption(hRequest, WINHTTP_OPTION_DISABLE_FEATURE, &dwOption, sizeof(dwOption));

        while (1) {
            std::this_thread::sleep_for(std::chrono::microseconds(1));
            if ((g_nBitFlags >> 3) & 1) {
                WinHttpCloseHandle(hRequest);
                WinHttpCloseHandle(hConnection);
                WinHttpCloseHandle(hSession);
                if ((g_nBitFlags >> 2) & 1) {
                    return -1;
                } else if ((g_nBitFlags >> 1) & 1) {
                    return 0;
                } else if ((g_nBitFlags >> 0) & 1) {
                    return g_nBitFlags >> 4;
                }
            }
        }
    }

    bool GetRequest::AddHeader(HeaderType Header, HeaderValue szValue) {
        switch (Header) {
        case HeaderType::Accept:
            _appendToHeader(m_szHeadersOut, (char*)szValue, "Accept");
            break;
        case HeaderType::Accept_Encoding:
            _appendToHeader(m_szHeadersOut, (char*)szValue, "Accept-Encoding");
            break;
        case HeaderType::Authorization:
            _appendToHeader(m_szHeadersOut, (char*)szValue, "Authorization");
            break;
        case HeaderType::Connection:
            _appendToHeader(m_szHeadersOut, (char*)szValue, "Connection");
            break;
        case HeaderType::Content_Encoding:
            _appendToHeader(m_szHeadersOut, (char*)szValue, "Content-Encoding");
            break;
        case HeaderType::Content_Length:
            _appendToHeader(m_szHeadersOut, (char*)szValue, "Content-Length");
            break;
        case HeaderType::Content_Type:
            _appendToHeader(m_szHeadersOut, (char*)szValue, "Content-Type");
            break;
        case HeaderType::User_Agent:
            _appendToHeader(m_szHeadersOut, (char*)szValue, "User-Agent");
            break;
        case HeaderType::Upgrade:
            _appendToHeader(m_szHeadersOut, (char*)szValue, "Upgrade");
            break;
        case HeaderType::Referer:
            _appendToHeader(m_szHeadersOut, (char*)szValue, "Referer");
            break;
        default:
            return false;
        }

        return true;
    }

    const char* GetRequest::GetData() {
        return *m_szData;
    }
}

void _appendToHeader(std::string &private_member, const char* szValue, const char* szHeaderName) {
    private_member += szHeaderName;
    private_member += ":";
    private_member += szValue;
    private_member += "\r\n";
}

Requested Edit: callback.cpp:

    #include "pch.h"

#include "Session.h"

#include "workers/dataAvail/dataAvail.h"
#include "workers/headersAvail/headersAvail.h"

namespace httplib {
    extern USHORT g_nBitFlags;
    extern char* g_szDataBuffer;
}

void WINAPI hSession_Callback(
    IN HINTERNET hInternet,
    IN DWORD_PTR dwContext,
    IN DWORD dwInternetStatus,
    IN LPVOID lpvStatusInformation,
    IN DWORD dwStatusInformationLength
) {
    switch (dwInternetStatus) {
    case WINHTTP_CALLBACK_STATUS_REQUEST_SENT:
        WinHttpReceiveResponse(hInternet, 0);
        break;
    case WINHTTP_CALLBACK_STATUS_HEADERS_AVAILABLE: {
        std::thread(headersAvail_worker, &httplib::g_nBitFlags, hInternet).detach();
        WinHttpQueryDataAvailable(hInternet, NULL);
    }
        break;
    case WINHTTP_CALLBACK_STATUS_DATA_AVAILABLE: {
        std::thread(dataAvail_worker, &httplib::g_nBitFlags, lpvStatusInformation, hInternet).detach();
    }
        break;
    case WINHTTP_CALLBACK_STATUS_SECURE_FAILURE:
        httplib::g_nBitFlags = 0;
        httplib::g_nBitFlags += (~(httplib::g_nBitFlags << 13) & 0b0100);
        httplib::g_nBitFlags += (~(httplib::g_nBitFlags << 12) & 0b1000);
        break;
    case WINHTTP_CALLBACK_STATUS_REQUEST_ERROR:
        httplib::g_nBitFlags = 0;
        httplib::g_nBitFlags += (~(httplib::g_nBitFlags << 14) & 0b0010);
        httplib::g_nBitFlags += (~(httplib::g_nBitFlags << 12) & 0b1000);
        break;
    case WINHTTP_CALLBACK_STATUS_READ_COMPLETE:
        WinHttpQueryDataAvailable(hInternet, NULL);
        break;
    case WINHTTP_CALLBACK_STATUS_REDIRECT:
        printf("Session handler: Redirection was attempted, and canceled. Redirect Destination: %s\n", (char*)  lpvStatusInformation);
        break;
    }
}

Requested Edit: main.cpp:

    #include "pch.h"

#include "../../httplib/src/get.hpp"
#include "../../httplib/src/post.hpp"

#ifdef _DEBUG
#pragma comment(lib, "../httplib/bin/Debug_x86/httplib_Debug_x86.lib")
#else
#pragma comment(lib, "../httplib/bin/Release_x86/httplib_Release_x86.lib")
#endif

int main() {
    //ToDo: Switch to WINDOWS sub, make gui with console to the side.

    httplib::GetRequest get(stdout);
    get.AddHeader(HeaderType::Authorization, "token");
    int nGet = get.SendRequest("discord.com/api/channels/678958544606724115/messages?limit=1");

    printf("HTTP/GET Method: %d\n\nData: %s\n\n\n\n", nGet, get.GetData());
}

So my question is why if that thread sleep in the end of the GET function is missing then its in a loop forever, I used many tools to debug it and see how it does that, but my only valid conclusion is that its in a never ending loop because that while statement is consuming all the resources and thus blocking further progression (Although cpu usage in that loop is not 100%, its only about 9% on a Ryzen 3600x)

Edit: To clarify this is working in Debug mode without the thread_sleep, only in Relase mode it is doing this behaviour.

Your code is lacking a lot of details. Please provide a [minimal reproducible example](https://stackoverflow.com/help/minimal-reproducible-example). — mpoeter, May 23 '20 at 17:39
I know its short but it has everything you need to know. Everything else has nothing to do with this code. But i will add it if that helps you. — 0000, May 23 '20 at 17:48

mpoeter · Answer 1 · 2020-05-24T20:41:39.440

0

You have a data race on g_nBitFlags and therefore undefined behavior. One thread is spinning on g_nBitFlags, waiting for another thread to update it, but you are not using an atomic variable or other synchronization mechanisms. Thus the compiler has the right to assume that the variable cannot change. So in release mode, the compiler simply decides to optimize away a reload of g_nBitFlags, and your code keeps looping on the same (constant) value.

Simple solution: make g_nBitFlags an atomic.

edited May 24 '20 at 20:41

answered May 23 '20 at 21:39

mpoeter

2,574
1
5
12

Ah, that makes sense, but how would you explain that when I add the delay on that thread its working without atomic ? If the compiler indeed is optimizing it by making it constant that wouldn't work even with that sleep, but it does, so i assume its not 100% correct what you say. – 0000 May 24 '20 at 16:21
Also would'nt the `volatile` keyword solve this issue? Edit: Tested it, and it doesnt fix the issue – 0000 May 24 '20 at 16:39
Because the sleep probably acts as barrier in this case, but it is still a data race. I suspect the compiler to optimize away the reload (I have seen several cases like this before), but to be able to say with certainty what is happening, I would have to take a look at the generated ASM code. – mpoeter May 24 '20 at 17:00
Regarding `volatile` - no `volatile` will _not_ fix the data race! `volatile` was never designed or intended to be used in multithreaded context. You should define `g_nBitFlags` to be a `std::atomic`. – mpoeter May 24 '20 at 17:04
Read my answer, i figured it out :D – 0000 May 24 '20 at 18:52

0000 · Answer 2 · 2020-05-24T19:02:38.920

-1

So after an assembly analisys, the issue is that while in Release mode the compiler uses this code to check the flag variable (g_nBitFlags_Get):

005B1500  test        al,8  
005B1502  je          httplib::GetRequest::SendRequest+350h (05B1500h)

Which I think (This might not be correct) is assuming that the flag variable will be in EAX (But because the flag variable's value is set in a different thread, and thus the EAX register is different value for each thread (As mpoeter said: data racing). I guess the compiler things this is an optimized way of doing it but it doesnt realize that that piece of data is modified externally

And here is the debug assembly:

; This is the ( while (1) )
009F1595  mov         eax,1  
009F159A  test        eax,eax  
009F159C  je          httplib::GetRequest::SendRequest+651h (09F1631h) ; never jumps
; Here begins the check ( (g_nBitFlags_Get >> 3) & 1) )
009F15A2  movzx       eax,word ptr [httplib::g_nBitFlags_Get (09FB2F4h)]  
009F15A9  sar         eax,3  
009F15AC  and         eax,1  ; This here is the important check
009F15AF  je          httplib::GetRequest::SendRequest+64Ch (09F162Ch)  ; if the flags variable is in a correct state it doesnt jump back to the start (if ZF=0 goes in the if statement)

Edit: The compiler is very confusingly compiling :D, I added asm code instead of the thread pause and it changed the code to actually work properly:

00CD1500  mov         edx,539h ; My asm
00CD1505  test        byte ptr [httplib::g_nBitFlags_Get (0CD53FCh)],8 ; ACTUAL working check :D
00CD150C  je          httplib::GetRequest::SendRequest+350h (0CD1500h)

Edit 2: Tested with NOP as my asm, it works perfectly!

edited May 24 '20 at 19:02

answered May 24 '20 at 18:51

0000

1
1

Yes, `g_nBitFlags` is loaded into EAX but never reloaded, so the loop keeps checking the same value over and over again - that is exactly what I suspected. However, this is a _result_ of the data race, not the data race itself! A [data race](https://en.cppreference.com/w/cpp/language/memory_model#Threads_and_data_races) occurs when you have conflicting accesses on a non-atomic variable without a proper happens-before order - and that is exactly what you have with `g_nBitFlags`. – mpoeter May 25 '20 at 07:42
Adding some asm code to avoid the compiler optimization is _the worst possible "solution" I can think of_! Most importantly, it may prevent the compiler from optimizing away the reload (in this particular case!), but it does _not_ fix the data race, so you still have undefined behavior and therefore your code can behave completely unpredictable at any time. To actually _fix_ this, you should make `g_nBitFlags` and atomic (as I explained several times already) or use other synchronization mechanisms like a condition_variable. – mpoeter May 25 '20 at 07:46
You are wrong, this is NOT undefined behaviour because only one thread of the 2 using this variable are actually modifying the value the other one is just testing for the right value. If more than 1 thread would modify it asynchronously then i agree, but this is not a problem at all, and from all the testing 100% of the time it works just like a charm. – 0000 May 25 '20 at 13:47
"one thread of the 2 using this variable are actually modifying the value the other one is just testing for the right value." Yes, and if you would actually bother to read the link I posted you would see that this the exact definition of conflicting operations (the wording in the standard is almost exactly the same): "When an evaluation of an expression writes to a memory location and another evaluation _reads_ or modifies the same memory location, the expressions are said to conflict." – mpoeter May 25 '20 at 14:50

Visual Studio release mode blocks code execution that is executed in debug mode. Using WinHTTP and Multi-Threading

2 Answers2