Concatenating a stack string with a heap string gives odd results

Question

I am sure the following has a rational explanation but I am nevertheless a bit baffled.

The issue is with a function which creates a _TCHAR[CONSTANT], a _TCHAR*, concatenates them and returns the result.

For some reason the call to whatTheHeck() from _tmain() returns gibberish.

_TCHAR* whatTheHeck(_TCHAR* name) {
    _TCHAR Buffer[BUFSIZE];
    DWORD dwRet;
    dwRet = GetCurrentDirectory(BUFSIZE, Buffer);
    _TCHAR* what = new _TCHAR[BUFSIZE];
    what = _tcscat(Buffer, TEXT("\\"));
    what = _tcscat(what, name);
    return what;
}

int _tmain(int argc, _TCHAR* argv[]) {

    _TCHAR* failure = whatTheHeck(TEXT("gibberish);")); // not again ..
    _tprintf(TEXT("|--> %s\n"), failure);

    _TCHAR* success = createFileName(TEXT("readme.txt")); // much better
    _tprintf(TEXT("|--> %s\n"), success);

    return 0;
}

In contrast, when going with heap things work as expected.

_TCHAR* createFileName(_TCHAR* name) {
    _TCHAR* Buffer = new _TCHAR[BUFSIZE];
    DWORD dwRet;
    dwRet = GetCurrentDirectory(BUFSIZE, Buffer);
    Buffer = _tcscat(Buffer, TEXT("\\"));
    Buffer = _tcscat(Buffer, name);
    return Buffer;
}

Why the difference?

Is it because _tcscat() concatenates memory addresses instead of their contents and return purges the stack?

Kerrek SB · Accepted Answer · 2011-07-29T10:56:24.877

There are lots of problems with your code. Let's take it apart, shall we:

_TCHAR* whatTheHeck(_TCHAR* name)   // We're inside a local scope
{
    _TCHAR Buffer[BUFSIZE];         // "Buffer" has automatic storage

    _TCHAR* what = new _TCHAR[BUFSIZE];  // "what" points to newly allocated dyn. memory

    what = _tcscat(Buffer, TEXT("\\"));  // Oh no, we overwrite "what" - leak!
                                         // Now what == Buffer.

    what = _tcscat(what, name);  // Equivalent to "_tcscat(Buffer, name)"

    return Buffer;               // WTPF? We're returning a local automatic!
 }

As you can see, you are both causing a memory leak with a gratuitious and reckless new, and you are also returning the address of a local object past its lifetime!

I would strongly recommmend

reading the documentation for strcat and understanding "source" and "destination",
not using strcat, but a safer version like strncat,
not using strncat, but instead std::string.

R. Martinho Fernandes · Answer 2 · 2011-07-29T11:02:58.397

That happens because _tcscat concatenates into the destination parameter, which is the first one, and then returns it. So, it returns a pointer to the array Buffer and it gets stored in what in this line:

what = _tcscat(Buffer, TEXT("\\"));

Then this pointer is returned, and you have undefined behavior once you try to use it, because the local Buffer no longer exists.

Furthermore, the line above also causes the memory allocated for what to be leaked:

_TCHAR* what = new _TCHAR[BUFSIZE];
what = _tcscat(Buffer, TEXT("\\")); // this loses the memory allocated
                                    // in the previous line

visitor · Answer 3 · 2011-07-29T10:55:20.500

0

The dynamically allocated memory what points to is not initialized. It contains gibberish. _tcscat expects the string to be properly null-terminated.

_TCHAR* what = new _TCHAR[BUFSIZE]();

This fills what with '\0' characters.

_TCHAR* what = new _TCHAR[BUFSIZE];
what[0] = '\0';

This properly terminates the empty string.

GetCurrentDirectory doesn't expect the buffer to be null-terminated. It writes something to that and properly null-terminates it. You can then pass that to concatenation functions.

As a side note, your function seems to be vulnerable to buffer overflows. Apparently you allow GetCurrentDirectory to fill up everything you allocated, and then you want to append to it without caring if there is any more space left.

edited Jul 29 '11 at 10:55

answered Jul 29 '11 at 10:42

visitor

1,781
10
7

The allocated memory `what` points to is leaked and not the cause of the gibberish. – R. Martinho Fernandes Jul 29 '11 at 10:46
I assumed `_tcscat` works like `strcat` in which case assignments of the result should be ok, though redundant (the destination pointer is not modified anyway). – visitor Jul 29 '11 at 10:52
@Martinho: I missed that OP's passing `Buffer` the first time. Can't imagine someone managing C strings without any idea how the C string functions work... – visitor Jul 29 '11 at 10:59

score 0 · Answer 4 · answered Jul 29 '11 at 10:50

_TCHAR* what = new _TCHAR[BUFSIZE];
what = _tcscat(Buffer, TEXT("\\"));

Aren't you overwriting what with the Buffer which is a local variable to the function. Once the stack is unwound, Buffer goes out of scope and thus you get unexpected values. Also it's a memory leak.

In heap allocation scenarios, you may prefer to declare the pointer a const to avoid such hazards:

_TCHAR* const what = new _TCHAR[BUFSIZE];
        ^^^^^ (avoids overwriting)

The better approach is to use std::string and be out of such small issues.

Concatenating a stack string with a heap string gives odd results

4 Answers4