CODEWITHSUNDEEP
c++stringpointersstreamostringstream
Borislav Stoilov
Borislav Stoilov

Reputation: 3677

C++ ostringstream strange behavior

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.

Upvotes: 4

Views: 326

Answers (4)

CiaPan
CiaPan

Reputation: 9571

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.

Upvotes: 0

Bathsheba
Bathsheba

Reputation: 234635

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.

Upvotes: 1

gsamaras
gsamaras

Reputation: 73366

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!

Upvotes: 2

Some programmer dude
Some programmer dude

Reputation: 409136

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.

Upvotes: 5

Related Questions