C++: Trouble with Pointers, loop variables, and structs

Question

Consider the following example:

#include <iostream>
#include <sstream>
#include <vector>
#include <wchar.h>
#include <stdlib.h>

using namespace std;

struct odp {
    int f;
    wchar_t* pstr;
};

int main()
{
    vector<odp> vec;
    ostringstream ss;

    wchar_t base[5]; 
    wcscpy_s(base, L"1234");

    for (int i = 0; i < 4; i++)
    {
        odp foo;
        foo.f = i;

        wchar_t loopStr[1];
        foo.pstr = loopStr; // wchar_t* = wchar_t ? Why does this work?
        foo.pstr[0] = base[i];

        vec.push_back(foo);
    }

    for (vector<odp>::iterator iter = vec.begin(); iter != vec.end(); iter++)
    {
        cout << "Vec contains: " << iter->f << ", " << *(iter->pstr) << endl;
    }
}

This produces:

Vec contains: 0, 52
Vec contains: 1, 52
Vec contains: 2, 52
Vec contains: 3, 52

I would hope that each time, iter->f and iter->pstr would yield a different result. Unfortunately, iter->pstr is always the same.

My suspicion is that each time through the loop, a new loopStr is created. Instead of copying it into the struct, I'm only copying a pointer. The location that the pointer writes to is getting overwritten.

How can I avoid this? Is it possible to solve this problem without allocating memory on the heap?

Answer 1

It looks like pstr is pointing to the locally scoped variable loopStr, so the results are undefined (it appears to still have the last stored value in it when printing the results). If you printed the address of pstr in the loop, I believe it would be the same for each position in the loop.

Answer 2

What you've got here is undefined behavior. Each time through your loop you create and destroy an array and then assign its address to foo.pstr and push it back into your vector. The compiler just happens to create that array in the same place every time (which is logical but not necessary). When you print it out you're technically printing out deleted data it's just that the system isn't bitch slapping you for it because it's not protected space. The memory location simply has whatever it was assigned last.

You fix this by discontinuing the use of raw character pointers and arrays.

Answer 3

    ...
    odp foo;
    foo.f = i;

    wchar_t loopStr[1];    //A
    foo.pstr = loopStr;    //B
    foo.pstr[0] = base[i]; //C

    vec.push_back(foo);
    ...

A - You allocate an array (of size 1) to the stack

B - You assign foo.pstr to point to the array on the stack

C - You assign base[i] to the first element of the array (which is on the stack)

After the for loop exits its current cycle the variable loopStr is no longer in scope and its content is undefined. The next loop iteration will most likley re-use the same memory address (hence why you get the same value when you print at the end). If you have optimisations turned on your C compiler may be able to warn you about taking addresses of local variables (although I doubt it).

Without using any heap allocation I would think your only option is to fix the size of foo.pstr in odp, i.e.

 struct odp {
     int f;
     wchar_t pstr[1];
 };

or allocate the array on the heap as part of the odp initialisation

    ...
    odp foo;
    foo.f = i;

    foo.pstr = new wchar_t [1];
    foo.pstr[0] = base[i];

    vec.push_back(foo);
    ...

better still use std::wstring since you are using c++, and let it do the memory allocation and management for you.

Answer 4

foo.pstr = loopStr; // wchar_t* = wchar_t ? Why does this work?

This doesn't work, at least not in the way you want it to. loopStr is an array, but you can use it like a pointer too. So when you assign to the pointer foo.pstr, it gets the address of the first element in loopStr. That happens to be a local variable allocated on the stack and is only valid inside the for loop.