Memory leak after pointing to NEW object

Question

struct StructA {

    StructA(parameters) { ... } //StructA onstructor
};

struct StructB {

    StructA *pObjectA;
    int counter = 0;

    void function() {
        if (counter < 1) { pObjectA = new StructA[100]; }

        pObjectA[counter] = *new StructA(parameters); //Memory leak here
        counter++;
    }
};

struct StructC {

    StructB objectB;

    ~StructC() { //StructC destructor
        delete[] objectB.pObjectA;
        objectB.pObjectA = NULL;
    }
};

int main() {

    StructC objectC;
    for (int i = 0; i < 900; i++) {
        objectC.objectB.function();
    }

    return 0;
} //Struct C destructor here

I need to create an object array and then, with each call to objectB.function(), to pass specific parameters to the constructor of StructA. The code above works perfectly, except for the memory leak, which I am unable to get rid of.

My guess is that the StructC destructor deletes only the object array, not each *new StructA(parameters). I tried to play around with pointers and delete[] a little bit, but all I got was access memory violation errors. This is the only way I can think of that works. All help appreciated.

Possible duplicate of [Use of new and delete in C++](https://stackoverflow.com/questions/9698865/use-of-new-and-delete-in-c) — dandan78, Dec 07 '18 at 18:53
`(*new T)` is almost always wrong. You would need to take a reference to it and `delete` it later (very odd!) or have the object manage it's own life time (unusual and hard to do right). — François Andrieux, Dec 07 '18 at 18:54
This looks like an overuse of `new`. Simply `pObjectA[counter] = {parameters};` ought to do the trick. — François Andrieux, Dec 07 '18 at 18:55
Having StructC's destructor doing stuff that the constructor does not is undoubtedly a source of trouble. There is *always* symmetry between the constructor and destructor. — wallyk, Dec 07 '18 at 18:56
there is no obvious reason to use a single `new` or `delete`, use `std::vector` for dynamic arrays and forget about `new` and `delete` until you really need them — 463035818_is_not_an_ai, Dec 07 '18 at 18:57
@FrançoisAndrieux: I was referring to `StructC`; the code is clearly c++ since it has `~StructC()`. I have modified my comment (above). — wallyk, Dec 07 '18 at 18:58
This looks like an attempt to do a `placement new` after allocating an array of objects, but the syntax isn't right. Perhaps `new(pObjectA+counter)StructA(parameters);` would work. There are other issues, though — Tim Randall, Dec 07 '18 at 19:01
@FrançoisAndrieux while the syntax `x = {...}` works, I'd advocate against it. In my view, it lacks clarity, and potentially could lead to problems with type conversions. I think, putting the type there like `x = X{...}`. — SergeyA, Dec 07 '18 at 19:01
@TimRandall For that to work, you would need to first explicitly call the destructor of the element currently residing at `pObjectA+counter`. That array already contains default constructed instances. — François Andrieux, Dec 07 '18 at 19:10
Thanks to everyone for suggestions. In the real code, StructA actually has about a dozen of objects of another class. Without using *new, as soon as function() finishes, these objects are destroyed, and everything dependent on it stops working. I don't think this is the way it should work, I must be missing something. — J. Doe, Dec 07 '18 at 20:55
@François Andrieux taking a reference to it and deleting it later would definitely solve the problem. Any hints on how to do that properly? — J. Doe, Dec 07 '18 at 20:57
@J.Doe That's not something you do. If you want to refer to an object you allocate, assign the pointer returned by `new` to a variable. Do not directly dereference `new`, dereference a pointer that was assigned to the result of `new`. But that's not what you need to do here at all anyway. Dynamic allocation isn't needed for assigning to `pObjectA[counter]`. — François Andrieux, Dec 07 '18 at 21:03

score 1 · Answer 1 · answered Dec 07 '18 at 19:03

A class destructor should release resources that were acquired in its constructor. It seems like you wanted to defer deleting an array allocated in one class to the destructor of a second class. Thats never a good idea. In the best case you dont have to do anything in the destructor because you use automatic storage (means what the name suggest: memory is managed automatically).

Your code could look like this:

struct StructA {    
    StructA(parameters) { ... } //StructA onstructor
};

struct StructB {
    std::vector<StructA> pObjectA;
    int counter = 0;

    void function() {
        if (counter < 1) { pObjectA.reserve(100); }
        pObjectA.emplace_back(parameters);    
        counter++;
    }
};

struct StructC {
    StructB objectB;
};

int main() {

    StructC objectC;
    for (int i = 0; i < 900; i++) {
        objectC.objectB.function();
    }    
    return 0;
}

Note that I tried to keep the structure as is maybe there are other things to change. For example you dont need counter, as you can use std::vector::size to query the number of elements in the vector.

PS: As you already noticed, this is a memory leak:

  pObjectA[counter] = *new StructA(parameters); //Memory leak here

It is not really clear why you wrote that code in the first place. The idomatic way to create an object of type StructA is StructA a; (no new!).

score 0 · Answer 2 · answered Dec 07 '18 at 19:01

0

As you correctly assumed, memory leaks are caused by not properly cleaning up all new with corresponsing delete. However in idiomatic C++ there's no use to use new and delete directly.

Use std::vector, std::shared_ptr and std::unique_ptr to let RAII keep track of dynamically created objects, references to them and when to clean up. Not only is it more robust, it's also a lot shorter and easier to read.

With your code's general overall structure:

#include <memory>
#include <vector>

struct StructA {
};

struct StructB {
    std::vector<std::shared_ptr<StructA>> objectAs;

    void function() {
        objectAs.push_back(
            std::make_shared<StructA>( /*parameters*/ )
        );
    }
};

struct StructC {
    StructB objectB;
};

int main() {
    StructC objectC;
    for (int i = 0; i < 900; i++) {
        objectC.objectB.function();
    }
    return 0;
}

answered Dec 07 '18 at 19:01

datenwolf

159,371
13
185
298

From the code, it is not clear why `shared_ptr` was chosen. – SergeyA Dec 07 '18 at 19:02
@SergeyA: You mean instead of `unique_ptr`? I assumed OP may have wanted to keep several "references" (not the C++ kind, the logical kind) to specific objects, from different places. For non-concurrent programs the practical difference between `unique_ptr` and `shared_ptr` at runtime is minimal. At compile time it boils down to only moves allowes, vs. copies allowed. One can still move `shared_ptr` around. – datenwolf Dec 07 '18 at 19:07
For pointers in general. I see no reason for pointer semantic in shown program. We can assume there is one, but it is not shown here, and I have a feeling OP is just misusing it. – SergeyA Dec 07 '18 at 19:09
OP: "I need to create an object array and then..." also the objects being created in the class, not pointers getting passed from somewhere suggests that there was no need for pointers in the first place, though maybe there is.... too often pointers are only used due to a misunderstanding of how to create objects or arrays – 463035818_is_not_an_ai Dec 07 '18 at 19:10
Terminology helper: [What is meant by Resource Acquisition is Initialization (RAII)?](https://stackoverflow.com/questions/2321511/what-is-meant-by-resource-acquisition-is-initialization-raii) – user4581301 Dec 07 '18 at 19:12

Memory leak after pointing to NEW object

2 Answers2