0

I've a C++ code sample that I can't explain why it randomly causes the app to fail with either an access violation or heap corruption. I know the sample contains code that is currently frown upon (working directly with pointers to char), but this is just for learning purposes. If anyone could take a look over the code and let me know if you see something that I'm missing, I'd very much appreciate it. Thanks.

class Operand
{
private:
    double value;
    char* message;
public:
    Operand(double value);
    ~Operand();
    char* operator+(const Operand& other);
};

Operand::Operand(double value)
{
    this->value = value;
}

char* Operand::operator+(const Operand& other)
{
    char s[256];
    double sum = this->value + other.value;
    sprintf_s(s, "The sum of %f and %f is %f", this->value, other.value, sum);
    this->message = new char[strlen(s)];  //this is where it sometimes crashes with an access violation
    strcpy_s(this->message, sizeof(s), s);
    return this->message;
}

Operand::~Operand()
{
    if (this->message != nullptr)
        delete[] this->message;
}

int main()
{
    double operand1, operand2;
    cout << "Please input the calculator parameters: operand1 operand2: ";
    cin >> operand1 >> operand2;

    auto Operand1 = Operand(operand1);
    auto Operand2 = Operand(operand2);
    char* message1 = Operand1 + Operand2;
    char* message2 = Operand2 + Operand1;
    cout << message1 << "\n";
    cout << message2 << "\n";

    return 0; //and sometimes it crashes right after outputting to cout
}

I don't see why the two char* message pointers would interfere with each other, since they belong to different Operand instances. If that is indeed the cause of it. Or maybe I'm missing something.

I'd really appreciate a second pair of eyes, as I'm out of ideas. Thanks.

samgak
  • 23,944
  • 4
  • 60
  • 82
danutz_plusplus
  • 67
  • 7

3 Answers3

4

There are multiple bugs that will result in memory corruption.

The other answers already mentioned the insufficient memory allocation. But that's the least of your problems.

Uninitialized pointer

Operand's constructor fails to initialize the message member. The destructor will then likely attempt to delete a pointer that was never newed.

Rule Of 3/5 violation

The shown class violates the Rule Of The Three. At the minimum, it also needs a proper copy constructor, and an assignment operator.

operator+ leaks memory

This will result in a memory leak, due to the failure to delete the existing contents of message, if it was already allocated.

These are all the bugs that were immediately apparent, after a cursory review of the code. There are probably a couple more.

Community
  • 1
  • 1
Sam Varshavchik
  • 114,536
  • 5
  • 94
  • 148
  • Thanks for the good feedback. I realize that there may be multiple best practices being broken in my sample, but it's mainly for learning purposes. It's not meant to be perfectly designed. But I really appreciate your pointers. – danutz_plusplus Mar 21 '17 at 23:28
  • About the uninitialized pointer, if I initialize it to nullptr, that should make the issue go away, right? Since I'm already checking for nullptr before delete-ing – danutz_plusplus Mar 21 '17 at 23:30
  • Yes, that will address that specific issue. – Sam Varshavchik Mar 21 '17 at 23:33
  • About the operator+ leaking memory, are you sure? Again, I realize it may be a very unorthodox code sample (again, for learning purposes), but I do attempt to free memory in the destructor. – danutz_plusplus Mar 21 '17 at 23:33
  • What happens when you invoke `operator+` a second time? – Sam Varshavchik Mar 21 '17 at 23:34
  • Oh, I think I get it. You mean a second time on the same object. I don't have that being done yet, in this sample. But it's a very good point. Thanks – danutz_plusplus Mar 21 '17 at 23:35
3

You need to allocate an extra byte for the terminating null:

this->message = new char[strlen(s) + 1];
samgak
  • 23,944
  • 4
  • 60
  • 82
  • Thanks, I missed that. And it seems I also had another issue right after the allocation. In the strcpy_s call, I was specifying an incorrect sizeinbytes. Specifying strlen(s)+1 made the memory violation go away – danutz_plusplus Mar 21 '17 at 23:31
1

In operator+, you are creating place for a new string withnew char[strlen(s)]. That is one character short; you need to have space for the final '\0'.

When you then copy the string into it, the '\0' overwrites the next byte in memory, which leads to undefined behavior.

Aganju
  • 6,295
  • 1
  • 12
  • 23