Skip to content

Fix memory leak in drbgtest#12613

Closed
slontis wants to merge 1 commit intoopenssl:masterfrom
slontis:drbg_mem_leak_fix_09Aug2020
Closed

Fix memory leak in drbgtest#12613
slontis wants to merge 1 commit intoopenssl:masterfrom
slontis:drbg_mem_leak_fix_09Aug2020

Conversation

@slontis
Copy link
Member

@slontis slontis commented Aug 9, 2020

Valgrind showed that the second test contains a memory leak.
new_drbg() increased the ref count for the CTR_DRBG for both the EVP_RAND_fetch() and EVP_RAND_CTX_new().
it was then only decremented once by the EVP_RAND_CTX_free().

Checklist
  • documentation is added or updated
  • tests are added or updated

@slontis slontis added branch: master Applies to master branch approval: review pending This pull request needs review by a committer severity: urgent Fixes an urgent issue (exempt from 24h grace period) labels Aug 9, 2020
mspncp
mspncp approved these changes Aug 9, 2020
View reviewed changes
Copy link
Contributor

@mspncp mspncp left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

From the asan callstack it looks like this is the fix for @paulidale's memory leak. Could you please also revert his workaround 64827f4?

https://github.com/openssl/openssl/blob/cc796ed3c7592825c0c42498bf251fc7bd0fd981/test/drbgtest.c#L630-L636

@mspncp
Copy link
Contributor

mspncp commented Aug 9, 2020

And, while your at it, maybe you also want to fix this coverity issue?

*** CID 1465796:  Incorrect expression  (USELESS_CALL)
/test/drbgtest.c: 534 in new_drbg()
528                                                      "AES-256-CTR", 0);
529         params[1] = OSSL_PARAM_construct_end();
530     
531         if (!TEST_ptr(rand = EVP_RAND_fetch(NULL, "CTR-DRBG", NULL))
532                 || !TEST_ptr(drbg = EVP_RAND_CTX_new(rand, parent))
533                 || !TEST_true(EVP_RAND_set_ctx_params(drbg, params))) {
>>>     CID 1465796:  Incorrect expression  (USELESS_CALL)
>>>     Calling "EVP_RAND_CTX_rand(drbg)" is only useful for its return value, which is ignored.
534             EVP_RAND_CTX_rand(drbg);
535             EVP_RAND_free(rand);
536             drbg = NULL;
537         }
538         return drbg;
539     }

@mspncp mspncp added approval: ready to merge The 24 hour grace period has passed, ready to merge and removed approval: review pending This pull request needs review by a committer labels Aug 9, 2020
@mspncp
Copy link
Contributor

mspncp commented Aug 9, 2020

Approval includes coverity fix (removal of line 534)

@paulidale
Copy link
Contributor

paulidale commented Aug 9, 2020

Nice work @slontis.

I don't understand how my "fix" fixed anything.

@slontis slontis force-pushed the drbg_mem_leak_fix_09Aug2020 branch from cc796ed to de1bc5e Compare August 9, 2020 21:48
@slontis
Copy link
Member Author

slontis commented Aug 9, 2020

Short answer - it didnt :).
I just updated it - can you see if you agree with the last change..

@slontis
Copy link
Member Author

slontis commented Aug 9, 2020

The way the fetch works is very hard to follow..
The only way to figure this out is by putting breakpoints in
evp_rand_new(), evp_rand_up_ref() and evp_rand_free().

@paulidale
Copy link
Contributor

paulidale commented Aug 9, 2020

The fix looks okay. I still don't know what I was seeing -- it showed up as a leak in RAND_get0_primary() and the fix here seems unrelated to that code.

@slontis
Copy link
Member Author

slontis commented Aug 9, 2020 •
edited
Loading

The fix looks okay. I still don't know what I was seeing -- it showed up as a leak in RAND_get0_primary() and the fix here seems unrelated to that code.

