Deprecate most of debug-memory by richsalz · Pull Request #10572 · openssl/openssl

richsalz · 2019-12-04T19:27:51Z

The leak-checking (and backtrace option, on some platforms) provided
by crypto-mdebug and crypto-mdebug-backtrace have been mostly neutered;
only the "make malloc fail" capability remains. OpenSSL recommends using
clang leak-detection instead.

The OPENSSL_DEBUG_MEMORY environment variable is no longer used.
CRYPTO_mem_ctrl(), CRYPTO_set_mem_debug(), CRYPTO_mem_leaks(),
CRYPTO_mem_leaks_fp() and CRYPTO_mem_leaks_cb() return a failure code.
CRYPTO_mem_debug_{malloc,realloc,free}() have been removed. All of the
above are now deprecated.

richsalz · 2019-12-04T19:38:15Z

Question for @slontis: since crypto-mdebug only does the counts and malloc-fail stuff, does it make sense to put something put an error in configure if you try to enable both?

test/build.info

doc/man3/OPENSSL_malloc.pod

slontis · 2019-12-04T22:09:13Z

since crypto-mdebug only does the counts and malloc-fail stuff, does it make sense to put something put an error in configure if you try to enable both?

Probably not necessary :)

richsalz · 2019-12-05T04:33:54Z

The memleaktest stuff needs to be revisited after #9294 gets merged. For now I had to comment out the "leak" test. Fixup commit pushed.

INSTALL

Fixes #8322 The leak-checking (and backtrace option, on some platforms) provided by crypto-mdebug and crypto-mdebug-backtrace have been mostly neutered; only the "make malloc fail" capability remains. OpenSSL recommends using the compiler's leak-detection instead. The OPENSSL_DEBUG_MEMORY environment variable is no longer used. CRYPTO_mem_ctrl(), CRYPTO_set_mem_debug(), CRYPTO_mem_leaks(), CRYPTO_mem_leaks_fp() and CRYPTO_mem_leaks_cb() return a failure code. CRYPTO_mem_debug_{malloc,realloc,free}() have been removed. All of the above are now deprecated. Merge (now really small) mem_dbg.c into mem.c

richsalz · 2019-12-10T16:54:08Z

Resolved conflicts, squashed, pushed.

richsalz · 2019-12-11T02:59:44Z

Travis failure is a timeout on Mac, after all tests passed.

levitte · 2019-12-11T09:09:35Z

INSTALL

-                   system (through Gnulib), the functions might just be stubs
-                   that do nothing.
+                   This is a no-op; the project uses the compiler's
+                   address/leak sanitizer instead.


In Configure, there's the hash table %deprecated_disablables. "crypto-mdebug-backtrace" should be removed from @disablables and added to %deprecated_disablables, like this:

