C++ : Coverity reports leaks for peculiar use of references and containers

Question

Coverity reports leaks for the following code. I would like some help understanding the errors and to re-write this code to be error free. ( The errors are annotated as comments in the code below )

int main()
{
    ...
    B* b = ...
    //  (1) Coverity: Storage is returned from 
    //      allocation function operator new
    //  (2) Coverity: Assigning ...
    A* a = new A();

    // (3) Coverity: noescape: Resource a is not freed 
    //     or pointed-to in add_a_to_b    
    b->add_a_to_b( *a );
    ...

   // (4) Coverity: Resource leak: Variable a going out 
   //     of scope leaks the storage it points to.
}

class B {
public:
    std::vector<A> a_vector;
    void add_a_to_b( const A& a )
    {
       a_vector.push_back( a );
    }

-- EDIT ---

I had a particular question about the B::add_a_to_b function, and this reflects my incomplete understanding of references perhaps: Does a_vector store a reference to A or does it create a copy of the object passed to add_a_to_b?

`b->add_a_to_b( *a );` This does not store a pointer. This does `b->add_a_to_b( a );`, but then, you would have to change the definition of `add_a_to_b` (or create an overload that takes a pointer). — PaulMcKenzie, Jan 04 '16 at 20:27
@PaulMcKenzie So it follows that a_vector would store a copy of the object A supplied to it even though add_a_to_b takes a reference as its argument? — unshul, Jan 04 '16 at 20:32
The `vector::push_back` function takes a value and creates a new object with the same value as the one you passed to it. — David Schwartz, Jan 04 '16 at 20:37
Yes. Copies are stored in a vector. If it were a `vector`, then still copies are made, but the copy that is being made is a copy of the pointer. — PaulMcKenzie, Jan 04 '16 at 20:37
If A does any resource management on its own which would make it expensive to copy, consider using emplace_back(std::move(a)), if A has a move constructor. — Michael Mueller, Jan 04 '16 at 22:19

score 3 · Accepted Answer · answered Jan 04 '16 at 20:33

You have a memory leak because you called new and you don't call delete. Furthermore, there is no reason for you to call new or allocate dynamically. You can simply allocate a automatically. The same applies to b.

B b;
A a;

...   
b.add_a_to_b(a); // b stores a copy of `a`.

score 2 · Answer 2 · edited May 23 '17 at 12:01

2

Well. You allocate memory for a, but you never use delete.

For every new there must be one delete.

delete a; // Do this when you don't need a anymore.

You can also do this - a = nullptr; to avoid a dangling pointer.

Edit:

You should learn how to use smart pointers. They're fairly easy to learn and you wouldnt have to worry about using new and delete, it'll take care of the delete.

Read this - Wiki & What is a smart pointer and when should I use one?

edited May 23 '17 at 12:01

Community

1
1

answered Jan 04 '16 at 20:16

Tarik Neaj

548
2
10

2

The real solution would be to not `new` anything. – juanchopanza Jan 04 '16 at 20:19
@juanchopanza Yes, I want to get this code to the point of not 'new' -ing anything. Please see my EDIT question above. Thanks – unshul Jan 04 '16 at 20:21
Yep, for sure. But 99/100 universities still teach new and delete so most student end up using it for a while. – Tarik Neaj Jan 04 '16 at 20:22
I doubt that. In any case, you can use this opportunity to show them how to do it right. – juanchopanza Jan 04 '16 at 20:23
@unshul STL Containers store by value. – Tarik Neaj Jan 04 '16 at 20:24

C++ : Coverity reports leaks for peculiar use of references and containers

2 Answers2

Linked