CODEWITHSUNDEEP
c++exceptionofstream
abcdef
abcdef

Reputation: 349

C++, std::ofstream, exception

What is wrong in this code and how to fix it?

int _tmain(int argc, _TCHAR* argv[])
{
std::ostream * o = &std::cout;
char text[4096];
char *file = "D://test.txt";

if ( file != NULL ) 
{
  strcpy ( text, file );
  strcat ( text, ".log" );
  o = & std::ofstream ( text );
}
*o << "test"; //Exception
return 0;
}

Upvotes: 1

Views: 2447

Answers (6)

Matthieu M.
Matthieu M.

Reputation: 299740

You are mixing C and C++ in a very unhealthy way, I fear.

First, I heartily recommend using std::string instead of a char*, believe me, you'll have far less troubles.

Second, you should beware of pointers: they may point, if you are not careful, to places in memory that no longer host any "live" object.

I would propose the following code:

void execute(std::ostream& out) {
  out << "test\n";
} // execute

int main(int argc, char* argv[]) {
  if (argc == 1) {
    execute(std::cout);
    return 0;
  }

  std::string filename = argv[1];
  filename += ".log";

  std::ofstream file(filename.c_str());
  execute(file);
  return 0;
}

Which illustrate how to avoid the two pitfalls you fell into:

  • using std::string I avoid allocating a statically sized buffer, and thus I do risk a buffer overflow. Furthermore operations are so much easier.
  • using a function to hoist out the printing logic, I do away with the pointer and the subtle issues it introduced.

It is unfortunate that std::string and std::fstream (and consorts) do not mix so well, at the moment. Historical defect... fixed in C++0x if I remember correctly.

Upvotes: 0

Kiril Kirov
Kiril Kirov

Reputation: 38153

This

o = & std::ofstream ( text );

creates temp object, o starts pointing to the address of this object and later(right after the execution of this row) the object is destroied. Thus undefined behavior (when dereferencing invalid pointer).

The solution - create it with new:

o = new std::ofstraem( text );

BUT don't forget to free the allocated memory, before return:

*o << "test";

if  ( &std::cout != o  ) // don't forget the check .. as I did at the first time
{
    o->close();  // not absolutely necessary, 
             // as the desctructor will close the file
    delete o;
}
return 0;

Upvotes: 1

James Kanze
James Kanze

Reputation: 153899

It shouldn't compile; the expression std::ofstream( text ) is an rvalue (a temporary), and C++ doesn't allow you to take the address (operator &) of a temporary. And the lifetime of a temporary is only until the end of the full expression, so its destructor will be called (and the memory it resides in may be used for other things) as soon as you pass the ; at the end of the statement.

Just making the ofstream a named local variable doesn't help, either, since the lifetime of a variable is only to the end of the block in which it was declared (the next }). You have to define the std::ofstream before the if, and open it and set o in the if, e.g.:

std::ofstream mayOrMayNotBeUsed;
if ( file != NULL ) {
    //  ...
    mayOrMayNotBeUsed.open( text );
    if ( !mayOrMayNotBeUsed.is_open() ) {
        //  Do something intelligent here...
    }
    o = &mayOrMayNotBeUsed;
}

Upvotes: 2

Sarfaraz Nawaz
Sarfaraz Nawaz

Reputation: 361302

o = & std::ofstream ( text );

The right-side expression creates a temporary and you get the address of the temporary which is destroyed at the end of the expression. After that using o would invoked undefined behaviour.

You should be doing this:

{
   //...
   o = new std::ofstream ( text );
   if ( *o )
        throw std::exception("couldn't open the file");
}
//...

if  ( o != &std::cout )
   delete o; //must do this!

Upvotes: 4

vstm
vstm

Reputation: 12537

o = & std::ofstream ( text ); this creates a temporary ofstream object whose address is assigned to o but the object is instantly destroyed, so o points to a deleted object. This should work (using static):

int _tmain(int argc, _TCHAR* argv[])
{
    std::ostream * o = &std::cout;
    char text[4096];
    char *file = "D://test.txt";

    if ( file != NULL ) 
    {
        strcpy ( text, file );
        strcat ( text, ".log" );
        static std::ofstream myofstream( text );
        o = &myofstream;
    }
    *o << "test"; //Exception
    return 0;
}

Upvotes: 1

templatetypedef
templatetypedef

Reputation: 372694

The problem is that this code results in undefined behavior:

o = & std::ofstream ( text );

When you write

std::ofstream ( text )

This creates a temporary ostream object whose lifetime ends as soon as the statement it's in finishes executing. When you take its address and assign it to the pointer o, the pointer now points at a temporary object whose lifetime is about to end. As soon as the statement finishes executing, o is now pointing at an object whose lifetime has ended, so using that object has undefined behavior. Consequently, when you write

*o << "test";

You're trying to perform an operation on a dead object, causing problems.

To fix this, you should either

  1. Dynamically-allocate the ofstream by writing o = new std::ofstream(text);, which creates the object such that its lifetime extends past the end of the statement, or
  2. Declare the std::ofstream at the top of _tmain so that its lifetime extends throughout the rest of the function.

Hope this helps!

Upvotes: 1

Related Questions