CODEWITHSUNDEEP
cvisual-studio-2010exceptionmfcstruct
rbhkamal
rbhkamal

Reputation: 155

Can structs be cast to a different size in the following way?

OS: Windows x86, MFC, CRT, VS2010

I'm running into a run-time CRT heap corruption exception after upgrading from Visual Studio 2005 (SP1) to VS2010-SP1.

The CRT only complains about the corruption when my app is compiled with debug. And I believe that VS2010 is just too strict when it is evaluating memory accesses.

The suspected offending code does the following:

typedef struct _T_TEST
{
    DWORD flag;
    TCHAR str[1];
} T_TEST;

// Then the structure is used in this way
void test(TCHAR * p_str) {
    DWORD size = sizeof(DWORD) + (_tcslen(p_str) + 1) * sizeof(TCHAR);
    T_TEST * foo = (T_TEST *) calloc(size, 1);
    foo->flag = 0;
    _tcscpy_s(foo->str, size - sizeof(DWORD), p_str);
    // Do IPC with the struct
    // When finished, free it
    free(foo);
}

This code sometimes fails on free(foo) and other times when casting the allocated buffer to (T_TEST *).

Is extending the struct size in that way valid? And is there something different in VS2010 that treats that as a heap corruption?

Any help will be greatly appreciated!

Upvotes: 0

Views: 257

Answers (2)

IInspectable
IInspectable

Reputation: 51365

There are 2 issues with the code that could cause heap corruption: Passing the size in bytes instead of the character count to _tcscpy_s and calculating the wrong buffer size.

A correct implementation should look roughly like this:

void test(TCHAR * p_str) {
    // Calculate the input string length
    size_t len = _tcslen(p_str);

    // Calculate the buffer size  (1)
    size_t bufferSize = offsetof(T_TEST, str[len + 1]);

    // Allocate a buffer
    vector<char> buffer(bufferSize);

    // Map the structure onto the buffer to get a T_TEST*
    T_TEST* foo = reinterpret_cast<T_TEST*>(&buffer[0]);

    foo->flag = 0;
    // This potentially writes beyond the end  (2)
    //_tcscpy_s(foo->str, size - sizeof(DWORD), p_str);
    _tcscpy(foo->str, len + 1, p_str);

    // Do IPC with the struct
    // When finished, free it

    // buffer will be automatically cleaned up
    //free(foo);
}

(1) The size of a structure is not necessarily the same as the sum of the sizes of its members. Depending on the target platform and/or compiler settings the compiler will align the individual members to meet the alignment requirements. To do so it (potentially) adds padding between them. Using offsetof is a convenient way to have the compiler do the calculations.

(2) In the original code there are 2 issues: The call to _tcscpy_s requires the size of the destination buffer in number of characters, not the size in bytes. There is also a hidden gotcha: The expression foo->str instructs the compiler to calculate the offset, taking potential padding into account. The expression size - sizeof(DWORD) however does not account for padding, which opens the possibility to write beyond the end of the allocated buffer.

Since you are dealing with an MFC application I assume that using C++ is ok, even though the question is not tagged C++. If you cannot or do not want to use std::vector you can allocate the buffer using new char[bufferSize] and delete[] buffer; to clean up.

Upvotes: 0

Mark Ransom
Mark Ransom

Reputation: 308140

The second parameter to _tcscpy_s is the number of elements but you're feeding it the number of bytes. According to the documentation:

The debug versions of these functions first fill the buffer with 0xFE.

This will result in a buffer overflow if sizeof(TCHAR) != 1.

Upvotes: 2

Related Questions