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.

Sign up for GitHub

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

Jump to bottom

DO NOT MERGE foobar look at me with my fancy message #1

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

DO NOT MERGE foobar look at me with my fancy message #1

wants to merge 1 commit into from

Conversation

bbbrumley
Copy link
Owner

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

romen
romen reviewed Jan 23, 2024
View reviewed changes
README.md
Copy link
Collaborator

Choose a reason for hiding this comment

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

Nice!

Copy link
Owner Author

Choose a reason for hiding this comment

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

🙈 this was part of my git walkthrough in my lecture today

awesome, my students are gonna love seeing an OpenSSL committer comment on this PR on Thursday!

Copy link
Owner Author

Choose a reason for hiding this comment

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

thanks for the feedback! fixed in 7f66169

Copy link
Owner Author

Choose a reason for hiding this comment

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

rebased on master

bbbrumley pushed a commit that referenced this pull request Jan 25, 2024
@bernd-edlinger @t8m
Fix a possible memory leak in req_main
ff78d94 
if the private key is output to stdout using the HARNESS_OSSL_PREFIX,
out is a stack of BIOs and must therefore free'd using BIO_free_all.

Steps to reproduce:

$ HARNESS_OSSL_PREFIX=x OPENSSL_CONF=apps/openssl.cnf util/shlib_wrap.sh apps/openssl req -new -keyout - -passout pass: </dev/null
[...]
Direct leak of 128 byte(s) in 1 object(s) allocated from:
    #0 0x7f6f692b89cf in __interceptor_malloc ../../../../src/libsanitizer/asan/asan_malloc_linux.cpp:69
    #1 0x7f6f686eda00 in CRYPTO_malloc crypto/mem.c:202
    openssl#2 0x7f6f686edba0 in CRYPTO_zalloc crypto/mem.c:222
    openssl#3 0x7f6f68471bdf in BIO_new_ex crypto/bio/bio_lib.c:83
    openssl#4 0x7f6f68491a8f in BIO_new_fp crypto/bio/bss_file.c:95
    openssl#5 0x555c5f58b378 in dup_bio_out apps/lib/apps.c:3014
    openssl#6 0x555c5f58f9ac in bio_open_default_ apps/lib/apps.c:3175
    openssl#7 0x555c5f58f9ac in bio_open_default apps/lib/apps.c:3203
    openssl#8 0x555c5f528537 in req_main apps/req.c:683
    openssl#9 0x555c5f50e315 in do_cmd apps/openssl.c:426
    openssl#10 0x555c5f4c5575 in main apps/openssl.c:307
    openssl#11 0x7f6f680461c9 in __libc_start_call_main ../sysdeps/nptl/libc_start_call_main.h:58

SUMMARY: AddressSanitizer: 128 byte(s) leaked in 1 allocation(s).

