@cryptogenomicon : I found and 'fixed' 10 files with a _Destroy() function that did not enforce the convention that a pointer be checked for non-NULLness before calling free. I did a separate commit for each fix with the hope that it might make it easier for you to accept some and not others if that's what you want to do (but I'm not sure if it will make it easier).
The motivation for this was a bug report for a non-reproducible bug in cmscan that user provided error output for including the message
I couldn't reproduce in my hands, but checked all possible free() calls and found exactly 1 that didn't check for non-NULLness before freeing (e.g. with if(foo) prior to free(foo)). That was in esl_random.c::esl_randomness_Destroy(). The large majority of easel _Destroy() functions follow the if(foo) before free(foo) convention, so I decided to go ahead and check all easel destroy functions to make sure they all did. I found 9 others that did not and propose the fixes here.
There are 2 cases that I found that did not enforce the convention but that I did not understand well enough to propose a fix. I ask that you and Nick (who I think wrote esl_red_black) take a look at them and make changes if appropriate.
esl_red_black.h:void esl_red_black_doublekey_linked_list_Destroy(ESL_RED_BLACK_DOUBLEKEY *tree): doesn't check that tree is non-NULL at start
esl_sq.c:sq_free: doesn't check that sq is non-NULL at start
@cryptogenomicon : I found and 'fixed' 10 files with a
_Destroy()function that did not enforce the convention that a pointer be checked for non-NULLness before calling free. I did a separate commit for each fix with the hope that it might make it easier for you to accept some and not others if that's what you want to do (but I'm not sure if it will make it easier).The motivation for this was a bug report for a non-reproducible bug in cmscan that user provided error output for including the message
** glibc detected *** cmscan: double free or corruption (!prev): 0x000000000125c440 ***I couldn't reproduce in my hands, but checked all possible
free()calls and found exactly 1 that didn't check for non-NULLness before freeing (e.g. withif(foo)prior tofree(foo)). That was inesl_random.c::esl_randomness_Destroy(). The large majority of easel_Destroy()functions follow theif(foo)beforefree(foo)convention, so I decided to go ahead and check all easel destroy functions to make sure they all did. I found 9 others that did not and propose the fixes here.There are 2 cases that I found that did not enforce the convention but that I did not understand well enough to propose a fix. I ask that you and Nick (who I think wrote
esl_red_black) take a look at them and make changes if appropriate.esl_red_black.h:void esl_red_black_doublekey_linked_list_Destroy(ESL_RED_BLACK_DOUBLEKEY *tree): doesn't check that tree is non-NULL at startesl_sq.c:sq_free: doesn't check that sq is non-NULL at start