They share the same RAND object since the ref count just gets incremented - it just shows up in RAND_get0_primary(0 as that was the first to allocate.. it isnt the one that incremented the refcount and then did not decrement it.

What I was seeing was a refcount of 2 after the setup for the CTR_DRBG.. Then it got incremented 3*2 times during the test.. Which was then decremented 3 times at the end of the test followed by a decrement of 2 on shutdown.. leaving a count of 3 which is the leak.

@slontis slontis mentioned this pull request Aug 9, 2020
Closed
1 task
@mspncp
Copy link
Contributor

mspncp commented Aug 9, 2020

... it showed up as a leak in RAND_get0_primary() and the fix here seems unrelated to that code.

Indeed, the leak is unrelated to RAND_get0_primary(). The leak sanitizer callstack shows that RAND_get0_primary() just happens to be the function which triggers the allocation of the object that gets leaked (see frame 16).

==6015==ERROR: LeakSanitizer: detected memory leaks
Direct leak of 168 byte(s) in 1 object(s) allocated from:
    #0 0x7f8ec0ab3330 in __interceptor_malloc (/usr/lib/x86_64-linux-gnu/libasan.so.5+0xe9330)
    #1 0x55da03a61705 in CRYPTO_malloc crypto/mem.c:184
    #2 0x55da03a61730 in CRYPTO_zalloc crypto/mem.c:191
    #3 0x55da03a495c3 in evp_rand_new crypto/evp/evp_rand.c:80
    #4 0x55da03a499de in evp_rand_from_dispatch crypto/evp/evp_rand.c:125
    #5 0x55da03cb466f in construct_evp_method crypto/evp/evp_fetch.c:192
    #6 0x55da03cfe6ce in ossl_method_construct_this crypto/core_fetch.c:70
    #7 0x55da03cfde85 in algorithm_do_this crypto/core_algorithm.c:65
    #8 0x55da03a6ea46 in provider_forall_loaded crypto/provider_core.c:662
    #9 0x55da03a6ecdb in ossl_provider_forall_loaded crypto/provider_core.c:740
    #10 0x55da03cfe37d in ossl_algorithm_do_all crypto/core_algorithm.c:109
    #11 0x55da03cfed5f in ossl_method_construct crypto/core_fetch.c:124
    #12 0x55da03cb4c66 in inner_evp_generic_fetch crypto/evp/evp_fetch.c:270
    #13 0x55da03cb4ecd in evp_generic_fetch crypto/evp/evp_fetch.c:309
    #14 0x55da03a4a4de in EVP_RAND_fetch crypto/evp/evp_rand.c:269
    #15 0x55da03a795c0 in rand_new_drbg crypto/rand/rand_lib.c:445
    #16 0x55da03a79ba0 in RAND_get0_primary crypto/rand/rand_lib.c:491
    #17 0x55da03a05081 in setup_tests test/drbgtest.c:635
    #18 0x55da03a06ecf in main test/testutil/main.c:29
    #19 0x7f8ec080709a in __libc_start_main ../csu/libc-start.c:308

mspncp
mspncp approved these changes Aug 9, 2020
View reviewed changes
Copy link
Contributor

@mspncp mspncp left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

reconfirmed

@slontis
Copy link
Member Author

slontis commented Aug 9, 2020

Is this an urgent fix?

@mspncp
Copy link
Contributor

mspncp commented Aug 9, 2020

It fixes a CI failure on master, so yes, it qualifies as 'urgent'. Sorry for not stating it explicitly, I thought my agreement was implied by setting the state to 'ready to merge'.

openssl-machine pushed a commit that referenced this pull request Aug 10, 2020
@slontis
Fix memory leak in drbgtest
c23add3 
Reviewed-by: Matthias St. Pierre <[email protected]>
(Merged from #12613)
@slontis
Copy link
Member Author

slontis commented Aug 10, 2020

I think it doesnt hurt to state it explicitly. (i.e. Share the blame when it goes wrong :))

@slontis
Copy link
Member Author

slontis commented Aug 10, 2020

Thanks.. Merged to master.

@slontis slontis closed this Aug 10, 2020
@DDvO DDvO mentioned this pull request Aug 10, 2020
Merged
2 tasks
swenkeratmicrosoft pushed a commit to swenkeratmicrosoft/openssl that referenced this pull request Sep 1, 2020
@slontis
Fix memory leak in drbgtest
6e485be 
Reviewed-by: Matthias St. Pierre <[email protected]>
(Merged from openssl#12613)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

1 more reviewer

@mspncp mspncp mspncp approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

approval: ready to merge The 24 hour grace period has passed, ready to merge branch: master Applies to master branch severity: urgent Fixes an urgent issue (exempt from 24h grace period)

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@slontis @mspncp @paulidale