Problem with wininet C++

Question

Can anyone find problem in this function? My application makes several requests and if first request is using SSL, app crashes on some computers(on mine 4 computers + vmware it works ok without crash).

Here is code

char Buffer[1024];
DWORD dwRead;
string data;

string Request(string method, string host, string file, string headers,
               string post, bool debug, bool SSL)
{
    HINTERNET hSession, hDownload, hRequest;
    DWORD flag;
    DWORD port;

    data.empty();

    //SSL or not + flag :)
    if (SSL)
    {
        port = INTERNET_DEFAULT_HTTPS_PORT;
        flag = INTERNET_FLAG_SECURE; // FLAG_SECURE
    }
    else
    {
        port = INTERNET_DEFAULT_HTTP_PORT;
        flag = INTERNET_FLAG_RELOAD; //FLAG_RELOAD
    }

    char * postdata;
    postdata = new char[post.size() + 1];
    strcpy(postdata, post.c_str());
    char * headersdata;
    headersdata = new char[headers.size() + 1];
    strcpy(headersdata, headers.c_str());

    //Actual request
    hSession
            = InternetOpen(
                    "Mozilla/5.0 (Windows; U; Windows NT 6.1; sl; rv:1.9.2.11) Gecko/20101012 Firefox/3.6.11",
                    INTERNET_OPEN_TYPE_DIRECT, NULL, NULL, 0);
    if (hSession)
    {
        hDownload = InternetConnect(hSession, host.c_str(), port, NULL, NULL,
                INTERNET_SERVICE_HTTP, 0, 0);
        if (hDownload)
        {
            hRequest = HttpOpenRequest(hDownload, method.c_str(), file.c_str(),
                    "HTTP/1.1", NULL, NULL, flag, 0);
            if (hRequest)
            {
                if (strlen(headersdata) && strlen(postdata))
                {
                    HttpSendRequest(hRequest, headersdata, strlen(headersdata),
                            postdata, strlen(postdata));
                }
                else
                {
                    HttpSendRequest(hRequest, NULL, 0, NULL, 0);
                }
            }
        }
    }
    //Writing HTML response in data buffer
    while (InternetReadFile(hRequest, Buffer, sizeof(Buffer), &dwRead))
    {
        if (dwRead == 0)
        {
            break;
        }
        Buffer[dwRead] = 0;
        data += Buffer;
    }

    //Debug :)
    if (debug)
    {
        ofstream dbgfile;
        dbgfile.open("debug.html");
        dbgfile << data;
        dbgfile.close();
    }

    //Close handles
    InternetCloseHandle(hSession);
    InternetCloseHandle(hDownload);
    InternetCloseHandle(hRequest);

    return data;
}

Thanks.

Answer 1

First of all, you have a buffer overflow with Buffer.

Consider these lines:

while (InternetReadFile(hRequest, Buffer, sizeof(Buffer), &dwRead))

and

Buffer[dwRead] = 0;

Since you pass sizeof(Buffer) as dwNumberOfBytesToRead parameter in the former line, maximum value of dwRead is sizeof(Buffer). If such a case occurs, the latter line will write one byte past the end of Buffer. Your data layout makes it unlikely to cause a crash (but it's pure chance!), unless you have Runtime Security Checks enabled, which can explain the crash message.

Also, as far as I can remember, the "This application has requested the Runtime to terminate it in an unusual way" message is displayed either by assert() or terminate() in Microsoft implementation. (I don't have MSVC available at the moment and can't verify it). I don't see the reason for either one in this piece of code, so if it's not the Buffer overflow, look for it in other places too.

Answer 2

Try to remove strlen:

HttpSendRequest(hRequest, &headers.front(), headers.size(),
                 &post.front(), post.size());

In this case the function will be a little more secure.

Anyway, consider using Crash Dump Analysis. In this case you will able to check the callstack from the crash dump gotten from 'some computers'.