Automatic memory deallocation



  • So, when a dev was leaving, we had a handoff meeting.
    I knew it was bad when he said during the handoff that there were cases he had been investigating where an operation would fail and the app would need
    to be killed and restarted.

    I fired up the app, ran a task, and it immediately crashed. This is the kind of thing I'm dealing with (written in C++, obfuscated)

    Calling code looks like this:

    ParamObject* param = createParam( ... parameters for creating object ... );
    class1Object->sendRequest(param, false);
    

    The param object that created is not freed anywhere from the caller.

    This is what the class to send the request looks like:

    Class1::sendRequest(ParamObject* param, bool useHttps)
    {
      m_msg = new Message();
      m_param = param;
    
      SessionID* sessionID = new SessionID();
      msg->setParams(sessionID, m_param);
    
      ... code to send message ...
    }
    
    Class1::handleResponse(ResponseObject* response)
    {
      if ( response->httpStatus == 301 )
      {
        delete m_msg;
        sendRequest(m_param, true);
      }
    
      ... other response handling code ...
    }
    

    And here is the automatic freeing of the memory.

    Message::~Message()
    {
      if ( m_param ) free(m_param);
    }
    

    So, you can see what happens when you get a redirect due to SSL being enabled.


  • Discourse touched me in a no-no place

    Times like this are when you start to think “maybe garbage collection isn't all bad after all”.



  • I don't know... those that write code like this could find ways to confuse the garbage collector into not releasing memory.



  • @JoeCool said:

    Message::~Message()
    {
    if ( m_param ) free(m_param);
    }

    So, apart from the redundant test and the use of free() instead of delete...?



  • @PJH said:

    So, apart from the redundant test and the use of free() instead of delete...?

    Yeah, apart from that, m_param gets sent off in another sendRequest() call right after the original delete.


  • Discourse touched me in a no-no place

    @PJH said:

    So, apart from the redundant test and the use of free() instead of delete...?

    It's the mysterious squelching of what the code is about to use that is the beaut in this. Quick, pull the rug out from under your own feet!

    In my experience, it's useful to use a custom memory allocator that fills every memory chunk with crap on deallocation so that it can't be used meaningfully after a delete or free(), as that stops ever so many shenanigans in their tracks.



  • @dkf said:

    In my experience, it's useful to use a custom memory allocator that fills every memory chunk with crap on deallocation so that it can't be used meaningfully after a delete or free(), as that stops ever so many shenanigans in their tracks.

    Or just use Valgrind or similar.



  • In my opinion, memory freeing should NOT be done. As contrary point of view, if you free memory, it will give wrong impression to user that program is not doing anything. So just keep using all the memory and give impression to user that this program is doing a very difficult job.


  • Winner of the 2016 Presidential Election

    @Nagesh said:

    In my opinion, memory freeing should NOT be done.

    Bridget, is that you?


    Filed under: Are you in the Swamp Shack?


  • BINNED

    @error said:

    Filed under: Are you in the Swamp Shack?

    Thanks for that, now I can't stop trying to compose a Weird Al style Love Shack parody called Swamp Shack.


    Filed under: Fucking hell why does the tag syntax survive the quoting?


  • Discourse touched me in a no-no place

    @PJH said:

    Or just use Valgrind or similar.

    That has more overhead, but it is an excellent tool. (It also catches more bugs; the overhead is very justifiable sometimes. Just not always.)



  • @dkf said:

    [Valgrind] has more overhead, but it is an excellent tool. (It also catches more bugs; the overhead is very justifiable sometimes. Just not always.)

    It's something you do when you're tracking down a suspected memory bug or periodically to check stuff.



  • free and delete are mutually incompatible. Memory allocated with new must be released with delete, memory allocated with new[] released with delete[] and memory allocated with malloc released with free, otherwise it will crash (in some cases and which cases depends on platform, phase of the moon and value of kilogramsalad field).



  • @Bulb said:

    otherwise it will crash

    You misspelled 'undefined behavior. '



  • @PJH said:

    You misspelled 'undefined behavior. '

    Sure, other effects like formatting harddrive, setting the computer on fire, eating all your cookies or making daemons fly out of your ears are also permitted. So far I have only observed crashes, though.



  • Except, of course, the times when the undefined behavior was 'not crash and carry on as if nothing wrong happened. '


  • Discourse touched me in a no-no place

    Ear dæmons are a new one on me; I thought they preferred noses…



  • @Bulb said:

    free and delete are mutually incompatible. Memory allocated with new must be released with delete, memory allocated with new[] released with delete[] and memory allocated with malloc released with free, otherwise it will crash (in some cases and which cases depends on platform, phase of the moon and value of kilogramsalad field).

    Except this has nothing to do with the snippet I provided. I did not give the implementation details of the createParam() function. Just assume it calls malloc.

    [Edit]
    In hindsight, my obfuscation created this confusion. I shouldn't have called it ParamObject



  • @JoeCool said:

    Except this has nothing to do with the snippet I provided. I did not give the implementation details of the createParam() function. Just assume it calls malloc.

    Yes, for all we know this particular case may be matched correctly. But I think it is still relevant as general point. When you have code that mixes new and malloc(), the risk that somebody somewhere will use the wrong deallocation function is considerable. So it's a serious code smell and it's a thing you have to watch for when debugging it.

    Of course using allocation and deallocation directly is a big code smell in C++ itself. Modern C++ style is to use containers, smart pointers and such and avoid direct use of new and delete outside classes dedicated to resource management.



  • Valid point.
    This whole app is a big WTF.
    Infinite hang when an outside service is not available.
    Another crash because of no error checking, and the code just continues on.


Log in to reply
 

Looks like your connection to What the Daily WTF? was lost, please wait while we try to reconnect.