2

I have a C++ code mentioned below:

#include<iostream>
#include<stdexcept>

const long MAX = 10240000;

class Widget{
      public:
             Widget(){
                      ok = new int [1024];
                      prob = new int [100*MAX];
             }
             ~Widget(){
                       std::cout<<"\nDtoR of Widget is called\n";
                       delete ok; ok = NULL;
                       delete prob; prob = NULL;
             }
             //keeping below public: Intentionally
              int* ok;
              int* prob;
};


void func(){
    Widget* pw = NULL;  // <<<<--------------- Issue in Here
    try{
        pw = new Widget;
    }
    catch(...){
               delete pw;
               std::cout<<"\nIn catch BLOCK\n";
               if(pw->ok == NULL){
                      std::cout<<"\n OK is NULL\n";
               }
               throw;
    }
    delete pw;
}

int main(){
    try{
          func();
    }
    catch(std::bad_alloc&){
                     std::cout<<"\nError allocating memory."<<std::endl;
    }

    std::cout<<std::endl;
    system("pause");
    return 0;
}

Now in function func(), i am seeing two different behavior depending on if i do not set pointer 'pw' to NULL and if i do set 'pw' pointer to NULL (like code above). I was under the impression that it is 'good' practise to first set pointer to NULL and then initialize it. But when i initilize it to NULL then the output just shows "In catch BLOCK" and later app crashes. but if i do not set pw pointer to NULL, then i see destructor of pw is called and following output is shown w/o any app crash.

DtoR of Widget is called

In catch BLOCK

OK is NULL

Error allocating memory.

Press any key to continue . . .

My question is why such difference in one case when we are not initializing 'pw' pointer to NULL and in other case we are setting it to NULL. Why destructor is called in one case and why it was not called in another.

Note: The intent of this code is to throw bad_alloc exeption.

Fred Larson
  • 60,987
  • 18
  • 112
  • 174
Manish Shukla
  • 562
  • 6
  • 18

6 Answers6

6

If you do not set pw to NULL then you will leave it uninitialized. Then, when new operator inside a "try" block throws an exception, it never returns, and you get into catch block. Since new never returned, pw will still be not initialized, and you will pass a random address to delete. That gives you an undefined behavior.

  • 3
    Don't forget to mention that he is doing if(pw->ok) after the delete – PlasmaHH Mar 07 '12 at 20:25
  • thanks vlad for explaning this behavior. PlasmaHH: I agree. My bad. I just skipped it out. – Manish Shukla Mar 07 '12 at 20:33
  • You may also want to add that it doesn't make sense to call `delete pw;` in the `catch` block since it is guaranteed that no memory was allocated if `pw = new Widget;` throws. – Brian Neal Mar 07 '12 at 21:47
  • @BrianNeal: I thought about that. But I believe that in real life it is most likely that some code will follow the `new` invocation in try-catch block. In that case, if smart pointer is not used to delete on stack unwinding, `delete` will be required in the `catch` block, or after it in case with no errors. –  Mar 07 '12 at 22:00
  • I agree, if there were other code inside the `try` then you would want the `delete` in the `catch`. Another reason to use smart pointers! – Brian Neal Mar 07 '12 at 22:05
  • @BrianNeal: Or not use exceptions :) –  Mar 07 '12 at 22:34
4

In your catch block, you have:

if(pw->ok == NULL)

At this point, pw is NULL (or garbage, in the case that you didn't initialise it). pw-ok attempts to dereference it, giving undefined behaviour (a crash in this case).

If you didn't initialise it, then the delete pw will crash before printing the "catch" message; most likely, it will print the "Dtor" message before crashing, but there is no guarantee since you're firmly in the realm of undefined behaviour.

If you did initialise it, then delete pw is unnecessary but harmless; deleting a null pointer is defined to do nothing. So in that case you won't crash until you dereference it.

In any event, you have an unfixable memory leak - the first allocation ok = new int[1024] will have succeeded, but you have lost the only pointer to it. This is why you should always manage dynamic memory using smart pointers, containers, and other RAII techniques.

Community
  • 1
  • 1
Mike Seymour
  • 249,747
  • 28
  • 448
  • 644
1

you are going to intend the bad_alloc exeption.but you have more unhandled exceptions! It is not ok to delete pw first and then using the pointer of it! 

           delete pw;
           if(pw->ok == NULL)
rene
  • 156
  • 1
  • 2
  • 11
0

You are seeing the app crash when you set pw to NULL because of the line

if (pw->ok == NULL)

You are dereferencing NULL which is causing the crash. In the other case you are deleting garbage which will give you undefined behavior.

Also, you should not use a pointer after calling delete on it. That can cause all kinds of weird behavior.

To explain more of what is happening. Your Widget constructor is throwing the allocation exception. In this case most likely the allocation for ok completed, but the allocation for prob failed. Your constructor never finishes, leaking the memory that was allocated to the ok variable. If you want to ensure the memory is cleaned up, you should add a try catch in your constructor.

Widget() : ok(NULL), prob(NULL)
{
    try
    {
        ok = new int [1024];
        prob = new int [100*MAX];
    }
    catch(...)
    {
        std::cout << "\nCtor of Widget threw exception\n";
        delete [] ok;
        delete [] prob;
        throw;
    }
}
pstrjds
  • 16,840
  • 6
  • 52
  • 61
  • Don't forget to `delete []` instead of just `delete`! – Fred Larson Mar 07 '12 at 20:52
  • good catch, and I usually call out other dev's on that one during code reviews :) – pstrjds Mar 07 '12 at 21:04
  • Should re-throw, too. I think the best idea, though, is to use a real container such as `std::vector`. `std::array` would also work in this case, it appears. – Fred Larson Mar 07 '12 at 21:13
  • the throw had been my initial intention, just typed it up in a hurry while waiting for a project I was working on to hit a breakpoint. I agree, no need here to use new for the array, could use a vector and reserve the space (or use resize if you are going to do a memcpy or something like that). I was just trying to show how to clean up the partially constructed class (I apparently can't get that right today though) – pstrjds Mar 07 '12 at 21:40
0
  1. Why would you call pw->ok after you delete pw? It's already gone.
  2. You constructor should have member initializes

Widget():ok(NULL), prob(NULL) {
...
}

because if Widget() fails, you don't know which member variable is initialized and which is not which can cause problem in your destructor.

  1. Since you allocated with int[], you need to delete[] not just delete in your destructor.
Apprentice Queue
  • 2,036
  • 13
  • 13
  • If the constructor throws, the destructor won't be called. So it might leak, even with your suggestions. The correct solution is to use proper containers instead of native arrays. – Fred Larson Mar 07 '12 at 20:36
-1

It's good to initialize pw to NULL, but when deleting it, you should first check if pw is not null. Something like:

if (pw) delete pw;

Also, if pw is NULL, you can't reference its members.

f00l
  • 59
  • 3
  • However, looking better at your code, the `delete pw` inside the `catch` seems not appropriate, since if your programs runs that code you will have a `NULL` `pw` – f00l Mar 07 '12 at 20:29
  • 1
    I should mention, however, that your second point is spot on. – Fred Larson Mar 07 '12 at 20:39