Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

openssl: fix leak when calling relocation related functions multiple times #11555

Merged
merged 2 commits into from
May 6, 2022

Conversation

lazka
Copy link
Member

@lazka lazka commented Apr 24, 2022

Instead of relocating every time the getters are called,
relocate only on the first call for each path.

Fixes #11552

@lazka
Copy link
Member Author

lazka commented Apr 24, 2022

This isn't thread safe, but I guess it is OK to leak one more string if there is a race (??).. otherwise we'd have to do locking somehow.

@ashie
Copy link
Contributor

ashie commented Apr 25, 2022

Thanks for the quick fix!

But it seems that memory leak still exists:

Dr. Memory version 2.5.0 build 0 built on Oct 18 2021 03:01:22
Windows version: WinVer=105;Rel=2009;Build=19044;Edition=Professional
Dr. Memory results for pid 27440: "a.exe"
Application cmdline: "a.exe"
Recorded 124 suppression(s) from default C:\Program Files (x86)\Dr. Memory\bin64\suppress-default.txt

Error #1: LEAK 121 direct bytes 0x0000028645460600-0x0000028645460679 + 0 indirect bytes
# 0 replace_malloc                             [d:\a\drmemory\drmemory\common\alloc_replace.c:2580]
# 1 libcrypto-1_1-x64.dll!simplify_path        
# 2 libcrypto-1_1-x64.dll!get_relative_path    
# 3 libcrypto-1_1-x64.dll!openssl_relocation   
# 4 libcrypto-1_1-x64.dll!X509_get_default_cert_file
# 5 libcrypto-1_1-x64.dll!by_file_ctrl         
# 6 libcrypto-1_1-x64.dll!X509_STORE_set_default_paths
# 7 main 

Error #2: LEAK 97 direct bytes 0x000002864548ef20-0x000002864548ef81 + 0 indirect bytes
# 0 replace_malloc                             [d:\a\drmemory\drmemory\common\alloc_replace.c:2580]
# 1 libcrypto-1_1-x64.dll!simplify_path        
# 2 libcrypto-1_1-x64.dll!get_relative_path    
# 3 libcrypto-1_1-x64.dll!openssl_relocation   
# 4 libcrypto-1_1-x64.dll!X509_get_default_cert_dir
# 5 libcrypto-1_1-x64.dll!dir_ctrl             
# 6 libcrypto-1_1-x64.dll!X509_STORE_set_default_paths
# 7 main 

===========================================================================
FINAL SUMMARY:

DUPLICATE ERROR COUNTS:

SUPPRESSIONS USED:

ERRORS FOUND:
      0 unique,     0 total invalid heap argument(s)
      0 unique,     0 total warning(s)
      2 unique,     2 total,    218 byte(s) of leak(s)
      0 unique,     0 total,      0 byte(s) of possible leak(s)
ERRORS IGNORED:
      1 potential leak(s) (suspected false positives)
         (details: C:\Users\aho\AppData\Roaming\Dr. Memory\DrMemory-a.exe.27440.000\potential_errors.txt)
      9 unique,     9 total,   1152 byte(s) of still-reachable allocation(s)
         (re-run with "-show_reachable" for details)

It seems that rel_to_datadir in enginedir_relocation() should be also freed.

+ char * rel_to_datadir = get_relative_path (OPENSSLBIN, path);
+ strcat (exe_path, rel_to_datadir);
+ simplify_path (&exe_path[0]);
+ return malloc_copy_string(exe_path);

@lazka
Copy link
Member Author

lazka commented Apr 25, 2022

ah, right, another leak.. thanks

@lazka lazka marked this pull request as draft April 25, 2022 08:56
@ashie
Copy link
Contributor

ashie commented May 6, 2022

@lazka I've created a patch to fix remaining leaks.
clear-code@905bb3a

Could you merge it to your pull request?
Or should I open a new pull request?

@lazka
Copy link
Member Author

lazka commented May 6, 2022

Sorry for the delay. I'll incorporate your changes shortly.

lazka and others added 2 commits May 6, 2022 11:37
…times

Instead of relocating every time the getters are called,
relocate only on the first call for each path.

Fixes msys2#11552
@lazka lazka force-pushed the openssl-reloc-leak branch from 73b5c13 to 6008a21 Compare May 6, 2022 09:39
@lazka lazka marked this pull request as ready for review May 6, 2022 09:39
@lazka lazka merged commit 7ff940b into msys2:master May 6, 2022
@ashie
Copy link
Contributor

ashie commented May 6, 2022

Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Memory leak in OpenSSL relocation patch
2 participants