C++ ostringstream strange behavior

Question

I had a very strange problem with c++ code recently. I have reproduced the case in minimalist example. We have an Egg class:

class Egg
{
private:
    const char* name;
public:
    Egg() {};
    Egg(const char* name) {
        this->name=name;
    }
    const char* getName() {
        return name;
    }
};

We also have a Basket class to hold the Eggs

const int size = 15;
class Basket
{
private:
    int currentSize=0;
    Egg* eggs;
public:
    Basket(){
        eggs=new Egg[size];
    }
    void addEgg(Egg e){
        eggs[currentSize]=e;
        currentSize++;
    }
    void printEggs(){
        for(int i=0; i<currentSize; i++)
        {
            cout<<eggs[i].getName()<<endl;
        }
    }
    ~Basket(){
        delete[] eggs;
    }
};

So here is example that works as expected.

 Basket basket;
 Egg egg1("Egg1");
 Egg egg2("Egg2");

 basket.addEgg(egg1);
 basket.addEgg(egg2);
 basket.printEggs();
 //Output: Egg1 Egg2

This is the expected result, but if I want to add N eggs with generated names depending on some loop variable, I have the following problem.

 Basket basket;
 for(int i = 0; i<2; i++) {
    ostringstream os;
    os<<"Egg"<<i;
    Egg egg(os.str().c_str());
    basket.addEgg(egg);
 }
 basket.printEggs();
 //Output: Egg1 Egg1

If I change the loop condition to i<5, I get "Egg4 Egg4 Egg4 Egg4 Egg4". It saves the last added Egg in all the indexes of the dynamic Egg array.

After some search in google I found out that giving the char* name variable in Egg a fixed size and using strcpy in the constructor fixes the issue.

Here is the "fixed" Egg class.

class Egg
{
private:
     char name[50];
public:
    Egg(){};
    Egg(const char* name)
    {
        strcpy(this->name, name);
    }
    const char* getName()
    {
        return name;
    }
};

Now the question is why?

Thanks in advance.

Here is a link to the whole code.

Since you're using C++ it's a much better plan to use `std::string` than the old C string functions. — tadman, Apr 20 '16 at 09:12
Yes, I am not that good with c++. Just wanted to create example for the case — Borislav Stoilov, Apr 20 '16 at 09:14
I agree, use `std::string` and `std::vector` + write your own copy constructor, as I guess you are experiencing undefined behavior — JVApen, Apr 20 '16 at 09:19
`std::string` is much, much more forgiving than C-style strings, so if you can, use those unless you have a very compelling reason not to. Bugs like this are all too common when you're manipulating raw character pointers. — tadman, Apr 20 '16 at 09:19
You have a mess with pointers and violation of the [rule of three](http://stackoverflow.com/q/4172722/3344612). You should get some basic knowledge of how to work with pointers and arrays in C++ — Teivaz, Apr 20 '16 at 09:20
Let me just note, that this is not a Question about how to do it, but why I am observing this. — Borislav Stoilov, Apr 20 '16 at 09:40

score 5 · Accepted Answer · answered Apr 20 '16 at 09:25

5

Lets take a closer look at this expression: os.str().c_str().

The function str returns a string by value, and using it this way make the returned string a temporary object whose lifetime is only until the end of the expression. Once the expression ends, the string object is destructed and exists no more.

The pointer you pass to the constructor is a pointer to the internal string of the temporary string object. Once the string object is destructed that pointer is no longer valid, and using it will lead to undefined behavior.

The simple solution is of course to use std::string whenever you want to use a string. The more complicated solution is to use an array and copy the contents of the string, before it disappears (like you do in the "fixed" Egg class). But be aware of that the "fixed" solution using fixed-sized arrays is prone to buffer overflows.

answered Apr 20 '16 at 09:25

Some programmer dude

400,186
35
402
621

Thanks, this cleared things a lot, so It turns out 'ostringstream' is using the same place in memory each time its created? Since its re-created in each iteration. And another thing, you say we get undefined behavior, but why since I copy the pointer in the array right? The variable in the loop is destroyed but a copy is stored in the egg array ( which turns out its not copy but the same pointer). – Borislav Stoilov Apr 20 '16 at 09:38
1

@BorislavStoilov Problem is that your variable is a pointer. Pointer does nothing, but points to **another** variable. And **that** variable is destroyed. Any attempt to access it through that pointer triggers UB – Revolver_Ocelot Apr 20 '16 at 09:41
1

@BorislavStoilov The compiler needs to allocate space for the `ostringstream` object each and every iteration of the loop, but why waste precious cycles with that when it can just reuse the space each iteration? So yes it just reuses the same space every iteration, and just calls the constructor/destructor. – Some programmer dude Apr 20 '16 at 09:41
@BorislavStoilov As for the UB, you copy the *pointer* and not the contents (in the original "non-fixed" code), that's the problem. A pointer is just what it sounds like, a pointer to some memory, and if that memory cease to exist the pointer is no longer valid. It doesn't matter how many copies of the pointer you have, all of them are pointing to the same memory. – Some programmer dude Apr 20 '16 at 09:43

gsamaras · Answer 2 · 2016-04-20T09:26:37.653

2

In your first case, you copy the pointer, which points to the string.

In the second case, with strcpy(), you actually deep-copy the string.

OK, I wasn't verbose, let me clarify. In the first case, you copy the pointer, which points to a string created with ostringstream. What happens when that goes out of scope?

Undefined behavior!

edited Apr 20 '16 at 09:26

answered Apr 20 '16 at 09:18

gsamaras

71,951
46
188
305

But isn't the pointer totally different? In the loop 'Egg egg(os.str().c_str());', each time a new Egg is created? – Borislav Stoilov Apr 20 '16 at 09:23
@BorislavStoilov I updated, sorry. Yes it is, but since you experience UB we can't tell for sure what the behavior is. I would guess that the function reuses its memory, thus the latest copy remains alive. Good question BTW, +1. – gsamaras Apr 20 '16 at 09:34

Bathsheba · Answer 3 · 2016-04-20T09:33:27.953

os.str() is an anonymous temporary of type std::string, and the behaviour on accessing the memory pointed to by .c_str(), once that anonymous temporary goes out of scope (which is does at the end of the statement), is undefined. Your second case works since strcpy(this->name, name); is taking a copy of the data pointed to by .c_str() before the temporary goes out of scope. But the code is still fragile: the fixed size character buffer is vulnerable to being overflowed. (A trivial fix would be to use strncpy).

But to fix properly, exploit the C++ standard library: use std::string as the type for name, const std::string& as the return type for getName, and a container like std::list<Egg> to hold the eggs in your basket.

score 0 · Answer 4 · answered Apr 20 '16 at 09:41

You don't copy the string in your Egg constructor, just a pointer, that is a starting address of the string.

It happened, that all instances of your ostrings allocate their buffers at the same place, again and again. And it happened that the buffer didn't get overwritten between the constructing for loop and the output-printing for loop.

That's why eventually all your Eggs have their name pointers pointing at the same place, and that place contains the last name built.

C++ ostringstream strange behavior

4 Answers4