search

EddyRivasLab / easel

Sequence analysis library used by Eddy/Rivas lab code
Other
46 stars 26 forks source link

Enforces if(foo) before free(foo) in esl _Destroy() functions #45

Closed nawrockie closed 4 years ago

nawrockie commented 4 years ago

@cryptogenomicon and @npcarter :

I just opened and closed a similar pull request (#44 ) between master and my destroy-null-check branch, which branched off develop. I closed it because the pull request should have been with develop and not master. This pull request is between develop and destroy-null-check. Sorry for the confusion.

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. 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.

  1. 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

  2. esl_sq.c:sq_free: doesn't check that sq is non-NULL at start

npcarter commented 4 years ago

I’ve added the NULL check to eel_red_black_doublekey_linked_list_Destroy and pushed it to develop.

On September 23, 2019 at 1:07:59 PM, Eric Nawrocki (notifications@github.com) wrote:

@cryptogenomicon https://github.com/cryptogenomicon :

I just opened and closed a similar pull request between master and my destroy-null-check branch, which branched off develop. I closed it because the pull request should have been with develop and not master. This pull request is between develop and destroy-null-check. Sorry for the confusion.

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. 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.

1.

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 2.

esl_sq.c:sq_free: doesn't check that sq is non-NULL at start

You can view, comment on, or merge this pull request online at:

https://github.com/EddyRivasLab/easel/pull/45 Commit Summary

File Changes

Patch Links:

— You are receiving this because your review was requested. Reply to this email directly, view it on GitHub https://github.com/EddyRivasLab/easel/pull/45?email_source=notifications&email_token=ABDJBZA3EMNANCDFDMKBJK3QLDZW5A5CNFSM4IZNNZVKYY3PNVWWK3TUL52HS4DFUVEXG43VMWVGG33NNVSW45C7NFSM4HNC7MEA, or mute the thread https://github.com/notifications/unsubscribe-auth/ABDJBZAVNZSTVV5GNLYQ4WTQLDZW5ANCNFSM4IZNNZVA .

cryptogenomicon commented 4 years ago

As of C99, free(NULL) is allowed and guaranteed to work. Therefore I've been deliberately simplifying if (foo) free(foo) to free(foo) when I've been visiting/revisiting Easel code. Most of the suggested changes are unnecessary.

That said, we do need Easel *_Destroy(foo) functions to allow for the case foo=NULL. A _Destroy function can't dereference foo (for example, it can't do free(foo->baz)) unless it checks first with if (foo). You've identified three cases of this: in esl_sq.c:sq_free(), in two _Destroy functions in esl_redblack.c, and in esl_regexp_Destroy().

Rather than disentangling the good from the unnecessary, what I'm going to do is reject the pull request (sorry), and make a separate commit with the three fixes.

None of this seems like it can be relevant to a double free() bug. In a double free() bug, you've free'd a valid (non-NULL) pointer once, then tried to free the same non-NULL pointer again. if (foo) doesn't protect you from that.