1

I have a very weird and probably obvious problem, but I can't seem to find the bug. I've got a class object that holds a pointer to another class object, and when the first's deconstructer is called, it tries to delete its pointer, but instead causes a segfault without ever entering the second's deconstructor.

Specifically, I have an instance of the class Optimizer:

class Optimizer {
public:
    Optimizer();
    ~Optimizer();

   //Lot's of public methods and such

private:
    PredictionRenderer *_predictionRenderer;

    //Lot's of member variables

};

Optimizer::~Optimizer() {
    std::cout<<"optimizer destructor:"<<_predictionRenderer->getWidth()<<std::endl;
    delete _predictionRenderer; //THIS LINE CRASHES AND NEVER MAKES IT INTO THE PREDICTION RENDERER DECONSTRUCTOR
    //other calls
}

(This is a big project, so for brevity I removed all the extra methods/variables).

Optimizer has a pointer to a PredictionRenderer object, _predictionRenderer. This pointer is initialized during the call to the constructor. The pointer is private, and I checked and made sure that it can't "get out" (that is, no one outside this Optimizer object can get ahold of this pointer. It is never returned by any of optimizer's methods and it is never passed to any method by an optimizer method).

When attempting to delete the Optimizer object, my program segfaults on the delete _predictionRenderer line. Execution never makes it into the PredictionRenderer deconstructor. I added in the print statement before the delete call to verify that the pointer was not NULL or already deleted, and the call to PredictionRenderer's getWidth method returns successfully, which suggests that it must be a valid pointer (is it possible to call a method of a deleted object?). Also, the print statement is only printed out once, so I'm pretty sure that the Optimizer object isn't being copied and deleted twice. Finally, the deconstructor for PredictionRenderer is never called, not by delete or anywhere else.

I have not idea what could be causing this. Does anyone have any insight into what is going on?

Edit: As I mentioned in the comments, this code base is big. I apologize for not showing much, but I can't really show everything as there just isn't enough space. This is someone else's code that I'm using, and from what I can tell, he never actually destructs this object, he just lets it get deallocated when the program quits. I could do this too, but it seems like a hack and not a good way to do business.

