Delete[] Error...

I just threw together a quick exception handling class, and when I use it, I'm getting an error for some odd reason.

Here's the code:

vectorexception.h

Code:

#ifndef VECTOREXCEPTION_H_
#define VECTOREXCEPTION_H_

class VectorException {

  protected:

    char *error;

  public:

    VectorException();
    VectorException(const char *);
    ~VectorException();

    char *getError();

};

#endif

vectorexception.cc

Code:

#include <cstring>
#include "vectorexception.h"

VectorException::VectorException() {

  error =  new char[sizeof(char) * strlen("No error message specified.")];
  strcpy(error,"No error message specified.");

}

VectorException::VectorException(const char *error) {

  this->error = new char[sizeof(char) * strlen(error)];
  strcpy(this->error,error);

}

VectorException::~VectorException() {

  delete[] error;
  error = NULL;

}

char *VectorException::getError() {

  return error;

}

main.cc

Code:

#include <cstdio>
#include "vectorexception.h"

int main(void) {

  try{
    throw VectorException("oh noes");
  }
  catch(VectorException e) {
    printf("%s\n",e.getError());
  }

  return 0;

}

And the error:

Code:

oh noes
*** glibc detected *** double free or corruption (fasttop): 0x0805f158 ***
Aborted

I'm using GCC, and compiling with the flags -MMD -ansi -Wall -pedantic.

You forgot to count the \0 at the end
strlen("string")+1 is what you want.

Thanks for pointing that out, but it's still throwing the glibc error, even after changing those two lines.

This is really odd... I just stepped through my code using GDB, and I got the exception at the very end of the constructor, after all the code was done executing.

You need a copy constructor (to perform the necessary "deep copy") or use an STL string instead of char* so there are no other implications when "error" is copied from one object to another.

Is there a reason to even bother with dynamically allocating the string? Why not do something like this:

Code:

class Error
{
public:
  Error(const char* error):err(error) { }
  ~Error() {  }
  const char* what() { return err; }
private:
  const char* err;
};

int main()
{
try
{
  throw Error("Error!");
}
catch(const Error& someErr)
{
 std::cout<<someErr.what();
}

}

Thank you muchly, Lucky. That fixed it.

Edit: At JaWiB: I used pointers because I didn't think about using that method. Plus, it allows for non-hardcoded error messages (assuming I get rid of the const).

Code:

#ifndef VECTOREXCEPTION_H_
#define VECTOREXCEPTION_H_

class VectorException {

  protected:
    char *error;

  public:

    VectorException(const VectorException &rhs);    
    VectorException();
    VectorException(const char *);
    ~VectorException();

    const char *getError();

};

#endif

////////////////////////////////////////////////////////////////////////////////

#include <cstring>
#include "vectorexception.h"

VectorException::VectorException(const VectorException &rhs) {
  error = new char[sizeof(char) * strlen(rhs.error) + 1];
  strcpy(error, rhs.error);
}
    
VectorException::VectorException() {
  error =  new char[sizeof(char) * strlen("No error message specified.") + 1];
  strcpy(error,"No error message specified.");
}

VectorException::VectorException(const char *error) {
  this->error = new char[sizeof(char) * strlen(error) + 1];
  strcpy(this->error,error);
}

VectorException::~VectorException() {
  delete[] error;
  error = NULL;
}

const char* VectorException::getError() {
  return error;
}

////////////////////////////////////////////////////////////////////////////////

#include <cstdio>
#include "vectorexception.h"

int main(void) {

  try {
    throw VectorException("oh noes");
  }
  catch(VectorException e) {
    printf("%s\n",e.getError());
  }

  return 0;

}

You might as well add a copy assignment operator to that while you're at it. Otherwise it will cause the same problem as the lack of a copy constructor did. The assignment operator would have to delete the currently stored error string, or use the swap trick to let the destructor do it automatically.

You should catch exceptions by reference, there's no reason not to and in this case might save some unnecessary copying.

Using the C++ string class would mean that the copy constructor, copy assignment operator and destructor wouldn't even be necessary for your class. It would just work.

Use std::string and the sizeof is not required for new, new already knows what size each object is, it just needs to know how many to make.

Originally Posted by orbitz

Use std::string and the sizeof is not required for new, new already knows what size each object is, it just needs to know how many to make.

That's only true under the assumption that it will remain a char. I, myself, was assuming that in his actual class he might be opting to use TCHARs for full compatibility. And, of course, if he is, he'd have to use _tcscpy() and _tcslen() instead of strcpy() and strlen(), respectively.

As a general rule, it is not a good idea for any type that you throw as an exception to use dynamic memory allocation. The reason is that the process of creating the exception, or copying it around (which is potentially required during stack unwinding) can throw an exception. That can result in calling code receiving an exception it doesn't expect (eg if the memory allocation fails) or [depending on how it happens] either undefined behaviour or a call to abort() if a second exception gets thrown while the first is active.

That also means that using std::string within an exception type is a bad idea; std::string uses dynamic memory.

The example given by JaWib (which I repeat here) ...

Code:

class Error
{
public:
  Error(const char* error):err(error) { }
  ~Error() {  }
  const char* what() { return err; }
private:
  const char* err;
};

int main()
{
try
{
  throw Error("Error!");
}
catch(const Error& someErr)
{
 std::cout<<someErr.what();
}

}

is also a problem as it implicitly passes a copy of a local string around in the exception. The result may not show up in the specific example (the exception is both thrown and caught in main()), but yields undefined behaviour. To make it obvious, consider that happens if the exception is thrown from a function called by main(): e.GetError() will point to a string that was in scope at the throw point, but not when it is caught.

The more usual way to do it is;

Code:

class Error
{
public:
  Error(const char* error) {if (error != NULL) strncpy(err, error, 29); else strcpy(err, "Uknown error");}
  ~Error() {  }
  const char* what() const { return err; }
private:
  char err[30];
};

int main()
{
try
{
  throw Error("Error!");
}
catch(const Error& someErr)
{
 std::cout<<someErr.what();
}

}

If "Error!" is a constant string literal, then it won't go out of scope, so JaWiB's example is fine, correct?

Originally Posted by Daved

If "Error!" is a constant string literal, then it won't go out of scope, so JaWiB's example is fine, correct?

No.

Strictly speaking (the letter of the standard) it is out of scope for the caller and therefore an access to it (particularly dereferencing it) yields undefined behaviour. In practice, what you say is often true, but that's because compilers and linkers do not tend to truncate lifetimes of constants if there is enough free resources around to allow them to keep existing without thrashing the system. But on systems which are short of memory, the compiler and linker can (and will) purge from memory or caches anything that is no longer required to exist --- and local constant strings are a prime candidate for that.

Hmm... How does std::exception work (in general), then? I had the same idea in my head that Daved did.