0

I have a bug which seems to cause access to memory which has already been cleared. There are 2 classes - class B (which contains struct instances of class C and unique_ptrs of class D) and Class A which contains a vector of class B objects. Here's the code structure of the area where the bug is caused:

void foo{
  A localA(...);
  bar(&localA);
  baz(localA);
}

void bar(A* a) {
  C c1 = constructLocalC1();
  D d1 = constructLocalD1();
  a.insertB(c1, &d1);
}

Note that insertB will call the constructor for class B - something like:

void A::insertB(C c, D* d) {
  bVector.push_back(B(c, d));
}

B::B(C cIn, D* dIn) : c_(cIn) { d_ = make_unique<D>(*dIn); }  

B {
public:
  B(C c, D* d);
  C c_;
  std::unique_ptr<D> d_;
}

The implementation of constructLocalC1() looks something like (similar for constructLocalD1())

 C constructLocalC1() {
   C c1;
   c1.blah = computeBlahParameter(); // blah is of type int
   c1.bleh = computeBlehParameter(); // bleh is of type double
   return c1;
}

The observation is that when baz tries to access (the copy of) c1 present in localA, the values in there are corrupted and not the same as the ones set by bar. My conclusion from this observation is that the vector which stores B is storing an element which has become de-allocated.

I know it is slightly complicated to understand the root cause through the code snippet here, as this is highly abstracted out - glad to provide more specific details which are required.

What are potential pitfalls and causes of memory leaks in this code snippet? What are good ways to approach the debugging?

vigs1990
  • 1,639
  • 4
  • 17
  • 19
  • What does constructLocalC1() and constructLocalD1() look like? Are they calling `new`? If they are not then the object they are created will be destroyed when those functions go out of scope. – jmstoker Nov 06 '13 at 23:35
  • constructLocalC1() does not call `new`, however a.insertB calls the constructor of B with c1 and d1 and inserts the constructs B object in the vector - so B should have stored a copy of c1 and d1 before they go out of scope, right? – vigs1990 Nov 06 '13 at 23:40
  • C and D aren't going out of scope, but what is being assigned to them might be when the scope leaves the construct methods. And if that's the case, since the illegal access occurs in `baz` that would indicate that you're not properly copying the C and D objects. – jmstoker Nov 06 '13 at 23:47
  • Agreed @jmstoker - that was exactly my hypothesis too. But given that the constructor for B is being called (updated question description above), it's not clear to me how C and D are not being copied properly? – vigs1990 Nov 06 '13 at 23:55
  • Have you written a custom copy constructor for C and D? What is the return type of `constructLocalC1` and `constructLocalD1`? – jmstoker Nov 06 '13 at 23:58
  • This may seem like a silly question, but why do we need to call `new` in `push_back` - isn't the B object copied into the vector? – vigs1990 Nov 07 '13 at 00:01
  • No custom copy constructor for C and D - it uses the default copy constructor. Prototypes are: `C constructLocalC1();` and `D constructLocalD1()` – vigs1990 Nov 07 '13 at 00:02
  • let us [continue this discussion in chat](http://chat.stackoverflow.com/rooms/40678/discussion-between-vigs1990-and-jmstoker) – vigs1990 Nov 07 '13 at 00:05
  • Updated question description with implementation of `constructLocalC1` – vigs1990 Nov 07 '13 at 00:17

1 Answers1

0

Your problem probably is that you don't dynamically allocate memory on your objects inside bar. If you create an object in a function that is not, explicitly or implicitly, dynamically allocated (using new, or by using temporary objects), then your objects are created on the stack, and they will be destroyed as soon as they come out of scope (in your case, when function returns). The one option is to allocate memory for the objects inserted in vector, but be extremely careful on memory handling and deallocation, as if not handled properly can result to memory leaks. You can also use smart pointers (Boost libraries have a great implementation of them but are also added on STL), that will prevent such situation, via RAII concept approach (For more on RAII, look on this topic: RAII and smart pointers in C++).

Community
  • 1
  • 1
Nick Louloudakis
  • 5,856
  • 4
  • 41
  • 54
  • Thanks for the suggestions. I've updated the question with the code for insertB which calls the constructor for B - do you think your hypothesis still holds? – vigs1990 Nov 06 '13 at 23:53
  • If the B constructor just sets the values of the pointers of objects to member variables and doesn't create new objects dynamically (shallow copy), when it comes out of scope the objects pointed to will probably be destroyed, and the result will probably be the same. – Nick Louloudakis Nov 07 '13 at 00:05
  • Agreed - I'm not able to figure out any reason for it to use shallow copy. `c_` is assigned using the initialization list (it should do a deep copy) and `d_` is initialized using make_unique which is supposed to use `new` to construct a new object. – vigs1990 Nov 07 '13 at 00:09
  • Initialization list is doing an explicit assigmnent on your member variable. I highly doubt it will do a deep copy. – Nick Louloudakis Nov 07 '13 at 00:15