diff --git a/Configure b/Configure index 22082deb4c..38b26b7d4f 100755 --- a/Configure +++ b/Configure @@ -366,7 +366,6 @@ my @disablables = ( "cms", "comp", "crypto-mdebug", - "crypto-mdebug-backtrace", "ct", "deprecated", "des", @@ -463,6 +462,7 @@ my @disablables_int = qw( my %deprecated_disablables = ( "ssl2" => undef, "buf-freelists" => undef, + "crypto-mdebug-backtrace" => undef, "hw" => "hw", # causes cascade, but no macro "hw-padlock" => "padlockeng", "ripemd" => "rmd160",

It could be that "crypto-mdebug" should be renamed to "failed-macro", with deprecation of the old name. That would look like this:

diff --git a/Configure b/Configure index 22082deb4c..5ee0955c5c 100755 --- a/Configure +++ b/Configure @@ -365,7 +365,6 @@ my @disablables = ( "cmp", "cms", "comp", - "crypto-mdebug", "crypto-mdebug-backtrace", "ct", "deprecated", @@ -386,6 +385,7 @@ my @disablables = ( "engine", "err", "external-tests", + "failed-malloc", "filenames", "fips", "fuzz-libfuzzer", @@ -463,6 +463,7 @@ my @disablables_int = qw( my %deprecated_disablables = ( "ssl2" => undef, "buf-freelists" => undef, + "crypto-mdebug" => "failed-malloc", "hw" => "hw", # causes cascade, but no macro "hw-padlock" => "padlockeng", "ripemd" => "rmd160",

You may need to check how that affects the feature macros (look in the generated include/openssl/opensslconf.h)

I did the deprecation. I don't want to add a new (esp new aliased!) ifdef. Also, malloc-failing is used for debugging :)

crypto/mem.c

include/openssl/crypto.h

CHANGES

richsalz · 2019-12-12T15:04:52Z

fixup commit pushed.

paulidale · 2019-12-12T21:04:04Z

Travis failure seems unrelated.

richsalz · 2019-12-14T18:45:34Z

ding, ready to merge.

Fixes #8322 The leak-checking (and backtrace option, on some platforms) provided by crypto-mdebug and crypto-mdebug-backtrace have been mostly neutered; only the "make malloc fail" capability remains. OpenSSL recommends using the compiler's leak-detection instead. The OPENSSL_DEBUG_MEMORY environment variable is no longer used. CRYPTO_mem_ctrl(), CRYPTO_set_mem_debug(), CRYPTO_mem_leaks(), CRYPTO_mem_leaks_fp() and CRYPTO_mem_leaks_cb() return a failure code. CRYPTO_mem_debug_{malloc,realloc,free}() have been removed. All of the above are now deprecated. Merge (now really small) mem_dbg.c into mem.c Reviewed-by: Paul Dale <[email protected]> Reviewed-by: Richard Levitte <[email protected]> (Merged from #10572)

Reviewed-by: Paul Dale <[email protected]> Reviewed-by: Richard Levitte <[email protected]> (Merged from #10572)

levitte · 2019-12-14T19:58:29Z

Merged.

742ccab Deprecate most of debug-memory
1461138 Deprecated crypto-mdebug-backtrace

romen · 2019-12-14T22:52:47Z

Now that this is merged I get errors during compilation if I configure with:

/usr/bin/perl ./Configure linux-x86_64 --debug --prefix=/usr/local/openssl -g --strict-warnings -Qunused-arguments -Wno-string-plus-int enable-ec_nistp_64_gcc_128 enable-crypto-mdebug enable-crypto-mdebug-backtrace

crypto/mem.c:151:23: error: implicit declaration of function 'backtrace' is invalid in C99 [-Werror,-Wimplicit-function-declaration]                                                                                                    
            int num = backtrace(addrs, OSSL_NELEM(addrs));
                      ^
crypto/mem.c:153:13: error: implicit declaration of function 'backtrace_symbols_fd' is invalid in C99 [-Werror,-Wimplicit-function-declaration]                                                                                         
            backtrace_symbols_fd(addrs, num, md_tracefd);
            ^
2 errors generated.
Makefile:14935: recipe for target 'crypto/libcrypto-lib-mem.o' failed
make[1]: *** [crypto/libcrypto-lib-mem.o] Error 1
make[1]: *** Waiting for unfinished jobs....
Makefile:2770: recipe for target 'build_sw' failed
make: *** [build_sw] Error 2

It disappears if I don't use enable-crypto-mdebug enable-crypto-mdebug-backtrace but it seems to me that in this PR we already did most of the work to convert the deprecated options to no-op.

What am I missing?

romen · 2019-12-14T23:03:15Z

Also, when configuring only with enable-crypto-mdebug, test fails:
make TESTS="test_symbol_presence" V=1 test :

make depend && make _tests
make[1]: Entering directory '/home/tuveri/repos/ossl_committer/move_ec'
make[1]: Leaving directory '/home/tuveri/repos/ossl_committer/move_ec'
make[1]: Entering directory '/home/tuveri/repos/ossl_committer/move_ec'
( cd test; \
  mkdir -p test-runs; \
  SRCTOP=../. \
  BLDTOP=../. \
  RESULT_D=test-runs \
  PERL="/usr/bin/perl" \
  EXE_EXT= \
  OPENSSL_ENGINES=`cd .././engines 2>/dev/null && pwd` \
  OPENSSL_MODULES=`cd .././providers 2>/dev/null && pwd` \
  /usr/bin/perl .././test/run_tests.pl test_symbol_presence )

1..4
# NOTE: developer test!  It's possible that it won't run on your
# platform, and that's perfectly fine.  This is mainly for developers
# on Unix to check that our shared libraries are consistent with the
# ordinals (util/*.num in the source tree), something that should be
# good enough a check for the other platforms as well.
ok 1 - running 'cd /home/tuveri/repos/ossl_committer/move_ec; /usr/bin/perl /home/tuveri/repos/ossl_committer/move_ec/util/mkdef.pl --ordinals /home/tuveri/repos/ossl_committer/move_ec/util/libcrypto.num --name crypto --OS linux' => 0
# Number of lines in @nm_lines before massaging: 5004
# Number of lines in @def_lines before massaging: 4864
# Number of lines in @nm_lines after massaging: 4857
# Number of lines in @def_lines after massaging: 4860
# The following symbols are missing in libcrypto.so:
#   CRYPTO_mem_debug_free
#   CRYPTO_mem_debug_malloc
#   CRYPTO_mem_debug_realloc
not ok 2 - check that there are no missing symbols in libcrypto.so

#   Failed test 'check that there are no missing symbols in libcrypto.so'
#   at ../test/recipes/01-test_symbol_presence.t line 114.
ok 3 - running 'cd /home/tuveri/repos/ossl_committer/move_ec; /usr/bin/perl /home/tuveri/repos/ossl_committer/move_ec/util/mkdef.pl --ordinals /home/tuveri/repos/ossl_committer/move_ec/util/libssl.num --name ssl --OS linux' => 0
# Number of lines in @nm_lines before massaging: 1003
# Number of lines in @def_lines before massaging: 512
# Number of lines in @nm_lines after massaging: 508
# Number of lines in @def_lines after massaging: 508
ok 4 - check that there are no missing symbols in libssl.so
# Looks like you failed 1 test of 4.
01-test_symbol_presence.t .. 
Dubious, test returned 1 (wstat 256, 0x100)
Failed 1/4 subtests 

Test Summary Report
-------------------
01-test_symbol_presence.t (Wstat: 256 Tests: 4 Failed: 1)
  Failed test:  2
  Non-zero exit status: 1
Files=1, Tests=4,  1 wallclock secs ( 0.01 usr  0.00 sys +  1.03 cusr  0.03 csys =  1.07 CPU)
Result: FAIL
Makefile:2801: recipe for target '_tests' failed
make[1]: Leaving directory '/home/tuveri/repos/ossl_committer/move_ec'
Makefile:2799: recipe for target 'tests' failed

richsalz · 2019-12-14T23:56:21Z

Please look at #10629 Sorry.

This was referenced Dec 4, 2019

Use leak sanitizer instead of internal mdebug to check for memory leaks #9294

Closed

Time to retired crypto-mdebug #8322

Closed

levitte reviewed Dec 4, 2019

View reviewed changes

test/build.info Outdated Show resolved Hide resolved

richsalz mentioned this pull request Dec 4, 2019

ex_data lock free'd too early? #10573

Closed

slontis reviewed Dec 4, 2019

View reviewed changes

doc/man3/OPENSSL_malloc.pod Outdated Show resolved Hide resolved

paulidale reviewed Dec 6, 2019

View reviewed changes

INSTALL Outdated Show resolved Hide resolved

levitte requested changes Dec 11, 2019

View reviewed changes

Deprecated crypto-mdebug-backtrace

31e9ce8

levitte approved these changes Dec 12, 2019

View reviewed changes

levitte added approval: review pending This pull request needs review by a committer branch: master Applies to master branch labels Dec 12, 2019

paulidale reviewed Dec 12, 2019

View reviewed changes

crypto/mem.c Show resolved Hide resolved

include/openssl/crypto.h Show resolved Hide resolved

CHANGES Outdated Show resolved Hide resolved

fixup! Deprecate most of debug-memory

b5c33b8

paulidale approved these changes Dec 12, 2019

View reviewed changes

paulidale added approval: done This pull request has the required number of approvals and removed approval: review pending This pull request needs review by a committer labels Dec 12, 2019

levitte added approval: ready to merge The 24 hour grace period has passed, ready to merge and removed approval: done This pull request has the required number of approvals labels Dec 14, 2019

levitte closed this Dec 14, 2019

openssl-machine pushed a commit that referenced this pull request Dec 14, 2019

Deprecated crypto-mdebug-backtrace

1461138

Reviewed-by: Paul Dale <[email protected]> Reviewed-by: Richard Levitte <[email protected]> (Merged from #10572)

richsalz deleted the rm-mdebug branch December 14, 2019 20:18

romen mentioned this pull request Dec 15, 2019

Fix build when enabling mdebug options. #10629

Closed

mattcaswell mentioned this pull request Dec 23, 2019

Fix Travis #10689

Closed

richsalz mentioned this pull request Jan 21, 2020

Remove NOTES.VALGRIND? #10921

Closed

i-to-r mentioned this pull request Jun 13, 2021

OpenSSl related fixes, Part 1 aither64/haveclip-core#2

Merged

Uh oh!

Comments

Conversation

richsalz commented Dec 4, 2019

Uh oh!

richsalz commented Dec 4, 2019

Uh oh!

Uh oh!

Uh oh!

slontis commented Dec 4, 2019

Uh oh!

richsalz commented Dec 5, 2019

Uh oh!

Uh oh!

richsalz commented Dec 10, 2019

Uh oh!

richsalz commented Dec 11, 2019

Uh oh!

levitte Dec 11, 2019

Choose a reason for hiding this comment

Uh oh!

richsalz Dec 11, 2019

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

richsalz commented Dec 12, 2019

Uh oh!

paulidale commented Dec 12, 2019

Uh oh!

richsalz commented Dec 14, 2019

Uh oh!

levitte commented Dec 14, 2019

Uh oh!

romen commented Dec 14, 2019

Uh oh!

romen commented Dec 14, 2019

Uh oh!

richsalz commented Dec 14, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants