On the topic of Memory Leaks



  • This was a fun bug to find in 10,000,000+ lines of code. It was a "fix" for another bug. 

    Original code which "worked":

    void state_menu(Base_Op * base_op)

       static  clut * prev_clut = NULL;

       ...

       if( prev_clut == NULL )

       {

          prev_clut = (Clut*)malloc(sizeof(Clut));

       }

       ...

       set_clut(prev_clut);

       ...

    [i]Note: free(prev_clut); never clalled[/i] 

    At some point someone changed where the variable was initialized:

    void state_menu(Base_Op * base_op)

       static  clut * prev_clut;

       ...

       prev_clut = NULL;

       ...

       if( prev_clut == NULL )

       {

          prev_clut = (Clut*)malloc(sizeof(Clut));

       }

       ...

       set_clut(prev_clut);

       ...

    Of course since prev_clut is never freed and static the memory is never released and every time this function gets called it grabs another "Clut" (about 20k) off the heap the machine would eventually crash given enough time. What made this REALLY fun was that no one knew how to reproduce it, no one besides end users had seen it, I did not introduce it, and the only place it was being used was in Malta (I am in the US).

     Don't tear me a new one too badly! I tried!



  • <sarcasm>I'm surprised you didn't obfuscate that code.  I can tell exactly where that came from because of the variable names.  </sarcasm>At least change it to something like slut.

    static slut * prev_slut = NULL;

    ... 

    set_slut(prev_slut);

    Sounds like a guy who hates his previous prostitute.  But by the end of the function, he wants to make him his current prostitute again. 



  • OMG why can't I be that funny!

    Going to get me in trouble for laughing at work!

    Lol.. "prev_slut" I LOVE IT!!!



  • Declaring a static slut is a bad practice. They ought to be dynamic.

    PS - I read "static clit" when I first put my eyes on the code. 



  •  

    Oh the Vulgarity!

     



  • @Siloria said:

          prev_clut = (Clut*)malloc(sizeof(Clut));

    Ow, ow, ow, ow, ow!

    Makes me cringe every time I see that (a void* casted to another pointer, or vice versa)

    Bad compiler? Bad habits? 



  • @aib said:

    @Siloria said:

          prev_clut = (Clut*)malloc(sizeof(Clut));

    Ow, ow, ow, ow, ow!

    Makes me cringe every time I see that (a void* casted to another pointer, or vice versa)

    Bad compiler? Bad habits? 

    How the fuck do you allocate memory on the heap without having to cast the void pointers?

    Also, it's not like you really had to look through 10 millions lines of code to find this.  Surely you could just take a delta between the last version without the problem and the first version with the problem.  How long did you spend looking for it?
     



  • @tster said:

    @aib said:
    @Siloria said:

          prev_clut = (Clut*)malloc(sizeof(Clut));

    Ow, ow, ow, ow, ow!

    Makes me cringe every time I see that (a void* casted to another pointer, or vice versa)

    Bad compiler? Bad habits? 

    How the fuck do you allocate memory on the heap without having to cast the void pointers?

    Umm exactly like that, without casting the void pointers...

    Unless of course you are using C++, in which case why are you using malloc?

    If you have to cast the void pointers in C, then you haven't included the right header file and you should fix that rather than hiding the error with a cast. 



  • @gremlin said:

    Unless of course you are using C++, in which case why are you using malloc?

    Interfacing with a C library? 



  • Most C libraries don't care how the memory was allocated.


  • Discourse touched me in a no-no place

    @Siloria said:

    <font face="courier new,courier">void state_menu(Base_Op * base_op){ </font>

    <font face="courier new,courier">   static  clut * prev_clut = NULL;</font>

    <font face="courier new,courier">   ...</font>

    <font face="courier new,courier">   if( prev_clut == NULL ){</font>

    <font face="courier new,courier">      prev_clut = (Clut*)malloc(sizeof(Clut));</font>

    <font face="courier new,courier">   }</font>

    <font face="courier new,courier">   ...</font>

    <font face="courier new,courier">   set_clut(prev_clut);</font>

    <font face="courier new,courier">   ...</font>

    <font face="courier new,courier">} </font>

    [i]Note: free(prev_clut); never clalled[/i] 

    Well since the pointer is static, and only one object will be allocated, I wouldn't regard it as a memory leak since (usually) the only time you would free it is when the program exits, in which case the memory will be 'freed' anyway.

    That said, there must have been some highly obscure reason not to have a static object instead.

     



  • @H|B said:

    "static clit"

    *zap!* 

    ouch!! 



  • Had you used a real compiler and a real debugger, it would show you all the memory leaks right when your program exits.

     And using a static in such a fashion makes your code not thread-safe.



  • @henke37 said:

    Most C libraries don't care how the memory was allocated.
     

    true...  but all C++ compilers don't let you malloc without a cast.


Log in to reply