CODEWITHSUNDEEP
c++memorymemory-managementmemory-leaks
Hero
Hero

Reputation: 71

Possible logical flaw in a C++ test example

At my university, there is a practical programming test in C++ - and I'm stuck with an example where I am unsure about wheter or not the task in question is even valid and possible to complete correctly.

The (simple) tasks:

At first, the tasks seemed trivial for me: For the destructor, simply write delete[] name and in the main function, use delete[] friends. Presumably, that is also what the author of this example meant us to do.

However:

There seems to be a flaw in this code example, which causes memory leaks as well as destructors to be called more than once.

The person class has no assignment operator =, meaning that as the existing Person objects such as maria are assigned to slots in the friends array in the main function, the internal allocated names are not copied. So two objects now share the same internal char* pointer! Moreover, the pointer to the name of the Person previously residing in the said array slot is permanentely lost, leading to an unavoidable memory leak.

As delete[] friends; is called - the objects in the array are destroyed - leading to their destructors being called and freeing their name members. When the program ends, though, the local Person objects in the scope of main are destructed - which of course have their name members still pointing to memory which was already freed before.

The actual question:

..

#include <iostream>
using namespace std;

int strlen(const char *str) {
    if (str==0) return 0;
    int i=0;
    for (; str[i]; ++i);
    return i;
}

void strcpy(const char *src, char *dest) {
    if (src==0 || dest==0) return;
    int i=0;
    for (; src[i]; ++i) dest[i]=src[i];
    dest[i]=’\0’;
}

class Person {
    char *name;
public:
    Person(const char *str = "Susi") {
        name = new char[strlen(str)+1];
        strcpy(str,name);
    }

    Person(const Person &p) {
        name = new char[strlen(p.name)+1];
        strcpy(p.name,name);
    }

    ~Person() {
        //...
    }

    void change() {
        name[4]='e';
    }

    ostream &print(ostream &o) const {
        o<<name;
        return o;
    }
};

int main() {
    Person maria("Maria"), peter("Peter"), franz("Franz"), luisa("Luisa");
    Person mary(maria);
    Person luise;
    Person p(luise);

    Person *friends= new Person[7];
    friends[0]=maria;
    friends[1]=peter;
    friends[2]=franz;
    friends[3]=luisa;
    friends[4]=mary;
    friends[5]=luise;
    friends[6]=p;
    friends[5]=luisa;
    friends[3].change();
    friends[4].change();

    for (int i=0; i<7; ++i) {
    friends[i].print(cout);
    cout<<endl;
    }

    //???
    return 0;
}

Upvotes: 3

Views: 349

Answers (3)

Ben Voigt
Ben Voigt

Reputation: 283733

You are absolutely right. You can fix it by only making changes at the indicated positions, however they are going to be rather extreme:

Replace the //... inside the destructor with:

    delete[] name;
}

Person& operator=(const Person& other)
{
    if (this != &other) {
        delete[] name;  // not completely exception-safe!
        name = new char[strlen(other.name)+1];
        strcpy(other.name,name);
    }
    return *this;

Another serious problem is redefining a standard function (strcpy) with a new definition that reorders the arguments.

(See also: SQL injection attacks, which also cause existing pairs of syntax elements, frequently quotes and parentheses, to be re-paired with inserted syntax elements)

Upvotes: 5

awm
awm

Reputation: 1200

For every new there should be a delete[].

Upvotes: -1

Rost
Rost

Reputation: 9089

  1. Yes, the test example is flawed, possibly it was done consciously. Class Person definitely need assignment operator, remember the Rule Of Three.
  2. No, it's not possible. Default compiler-generated assignment operator will leak memory allocated by objects in friends array and double-delete memory allocated by auto Person objects.

Upvotes: 2

Related Questions