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

Verifying benchmark regression #6

Merged
merged 52 commits into from
Sep 19, 2023
Merged

Verifying benchmark regression #6

merged 52 commits into from
Sep 19, 2023

Conversation

eukarpov
Copy link
Member

No description provided.

eukarpov pushed a commit that referenced this pull request Aug 28, 2023
…STRINGS)

A recursive OPENSSL_init_crypto(OPENSSL_INIT_LOAD_CRYPTO_STRINGS) call
may happen if an out-of-memory error happens at the first callstack,
and the dead-lock happens at the second callstack, because ossl_err_get_state_int
calls OPENSSL_init_crypto(OPENSSL_INIT_LOAD_CRYPTO_STRINGS) although that
call is currently already executing.

At least on posix system this causes the process to freeze at this
point, and must be avoided whatever it takes.

The fix is using err_shelve_state around the critical region, which
makes ossl_err_get_state_int return early and not call the recursive
OPENSSL_init_crypto(OPENSSL_INIT_LOAD_CRYPTO_STRINGS).

This can be reproduced with my error injection patch.

The test vector has been validated on the master branch:

$ ERROR_INJECT=1692279870 ../util/shlib_wrap.sh ./asn1parse-test ./corpora/asn1parse/027f6e82ba01d9db9a9167b83e56cc9f2c602550
ERROR_INJECT=1692279870
    #0 0x7f280b42fef8 in __sanitizer_print_stack_trace ../../../../src/libsanitizer/asan/asan_stack.cpp:86
    #1 0x5610a3f396b4 in my_malloc fuzz/test-corpus.c:114
    #2 0x7f280a2eb94c in CRYPTO_malloc crypto/mem.c:177
    #3 0x7f280a2dafdb in OPENSSL_LH_insert crypto/lhash/lhash.c:114
    #4 0x7f280a1c87fe in err_load_strings crypto/err/err.c:264
    #5 0x7f280a1c87fe in err_load_strings crypto/err/err.c:259
    #6 0x7f280a1c87fe in ERR_load_strings_const crypto/err/err.c:301
    #7 0x7f280a6f513b in ossl_err_load_PROV_strings providers/common/provider_err.c:233
    #8 0x7f280a1cf015 in ossl_err_load_crypto_strings crypto/err/err_all.c:109
    #9 0x7f280a2e9b8c in ossl_init_load_crypto_strings crypto/init.c:190
    #10 0x7f280a2e9b8c in ossl_init_load_crypto_strings_ossl_ crypto/init.c:181
    #11 0x7f2808cfbf67  (/lib/x86_64-linux-gnu/libc.so.6+0x99f67)
    #12 0x7f280a32301e in CRYPTO_THREAD_run_once crypto/threads_pthread.c:154
    #13 0x7f280a2ea1da in OPENSSL_init_crypto crypto/init.c:553
    #14 0x5610a3f38e2f in FuzzerInitialize fuzz/asn1parse.c:29
    #15 0x5610a3f38783 in main fuzz/test-corpus.c:194
    #16 0x7f2808c8bd8f  (/lib/x86_64-linux-gnu/libc.so.6+0x29d8f)
    #17 0x7f2808c8be3f in __libc_start_main (/lib/x86_64-linux-gnu/libc.so.6+0x29e3f)
    openssl#18 0x5610a3f38d34 in _start (/home/runner/work/openssl/openssl/fuzz/asn1parse-test+0x3d34)

AddressSanitizer:DEADLYSIGNAL
=================================================================
==27629==ERROR: AddressSanitizer: ABRT on unknown address 0x03e900006e23 (pc 0x7f2808cfbef8 bp 0x7f280b36afe0 sp 0x7ffd545b2460 T0)
    #0 0x7f2808cfbef8  (/lib/x86_64-linux-gnu/libc.so.6+0x99ef8)
    #1 0x7f280a32301e in CRYPTO_THREAD_run_once crypto/threads_pthread.c:154
    #2 0x7f280a2ea1da in OPENSSL_init_crypto crypto/init.c:553
    #3 0x7f280a1c935e in ossl_err_get_state_int crypto/err/err.c:705
    #4 0x7f280a1cf1f9 in ERR_new crypto/err/err_blocks.c:20
    #5 0x7f280a2eb9ac in CRYPTO_malloc crypto/mem.c:205
    #6 0x7f280a2dafdb in OPENSSL_LH_insert crypto/lhash/lhash.c:114
    #7 0x7f280a1c87fe in err_load_strings crypto/err/err.c:264
    #8 0x7f280a1c87fe in err_load_strings crypto/err/err.c:259
    #9 0x7f280a1c87fe in ERR_load_strings_const crypto/err/err.c:301
    #10 0x7f280a6f513b in ossl_err_load_PROV_strings providers/common/provider_err.c:233
    #11 0x7f280a1cf015 in ossl_err_load_crypto_strings crypto/err/err_all.c:109
    #12 0x7f280a2e9b8c in ossl_init_load_crypto_strings crypto/init.c:190
    #13 0x7f280a2e9b8c in ossl_init_load_crypto_strings_ossl_ crypto/init.c:181
    #14 0x7f2808cfbf67  (/lib/x86_64-linux-gnu/libc.so.6+0x99f67)
    #15 0x7f280a32301e in CRYPTO_THREAD_run_once crypto/threads_pthread.c:154
    #16 0x7f280a2ea1da in OPENSSL_init_crypto crypto/init.c:553
    #17 0x5610a3f38e2f in FuzzerInitialize fuzz/asn1parse.c:29
    openssl#18 0x5610a3f38783 in main fuzz/test-corpus.c:194
    openssl#19 0x7f2808c8bd8f  (/lib/x86_64-linux-gnu/libc.so.6+0x29d8f)
    openssl#20 0x7f2808c8be3f in __libc_start_main (/lib/x86_64-linux-gnu/libc.so.6+0x29e3f)
    openssl#21 0x5610a3f38d34 in _start (/home/runner/work/openssl/openssl/fuzz/asn1parse-test+0x3d34)

AddressSanitizer can not provide additional info.
SUMMARY: AddressSanitizer: ABRT (/lib/x86_64-linux-gnu/libc.so.6+0x99ef8)
==27629==ABORTING

Reviewed-by: Tomas Mraz <[email protected]>
Reviewed-by: Tom Cosgrove <[email protected]>
(Merged from openssl#21683)
@eukarpov eukarpov changed the title Add benchmark comparison Verifying benchmark regression Sep 13, 2023
Copy link
Member

@Blackhex Blackhex left a comment

Choose a reason for hiding this comment

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

Few comments, otherwise LGTM. Overall, good job done and the result will provide really usefull regressions insights.

.github/workflows/ci-arm64.yml Outdated Show resolved Hide resolved
.github/workflows/ci-arm64.yml Outdated Show resolved Hide resolved
.github/workflows/scripts/benchmark.sh Show resolved Hide resolved
@eukarpov eukarpov merged commit ba61083 into dev Sep 19, 2023
5 checks passed
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.

2 participants