Can malloc cause a crash when detouring a function in C++ to copy a data string from ExtTextOut in a target process?

Question

I am trying to make a detour using MSDN Detours 3.0 to register text drawn by ExtTextOut() from a third party software. I have created a DLL which I inject in the target software. When I detour ExtTextOut I try to copy the string sent to the method and read the text to a textfile in modified format. Since the input string is UTF-16 and I am only interested in keeping the ASCII characters lower than 127 I have made some logic for this.

The problem is, though, that my program crashes a while after it is injected in the target. I suspect that it might have something to do with the malloc function.

If I use malloc to locate memory in a process, can I guaratee that this will not override any memory in the target process? If that would be the case, how can I create a function to make sure that my malloc wont interfere with the target process.

The code:

BOOL WINAPI Mine_ExtTextOut(HDC hdc, int X, int Y, UINT options, RECT* lprc, LPCWSTR text, UINT cbCount, INT* lpSpacingValues)
{

    // 
    if(reinterpret_cast<const char*>(text)[0] == '>'){ 

        //wstring_convert<codecvt_utf8_utf16<wchar_t>,wchar_t> convert;
        //string utf8_string = convert.to_bytes(text);
        //int n = utf8_string.length();

        int n = cbCount;

        char *buffer = (char*) malloc (n+1);
        char *bufferCopy = (char*) malloc (n+1);

        for (int i=0; i<n; i++){
            if((long) text[i] > 127){
                buffer[i] = ' ';
                continue;
            }
            buffer[i]= (char) text[i];
        }
        buffer[n]='\0';



        bool wasBlank = false;
        int ix = 0;
        for(int i = 0; i<n; ++i){
            if(buffer[i] == ' '){
                if(wasBlank || i < 2) continue;
                bufferCopy[ix++] = buffer[i];
                wasBlank = true;
                continue;
            }
            wasBlank = false;
            if(buffer[i] == '>') continue;
            bufferCopy[ix++] = buffer[i];
        }
        bufferCopy[ix]='\0';

        ofstream myFile;
        myFile.open("C:\\temp\\textHooking\\textHook\\example2.txt", ios::app);
        if(buffer[0] == '>'){
            //myFile.write(reinterpret_cast<const char*>(text), cbCount*sizeof(*text));
            myFile.write(bufferCopy, ix*sizeof(*bufferCopy));
            myFile << endl;
        }

        free(buffer);
        free(bufferCopy);

    }
    BOOL rv = Real_ExtTextOut(hdc, X, Y, options, lprc, text, cbCount, lpSpacingValues);
    return rv;
}

Answer 1

The cbCount parameter of ExtTextOut() is expressed in characters, but the input parameter of malloc() is expressed in bytes. You are hooking the Unicode version of ExtTextOut() (aka ExtTextOutW()), where sizeof(WCHAR) is 2 bytes. You are trying to treat the input string as Ansi, which it is not, and you are not taking UTF-16 surrogates into account.

To do what you are attempting, you need to actually decode the UTF-16 data into Unicode codepoints first before deciding which ones to keep, eg:

BOOL WINAPI Mine_ExtTextOut(HDC hdc, int X, int Y, UINT options, RECT* lprc, LPCWSTR text, UINT cbCount, INT* lpSpacingValues)
{
    if ((cbCount > 0) && (text != NULL) && (text[0] == L'>'))
    { 
        // worse case, every UTF-16 character is ASCII and will be kept,
        // so allocate enough memory for at least that many characters
        std::string buffer(cbCount);
        std::string bufferCopy(cbCount);

        int ix1 = 0;
        for (UINT i = 0; i < cbCount;)
        {
            ULONG c;

            // is it a UTF-16 high surrogate?
            if ((text[i] >= 0xD800) && (text[i] <= 0xDBFF))
            {
                // is it at the end of the string?
                if ((i+1) == cbCount)
                {
                    // malformed surrogate
                    break;
                }

                // is it followed by a UTF-16 low surrogate?
                if ((text[i+1] < 0xDC00) || (text[i+1] > 0xDFFF))
                {
                    // malformed surrogate
                    break;
                }

                // decode the surrogate and skip past it
                c = ((ULONG(text[i] - 0xD800) << 10) | ULONG(text[i+1] - 0xDC00)) + 0x10000;
                i += 2;
            }

            // is it a UTF-16 low surrogate?
            else if (text[i] >= 0xDC00) && (text[i] <= 0xDFFF))
            {
                // malformed surrogate
                break;
            }

            // must be a non-surrogated character
            else
            {
                c = (ULONG) text[i];
                ++i;
            }

            // keep it?
            if( c > 127 )
                buffer[ix1] = ' ';
            else
                buffer[ix1] = (char) c;

            ++ix1;
        }

        bool wasBlank = false;
        int ix2 = 0;
        for(int i = 0; i < ix1; ++i)
        {
            if (buffer[i] == ' ')
            {
                if (wasBlank || (i < 2)) continue;
                bufferCopy[ix2++] = buffer[i];
                wasBlank = true;
                continue;
            }
            wasBlank = false;
            if (buffer[i] == '>') continue;
            bufferCopy[ix2++] = buffer[i];
        }

        ofstream myFile;
        myFile.open("C:\\temp\\textHooking\\textHook\\example2.txt", ios::app);
        if (myFile)
        {
            myFile.write(bufferCopy.c_str(), ix2);
            myFile << endl;
        }
    }

    return Real_ExtTextOut(hdc, X, Y, options, lprc, text, cbCount, lpSpacingValues);
}

Answer 2

Based on the comments here are a few things you could do to try to narrow this down:

Replace malloc: Instead use buffer= (char *)GlobalAlloc(GPTR, n+1); and GlobalFree(buffer). This would make sure it's not a problem with having multiple c-runtime libraries.

Alternately, to make sure there are no situations where you run over the bounds of the allocated buffers, try to malloc a lot more, e.g. buffer= malloc(n+1024); and see if that fixes the problem. If that does, you'll need to comb through your code if you go beyond the valid offsets in the buffers.

A similar approach (for testing) would be to use stack versions of the buffer that are longer than the input you expect, e.g. char buffer[1024]; and use them like the malloc buffers (of course don't free them) and see what difference this makes.