Rae_III
  • 39
  • 1
  • 10
  • 4
    `(This is a big project, so for brevity I removed all the extra methods/variables). ` No. It is important that you don't leave out the internals of your class. The reason why is that we need to see if your class follows the "Rule of 3". http://stackoverflow.com/questions/4172722/what-is-the-rule-of-three – PaulMcKenzie Feb 10 '15 at 23:13
  • If your `Optimizer` objects are copied or assigned in your program, you must have defined a user-defined copy constructor and assignment operator. Or if not, enforce that these functions are not available by either having `private` implementations of these functions, or if using C++ 11, `delete` them. Otherwise, you will be victim of the `double deallocation` errors and memory leaks. – PaulMcKenzie Feb 10 '15 at 23:22
  • It does not follow the rule of 3 (this isn't actually my code, I'm using someone else's). But the only time this would be a problem is if Optimizer gets copied (which it might), which would lead to the pointer being deleted twice. This very well could happen, but the first call to the Optimzer deconstructer crashes, so even if there are copies, only one has been attempted to be deleted. – Rae_III Feb 10 '15 at 23:22
  • 4
    Most likely, what is happening is that you are double deleting, perhaps due to NOT following the rule of 3 (5), and copying a pointer without re-allocating, so when you destroy the COPY of the object, all goes well, but when you go to destroy the original object, you delete the same object again. But could be umpteen other things, such as overwriting some buffer and thus corrupting the heap. – Mats Petersson Feb 10 '15 at 23:23
  • 2
    `But the only time this would be a problem is if Optimizer gets copied (which it might),` Then stop right there -- the program will exhibit undefined behavior, no matter how much you may want to rationalize why there is no error. – PaulMcKenzie Feb 10 '15 at 23:24
  • 1
    @Rae_III BTW, the proper term is `destructor`, not `deconstructor`. Also, does your default constructor initialize your pointer to (at least) `NULL`? – PaulMcKenzie Feb 10 '15 at 23:27
  • @MatsPetersson Yes, I've been worried that there might be double deleting, but from what I can tell it doesn't appear that this is the case. That print statement in the Optimizer deconstructer only ever gets printed once, and the PredictionRenderer deconstructer is never called from anywhere, at the delete statement above or from anywhere else in the code. If it was being double deleted, it should be called at least once before the crash, but that doesn't seem to be the case. – Rae_III Feb 10 '15 at 23:29
  • @Rae_III - Then you need to show us more code. Just posting empty functions, a pointer, and a destructor isn't going to solve your problem, unless someone makes a lucky guess. However, my guess is that you corrupted the pointer value, and it points to garbage, and then you're issuing a delete on that "garbaged" pointer. – PaulMcKenzie Feb 10 '15 at 23:31
  • 1
    So run valgrind on your sample code. Or start reducing your code-base, until you understand EVERYTHING that goes on. We can't debug a problem based on the lines you've shown, because the problem isn't in those lines, it is SOMEWHERE else in your code. – Mats Petersson Feb 10 '15 at 23:31
  • Yes, for some reason my mind keeps thinking "deconstrutor," but you're right, it's destructor (it's the opposite of a constructor, so adding de- just sounds natural). And yes, the constructor initializes the pointer using new (not to NULL, although it seems `delete NULL` is a valid call). – Rae_III Feb 10 '15 at 23:32
  • Yes, the error does seem to be somewhere else in the code, but there is a lot of it, and I can't show it all. It doesn't appear to be a corrupt pointer, as far as I can tell the only place in code where the pointer value is changed is in the constructor when it is allocated. Is there any good way to check when a pointer value changes other than just ctrl+f and looking through the code? – Rae_III Feb 10 '15 at 23:42
  • When you call `delete` your `PredictionRenderer` destructor is called. What does this destructor do? Maybe it isn't `delete` that is crashing, it is your `PredictionRenderer` destructor that is crashing. – PaulMcKenzie Feb 11 '15 at 01:09
  • 2
    declare a *private* copy-ctor declaration `Optimizer(const Optimizer&);` in the class. No need to implement (in fact, we need you *not* to). Likewise for the assignment operator, `Optimizer& operator =(const Optimizer&);`. Both should be declared `private:` and non-implemented. Then recompile from scratch. If it breaks the compilation **anywhere**, you'll know you're dead-wrong about no value copies being made. – WhozCraig Feb 11 '15 at 01:09

2 Answers2

0

Are you sure there is even a _predictionRenderer to delete? You should check first.

if (_predictionRenderer)
    delete _predictionRenderer;

If you try to delete a pointer that was never allocated memory, your program will crash.

Lawrence Aiello
  • 4,560
  • 5
  • 21
  • 35
  • 1
    Yeah, that was my first thought, but I know it at least get's allocated because the allocation is done in the Optimizer constructor. Also, as you can see in the deconstructor above, right before the delete call I call the predictionRenderer's getWidth method, which returns the correct value, so it was at least initialized. – Rae_III Feb 10 '15 at 23:15
  • 5
    `delete` on a null object is fine (so adding an extra if just leads to code bloat when it's done thousands of times in the same executable [subject to compiler not detecting it and removing it, in which case it's just waste of programmer time every time they read it]. If it's uninitialized, already deleted or otherwise "not a good and valid pointer" the if doesn't really help much. – Mats Petersson Feb 10 '15 at 23:15
  • I only mention it because this has bitten me before. – Lawrence Aiello Feb 10 '15 at 23:16
  • 3
    @LawrenceAiello: What very old compiler/C++ library was that in. As far as I know delete on 0 pointers has been safe since at least the 1998 standard. – Mats Petersson Feb 10 '15 at 23:21
  • 2
    @MatsPetersson it probably bit him because he never initialised it to `NULL`, `nullptr` or a valid value in the constructor initialiser list. – Brandon Feb 11 '15 at 00:19
  • 3
    The `if` test is worse than useless. If it is a null pointer then it makes no difference because deleting a null pointer is safe; and if it is not a null pointer then it passes the test – M.M Feb 11 '15 at 01:24
0

nothing wrong in the lines of code you posted. i suggest cross-check the value of _predictionRenderer ptr right after its initialization and compare it with the value you see in the Optimizer::~Optimizer(). they should be the same, if not you have a problem outside. may be your container object is corrupted.

tvkc
  • 21
  • 2