Reviewed-by: Tom Cosgrove <[email protected]>
Reviewed-by: Tomas Mraz <[email protected]>
(Merged from openssl#23365)
@bbbrumley bbbrumley force-pushed the bbb_wip branch 2 times, most recently from 8cb70c8 to e8017f7 Compare January 25, 2024 15:49
bbbrumley pushed a commit that referenced this pull request Jan 21, 2025
@ifranzki @t8m
Fix memory leak in x509_req_test
a906436 
Running the x509_req_test with address sanitizer shows a memory leak:

==186455==ERROR: LeakSanitizer: detected memory leaks
Direct leak of 53 byte(s) in 1 object(s) allocated from:
    #0 0x3ffad5f47af in malloc (/lib64/libasan.so.8+0xf47af) (BuildId: 93b3d2536d76f772a95880d76c746c150daabbee)
    #1 0x3ffac4214fb in CRYPTO_malloc crypto/mem.c:202
    openssl#2 0x3ffac421759 in CRYPTO_zalloc crypto/mem.c:222
    openssl#3 0x100e58f in test_mk_file_path test/testutil/driver.c:450
    openssl#4 0x1004671 in test_x509_req_detect_invalid_version test/x509_req_test.c:32
    openssl#5 0x100d247 in run_tests test/testutil/driver.c:342
    openssl#6 0x10042e3 in main test/testutil/main.c:31
    openssl#7 0x3ffaad34a5b in __libc_start_call_main (/lib64/libc.so.6+0x34a5b) (BuildId: 461b58df774538594b6173825bed67a9247a014d)
    openssl#8 0x3ffaad34b5d in __libc_start_main@GLIBC_2.2 (/lib64/libc.so.6+0x34b5d) (BuildId: 461b58df774538594b6173825bed67a9247a014d)
    openssl#9 0x1004569  (/root/openssl/test/x509_req_test+0x1004569) (BuildId: ab6bce0e531df1e3626a8f506d07f6ad7c7c6d57)
SUMMARY: AddressSanitizer: 53 byte(s) leaked in 1 allocation(s).

The certFilePath that is obtained via test_mk_file_path() must be freed when
no longer used.

While at it, make the certFilePath variable a local variable, there is no need
to have this a global static variable.

Fixes: openssl@7d2c0a4

Signed-off-by: Ingo Franzki <[email protected]>

Reviewed-by: Dmitry Belyavskiy <[email protected]>
Reviewed-by: Neil Horman <[email protected]>
Reviewed-by: Tomas Mraz <[email protected]>
(Merged from openssl#24715)
bbbrumley pushed a commit that referenced this pull request Jan 21, 2025
@bernd-edlinger @t8m
Fix a minor memory sanitizer issue
e63e889 
Here the undefined value "npa" passed to a function
WPACKET_sub_memcpy_u16(pkt, npa, npalen).
However the value is not really used, because "npalen" is zero,
but the call statememt itself is considered an invalid operation
by the new sanitizer.

The original sanitizer error report was:

==49175==WARNING: MemorySanitizer: use-of-uninitialized-value
    #0 0x55a276b29d6f in tls_construct_stoc_next_proto_neg /home/runner/work/openssl/openssl/ssl/statem/extensions_srvr.c:1518:21
    #1 0x55a276b15d7d in tls_construct_extensions /home/runner/work/openssl/openssl/ssl/statem/extensions.c:909:15
    openssl#2 0x55a276b513dc in tls_construct_server_hello /home/runner/work/openssl/openssl/ssl/statem/statem_srvr.c:2471:10
    openssl#3 0x55a276b2e160 in write_state_machine /home/runner/work/openssl/openssl/ssl/statem/statem.c:896:26
    openssl#4 0x55a276b2e160 in state_machine /home/runner/work/openssl/openssl/ssl/statem/statem.c:490:21
    openssl#5 0x55a276b2f562 in ossl_statem_accept /home/runner/work/openssl/openssl/ssl/statem/statem.c:309:12
    openssl#6 0x55a276a9f867 in SSL_do_handshake /home/runner/work/openssl/openssl/ssl/ssl_lib.c:4890:19
    openssl#7 0x55a276a9f605 in SSL_accept /home/runner/work/openssl/openssl/ssl/ssl_lib.c:2169:12
    openssl#8 0x55a276a3d4db in create_bare_ssl_connection /home/runner/work/openssl/openssl/test/helpers/ssltestlib.c:1281:24
    openssl#9 0x55a276a3d7cb in create_ssl_connection /home/runner/work/openssl/openssl/test/helpers/ssltestlib.c:1350:10
    openssl#10 0x55a276a64c0b in test_npn /home/runner/work/openssl/openssl/test/sslapitest.c:12266:14
    openssl#11 0x55a276b9fc20 in run_tests /home/runner/work/openssl/openssl/test/testutil/driver.c:377:21
    openssl#12 0x55a276ba0b10 in main /home/runner/work/openssl/openssl/test/testutil/main.c:31:15

Reviewed-by: Saša Nedvědický <[email protected]>
Reviewed-by: Tomas Mraz <[email protected]>
(Merged from openssl#26269)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@romen romen romen left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@bbbrumley @romen