Fix memory leak with TLS1.2 compression#19030
Closed
juergenchrist wants to merge 1 commit intoopenssl:masterfrom
Closed
Fix memory leak with TLS1.2 compression#19030juergenchrist wants to merge 1 commit intoopenssl:masterfrom
juergenchrist wants to merge 1 commit intoopenssl:masterfrom
Conversation
Contributor
Author
|
Any chance to get this fix reviewed? The |
t8m
approved these changes
Aug 29, 2022
mattcaswell
reviewed
Aug 30, 2022
ssl/record/methods/tls_common.c
Outdated
| if (rl->version == SSL3_VERSION) | ||
| OPENSSL_cleanse(rl->mac_secret, sizeof(rl->mac_secret)); | ||
|
|
||
| SSL3_RECORD_release(rl->rrec, rl->num_recs); |
Member
There was a problem hiding this comment.
This should be SSL_MAX_PIPELINES not rl->num_recs. The number of active records at any one time may vary, so the current value for rl->num_recs may be less than the maximum value it has ever been. By using SSL_MAX_PIPELINES instead we release all records regardless of whether they are currently in use or not.
Leak sanitizer reports following leak for ssl-test-new subtest
4-tlsv1_2-both-compress:
==335733==ERROR: LeakSanitizer: detected memory leaks
Direct leak of 17728 byte(s) in 1 object(s) allocated from:
#0 0x3ff9fbba251 in malloc (/usr/lib64/libasan.so.8+0xba251)
openssl#1 0x3ff9f71744f in tls_do_uncompress ssl/record/methods/tls_common.c:868
openssl#2 0x3ff9f7175bd in tls_default_post_process_record ssl/record/methods/tls_common.c:896
openssl#3 0x3ff9f715ee7 in tls_get_more_records ssl/record/methods/tls_common.c:773
openssl#4 0x3ff9f712209 in tls_read_record ssl/record/methods/tls_common.c:958
openssl#5 0x3ff9f6ef73f in ssl3_read_bytes ssl/record/rec_layer_s3.c:1235
openssl#6 0x3ff9f776165 in tls_get_message_header ssl/statem/statem_lib.c:1198
openssl#7 0x3ff9f74709b in read_state_machine ssl/statem/statem.c:624
openssl#8 0x3ff9f74709b in state_machine ssl/statem/statem.c:478
openssl#9 0x3ff9f662e61 in SSL_do_handshake ssl/ssl_lib.c:4430
openssl#10 0x100c55d in do_handshake_step test/helpers/handshake.c:775
openssl#11 0x100c55d in do_connect_step test/helpers/handshake.c:1134
openssl#12 0x100e85b in do_handshake_internal test/helpers/handshake.c:1544
openssl#13 0x1011715 in do_handshake test/helpers/handshake.c:1738
openssl#14 0x101d1a7 in test_handshake test/ssl_test.c:543
openssl#15 0x1027875 in run_tests test/testutil/driver.c:370
openssl#16 0x1008393 in main test/testutil/main.c:30
openssl#17 0x3ff9cc2b871 in __libc_start_call_main (/usr/lib64/libc.so.6+0x2b871)
openssl#18 0x3ff9cc2b94f in __libc_start_main_alias_2 (/usr/lib64/libc.so.6+0x2b94f)
openssl#19 0x100864f (/code/openssl/test/ssl_test+0x100864f)
Direct leak of 17728 byte(s) in 1 object(s) allocated from:
#0 0x3ff9fbba251 in malloc (/usr/lib64/libasan.so.8+0xba251)
openssl#1 0x3ff9f71744f in tls_do_uncompress ssl/record/methods/tls_common.c:868
openssl#2 0x3ff9f7175bd in tls_default_post_process_record ssl/record/methods/tls_common.c:896
openssl#3 0x3ff9f715ee7 in tls_get_more_records ssl/record/methods/tls_common.c:773
openssl#4 0x3ff9f712209 in tls_read_record ssl/record/methods/tls_common.c:958
openssl#5 0x3ff9f6ef73f in ssl3_read_bytes ssl/record/rec_layer_s3.c:1235
openssl#6 0x3ff9f776165 in tls_get_message_header ssl/statem/statem_lib.c:1198
openssl#7 0x3ff9f74709b in read_state_machine ssl/statem/statem.c:624
openssl#8 0x3ff9f74709b in state_machine ssl/statem/statem.c:478
openssl#9 0x3ff9f662e61 in SSL_do_handshake ssl/ssl_lib.c:4430
openssl#10 0x100c55d in do_handshake_step test/helpers/handshake.c:775
openssl#11 0x100c55d in do_connect_step test/helpers/handshake.c:1134
openssl#12 0x1010b09 in do_handshake_internal test/helpers/handshake.c:1550
openssl#13 0x1011715 in do_handshake test/helpers/handshake.c:1738
openssl#14 0x101d1a7 in test_handshake test/ssl_test.c:543
openssl#15 0x1027875 in run_tests test/testutil/driver.c:370
openssl#16 0x1008393 in main test/testutil/main.c:30
openssl#17 0x3ff9cc2b871 in __libc_start_call_main (/usr/lib64/libc.so.6+0x2b871)
openssl#18 0x3ff9cc2b94f in __libc_start_main_alias_2 (/usr/lib64/libc.so.6+0x2b94f)
openssl#19 0x100864f (/code/openssl/test/ssl_test+0x100864f)
SUMMARY: AddressSanitizer: 35456 byte(s) leaked in 2 allocation(s).
Fix this by freeing the SSL3_RECORD structure inside the OSSL_RECORD_LAYER.
Signed-off-by: Juergen Christ <jchrist@linux.ibm.com>
b1c1c1f to
dfde7ba
Compare
t8m
approved these changes
Aug 30, 2022
mattcaswell
approved these changes
Aug 30, 2022
Collaborator
|
This pull request is ready to merge |
Member
|
Merged to master branch. Thank you for your contribution. |
openssl-machine
pushed a commit
that referenced
this pull request
Aug 31, 2022
Leak sanitizer reports following leak for ssl-test-new subtest
4-tlsv1_2-both-compress:
==335733==ERROR: LeakSanitizer: detected memory leaks
Direct leak of 17728 byte(s) in 1 object(s) allocated from:
#0 0x3ff9fbba251 in malloc (/usr/lib64/libasan.so.8+0xba251)
#1 0x3ff9f71744f in tls_do_uncompress ssl/record/methods/tls_common.c:868
#2 0x3ff9f7175bd in tls_default_post_process_record ssl/record/methods/tls_common.c:896
#3 0x3ff9f715ee7 in tls_get_more_records ssl/record/methods/tls_common.c:773
#4 0x3ff9f712209 in tls_read_record ssl/record/methods/tls_common.c:958
#5 0x3ff9f6ef73f in ssl3_read_bytes ssl/record/rec_layer_s3.c:1235
#6 0x3ff9f776165 in tls_get_message_header ssl/statem/statem_lib.c:1198
#7 0x3ff9f74709b in read_state_machine ssl/statem/statem.c:624
#8 0x3ff9f74709b in state_machine ssl/statem/statem.c:478
#9 0x3ff9f662e61 in SSL_do_handshake ssl/ssl_lib.c:4430
#10 0x100c55d in do_handshake_step test/helpers/handshake.c:775
#11 0x100c55d in do_connect_step test/helpers/handshake.c:1134
#12 0x100e85b in do_handshake_internal test/helpers/handshake.c:1544
#13 0x1011715 in do_handshake test/helpers/handshake.c:1738
#14 0x101d1a7 in test_handshake test/ssl_test.c:543
#15 0x1027875 in run_tests test/testutil/driver.c:370
#16 0x1008393 in main test/testutil/main.c:30
#17 0x3ff9cc2b871 in __libc_start_call_main (/usr/lib64/libc.so.6+0x2b871)
#18 0x3ff9cc2b94f in __libc_start_main_alias_2 (/usr/lib64/libc.so.6+0x2b94f)
#19 0x100864f (/code/openssl/test/ssl_test+0x100864f)
Direct leak of 17728 byte(s) in 1 object(s) allocated from:
#0 0x3ff9fbba251 in malloc (/usr/lib64/libasan.so.8+0xba251)
#1 0x3ff9f71744f in tls_do_uncompress ssl/record/methods/tls_common.c:868
#2 0x3ff9f7175bd in tls_default_post_process_record ssl/record/methods/tls_common.c:896
#3 0x3ff9f715ee7 in tls_get_more_records ssl/record/methods/tls_common.c:773
#4 0x3ff9f712209 in tls_read_record ssl/record/methods/tls_common.c:958
#5 0x3ff9f6ef73f in ssl3_read_bytes ssl/record/rec_layer_s3.c:1235
#6 0x3ff9f776165 in tls_get_message_header ssl/statem/statem_lib.c:1198
#7 0x3ff9f74709b in read_state_machine ssl/statem/statem.c:624
#8 0x3ff9f74709b in state_machine ssl/statem/statem.c:478
#9 0x3ff9f662e61 in SSL_do_handshake ssl/ssl_lib.c:4430
#10 0x100c55d in do_handshake_step test/helpers/handshake.c:775
#11 0x100c55d in do_connect_step test/helpers/handshake.c:1134
#12 0x1010b09 in do_handshake_internal test/helpers/handshake.c:1550
#13 0x1011715 in do_handshake test/helpers/handshake.c:1738
#14 0x101d1a7 in test_handshake test/ssl_test.c:543
#15 0x1027875 in run_tests test/testutil/driver.c:370
#16 0x1008393 in main test/testutil/main.c:30
#17 0x3ff9cc2b871 in __libc_start_call_main (/usr/lib64/libc.so.6+0x2b871)
#18 0x3ff9cc2b94f in __libc_start_main_alias_2 (/usr/lib64/libc.so.6+0x2b94f)
#19 0x100864f (/code/openssl/test/ssl_test+0x100864f)
SUMMARY: AddressSanitizer: 35456 byte(s) leaked in 2 allocation(s).
Fix this by freeing the SSL3_RECORD structure inside the OSSL_RECORD_LAYER.
Signed-off-by: Juergen Christ <jchrist@linux.ibm.com>
Reviewed-by: Matt Caswell <matt@openssl.org>
Reviewed-by: Tomas Mraz <tomas@openssl.org>
(Merged from #19030)
sftcd
pushed a commit
to sftcd/openssl
that referenced
this pull request
Sep 24, 2022
Leak sanitizer reports following leak for ssl-test-new subtest
4-tlsv1_2-both-compress:
==335733==ERROR: LeakSanitizer: detected memory leaks
Direct leak of 17728 byte(s) in 1 object(s) allocated from:
#0 0x3ff9fbba251 in malloc (/usr/lib64/libasan.so.8+0xba251)
#1 0x3ff9f71744f in tls_do_uncompress ssl/record/methods/tls_common.c:868
#2 0x3ff9f7175bd in tls_default_post_process_record ssl/record/methods/tls_common.c:896
#3 0x3ff9f715ee7 in tls_get_more_records ssl/record/methods/tls_common.c:773
#4 0x3ff9f712209 in tls_read_record ssl/record/methods/tls_common.c:958
#5 0x3ff9f6ef73f in ssl3_read_bytes ssl/record/rec_layer_s3.c:1235
#6 0x3ff9f776165 in tls_get_message_header ssl/statem/statem_lib.c:1198
#7 0x3ff9f74709b in read_state_machine ssl/statem/statem.c:624
#8 0x3ff9f74709b in state_machine ssl/statem/statem.c:478
#9 0x3ff9f662e61 in SSL_do_handshake ssl/ssl_lib.c:4430
#10 0x100c55d in do_handshake_step test/helpers/handshake.c:775
#11 0x100c55d in do_connect_step test/helpers/handshake.c:1134
#12 0x100e85b in do_handshake_internal test/helpers/handshake.c:1544
#13 0x1011715 in do_handshake test/helpers/handshake.c:1738
#14 0x101d1a7 in test_handshake test/ssl_test.c:543
#15 0x1027875 in run_tests test/testutil/driver.c:370
#16 0x1008393 in main test/testutil/main.c:30
#17 0x3ff9cc2b871 in __libc_start_call_main (/usr/lib64/libc.so.6+0x2b871)
#18 0x3ff9cc2b94f in __libc_start_main_alias_2 (/usr/lib64/libc.so.6+0x2b94f)
#19 0x100864f (/code/openssl/test/ssl_test+0x100864f)
Direct leak of 17728 byte(s) in 1 object(s) allocated from:
#0 0x3ff9fbba251 in malloc (/usr/lib64/libasan.so.8+0xba251)
#1 0x3ff9f71744f in tls_do_uncompress ssl/record/methods/tls_common.c:868
#2 0x3ff9f7175bd in tls_default_post_process_record ssl/record/methods/tls_common.c:896
#3 0x3ff9f715ee7 in tls_get_more_records ssl/record/methods/tls_common.c:773
#4 0x3ff9f712209 in tls_read_record ssl/record/methods/tls_common.c:958
#5 0x3ff9f6ef73f in ssl3_read_bytes ssl/record/rec_layer_s3.c:1235
#6 0x3ff9f776165 in tls_get_message_header ssl/statem/statem_lib.c:1198
#7 0x3ff9f74709b in read_state_machine ssl/statem/statem.c:624
#8 0x3ff9f74709b in state_machine ssl/statem/statem.c:478
#9 0x3ff9f662e61 in SSL_do_handshake ssl/ssl_lib.c:4430
#10 0x100c55d in do_handshake_step test/helpers/handshake.c:775
#11 0x100c55d in do_connect_step test/helpers/handshake.c:1134
#12 0x1010b09 in do_handshake_internal test/helpers/handshake.c:1550
#13 0x1011715 in do_handshake test/helpers/handshake.c:1738
#14 0x101d1a7 in test_handshake test/ssl_test.c:543
#15 0x1027875 in run_tests test/testutil/driver.c:370
#16 0x1008393 in main test/testutil/main.c:30
#17 0x3ff9cc2b871 in __libc_start_call_main (/usr/lib64/libc.so.6+0x2b871)
#18 0x3ff9cc2b94f in __libc_start_main_alias_2 (/usr/lib64/libc.so.6+0x2b94f)
#19 0x100864f (/code/openssl/test/ssl_test+0x100864f)
SUMMARY: AddressSanitizer: 35456 byte(s) leaked in 2 allocation(s).
Fix this by freeing the SSL3_RECORD structure inside the OSSL_RECORD_LAYER.
Signed-off-by: Juergen Christ <jchrist@linux.ibm.com>
Reviewed-by: Matt Caswell <matt@openssl.org>
Reviewed-by: Tomas Mraz <tomas@openssl.org>
(Merged from openssl#19030)
beldmit
pushed a commit
to beldmit/openssl
that referenced
this pull request
Dec 26, 2022
Leak sanitizer reports following leak for ssl-test-new subtest
4-tlsv1_2-both-compress:
==335733==ERROR: LeakSanitizer: detected memory leaks
Direct leak of 17728 byte(s) in 1 object(s) allocated from:
#0 0x3ff9fbba251 in malloc (/usr/lib64/libasan.so.8+0xba251)
#1 0x3ff9f71744f in tls_do_uncompress ssl/record/methods/tls_common.c:868
#2 0x3ff9f7175bd in tls_default_post_process_record ssl/record/methods/tls_common.c:896
#3 0x3ff9f715ee7 in tls_get_more_records ssl/record/methods/tls_common.c:773
openssl#4 0x3ff9f712209 in tls_read_record ssl/record/methods/tls_common.c:958
openssl#5 0x3ff9f6ef73f in ssl3_read_bytes ssl/record/rec_layer_s3.c:1235
openssl#6 0x3ff9f776165 in tls_get_message_header ssl/statem/statem_lib.c:1198
openssl#7 0x3ff9f74709b in read_state_machine ssl/statem/statem.c:624
openssl#8 0x3ff9f74709b in state_machine ssl/statem/statem.c:478
openssl#9 0x3ff9f662e61 in SSL_do_handshake ssl/ssl_lib.c:4430
openssl#10 0x100c55d in do_handshake_step test/helpers/handshake.c:775
openssl#11 0x100c55d in do_connect_step test/helpers/handshake.c:1134
openssl#12 0x100e85b in do_handshake_internal test/helpers/handshake.c:1544
openssl#13 0x1011715 in do_handshake test/helpers/handshake.c:1738
openssl#14 0x101d1a7 in test_handshake test/ssl_test.c:543
openssl#15 0x1027875 in run_tests test/testutil/driver.c:370
openssl#16 0x1008393 in main test/testutil/main.c:30
openssl#17 0x3ff9cc2b871 in __libc_start_call_main (/usr/lib64/libc.so.6+0x2b871)
openssl#18 0x3ff9cc2b94f in __libc_start_main_alias_2 (/usr/lib64/libc.so.6+0x2b94f)
openssl#19 0x100864f (/code/openssl/test/ssl_test+0x100864f)
Direct leak of 17728 byte(s) in 1 object(s) allocated from:
#0 0x3ff9fbba251 in malloc (/usr/lib64/libasan.so.8+0xba251)
#1 0x3ff9f71744f in tls_do_uncompress ssl/record/methods/tls_common.c:868
#2 0x3ff9f7175bd in tls_default_post_process_record ssl/record/methods/tls_common.c:896
#3 0x3ff9f715ee7 in tls_get_more_records ssl/record/methods/tls_common.c:773
openssl#4 0x3ff9f712209 in tls_read_record ssl/record/methods/tls_common.c:958
openssl#5 0x3ff9f6ef73f in ssl3_read_bytes ssl/record/rec_layer_s3.c:1235
openssl#6 0x3ff9f776165 in tls_get_message_header ssl/statem/statem_lib.c:1198
openssl#7 0x3ff9f74709b in read_state_machine ssl/statem/statem.c:624
openssl#8 0x3ff9f74709b in state_machine ssl/statem/statem.c:478
openssl#9 0x3ff9f662e61 in SSL_do_handshake ssl/ssl_lib.c:4430
openssl#10 0x100c55d in do_handshake_step test/helpers/handshake.c:775
openssl#11 0x100c55d in do_connect_step test/helpers/handshake.c:1134
openssl#12 0x1010b09 in do_handshake_internal test/helpers/handshake.c:1550
openssl#13 0x1011715 in do_handshake test/helpers/handshake.c:1738
openssl#14 0x101d1a7 in test_handshake test/ssl_test.c:543
openssl#15 0x1027875 in run_tests test/testutil/driver.c:370
openssl#16 0x1008393 in main test/testutil/main.c:30
openssl#17 0x3ff9cc2b871 in __libc_start_call_main (/usr/lib64/libc.so.6+0x2b871)
openssl#18 0x3ff9cc2b94f in __libc_start_main_alias_2 (/usr/lib64/libc.so.6+0x2b94f)
openssl#19 0x100864f (/code/openssl/test/ssl_test+0x100864f)
SUMMARY: AddressSanitizer: 35456 byte(s) leaked in 2 allocation(s).
Fix this by freeing the SSL3_RECORD structure inside the OSSL_RECORD_LAYER.
Signed-off-by: Juergen Christ <jchrist@linux.ibm.com>
Reviewed-by: Matt Caswell <matt@openssl.org>
Reviewed-by: Tomas Mraz <tomas@openssl.org>
(Merged from openssl#19030)
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Leak sanitizer reports following leak for ssl-test-new subtest
4-tlsv1_2-both-compress:
==335733==ERROR: LeakSanitizer: detected memory leaks
Direct leak of 17728 byte(s) in 1 object(s) allocated from:
#0 0x3ff9fbba251 in malloc (/usr/lib64/libasan.so.8+0xba251)
#1 0x3ff9f71744f in tls_do_uncompress ssl/record/methods/tls_common.c:868
#2 0x3ff9f7175bd in tls_default_post_process_record ssl/record/methods/tls_common.c:896
#3 0x3ff9f715ee7 in tls_get_more_records ssl/record/methods/tls_common.c:773
#4 0x3ff9f712209 in tls_read_record ssl/record/methods/tls_common.c:958
#5 0x3ff9f6ef73f in ssl3_read_bytes ssl/record/rec_layer_s3.c:1235
#6 0x3ff9f776165 in tls_get_message_header ssl/statem/statem_lib.c:1198
#7 0x3ff9f74709b in read_state_machine ssl/statem/statem.c:624
#8 0x3ff9f74709b in state_machine ssl/statem/statem.c:478
#9 0x3ff9f662e61 in SSL_do_handshake ssl/ssl_lib.c:4430
#10 0x100c55d in do_handshake_step test/helpers/handshake.c:775
#11 0x100c55d in do_connect_step test/helpers/handshake.c:1134
#12 0x100e85b in do_handshake_internal test/helpers/handshake.c:1544
#13 0x1011715 in do_handshake test/helpers/handshake.c:1738
#14 0x101d1a7 in test_handshake test/ssl_test.c:543
#15 0x1027875 in run_tests test/testutil/driver.c:370
#16 0x1008393 in main test/testutil/main.c:30
#17 0x3ff9cc2b871 in __libc_start_call_main (/usr/lib64/libc.so.6+0x2b871)
#18 0x3ff9cc2b94f in __libc_start_main_alias_2 (/usr/lib64/libc.so.6+0x2b94f)
#19 0x100864f (/code/openssl/test/ssl_test+0x100864f)
Direct leak of 17728 byte(s) in 1 object(s) allocated from:
#0 0x3ff9fbba251 in malloc (/usr/lib64/libasan.so.8+0xba251)
#1 0x3ff9f71744f in tls_do_uncompress ssl/record/methods/tls_common.c:868
#2 0x3ff9f7175bd in tls_default_post_process_record ssl/record/methods/tls_common.c:896
#3 0x3ff9f715ee7 in tls_get_more_records ssl/record/methods/tls_common.c:773
#4 0x3ff9f712209 in tls_read_record ssl/record/methods/tls_common.c:958
#5 0x3ff9f6ef73f in ssl3_read_bytes ssl/record/rec_layer_s3.c:1235
#6 0x3ff9f776165 in tls_get_message_header ssl/statem/statem_lib.c:1198
#7 0x3ff9f74709b in read_state_machine ssl/statem/statem.c:624
#8 0x3ff9f74709b in state_machine ssl/statem/statem.c:478
#9 0x3ff9f662e61 in SSL_do_handshake ssl/ssl_lib.c:4430
#10 0x100c55d in do_handshake_step test/helpers/handshake.c:775
#11 0x100c55d in do_connect_step test/helpers/handshake.c:1134
#12 0x1010b09 in do_handshake_internal test/helpers/handshake.c:1550
#13 0x1011715 in do_handshake test/helpers/handshake.c:1738
#14 0x101d1a7 in test_handshake test/ssl_test.c:543
#15 0x1027875 in run_tests test/testutil/driver.c:370
#16 0x1008393 in main test/testutil/main.c:30
#17 0x3ff9cc2b871 in __libc_start_call_main (/usr/lib64/libc.so.6+0x2b871)
#18 0x3ff9cc2b94f in __libc_start_main_alias_2 (/usr/lib64/libc.so.6+0x2b94f)
#19 0x100864f (/code/openssl/test/ssl_test+0x100864f)
SUMMARY: AddressSanitizer: 35456 byte(s) leaked in 2 allocation(s).
Fix this by freeing the SSL3_RECORD structure inside the OSSL_RECORD_LAYER.
Signed-off-by: Juergen Christ jchrist@linux.ibm.com