Skip to content
New issue

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

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

Already on GitHub? # to your account

Fix ASAN warning caused by global initialization order using CryptoPP functions #1747

Merged
merged 3 commits into from
Feb 19, 2019

Conversation

wezrule
Copy link
Contributor

@wezrule wezrule commented Feb 19, 2019

Fixes ASAN warning Mac/Clang:

[1m/Users/user/Library/raiblocks/crypto/cryptopp/randpool.cpp:46:4:[1m[31m runtime error: [1m[0m[1mreference binding to null pointer of type ‘const CryptoPP::NameValuePairs’[1m[0m
SUMMARY: UndefinedBehaviorSanitizer: undefined-behavior /Users/user/Library/raiblocks/crypto/cryptopp/randpool.cpp:46:4 in
[1m/Users/user/Library/raiblocks/crypto/cryptopp/cryptlib.cpp:61:64:[1m[31m runtime error: [1m[0m[1mreference binding to null pointer of type ‘const CryptoPP::NameValuePairs’[1m[0m
SUMMARY: UndefinedBehaviorSanitizer: undefined-behavior /Users/user/Library/raiblocks/crypto/cryptopp/cryptlib.cpp:61:64

This is because our ledger_constants globals variable is initialized with globals from CryptoPP which haven't been properly initialized yet. There is not specified order of global initialization within different translation units. The problem specifically comes from:

random_pool.GenerateBlock (not_an_account.bytes.data (), not_an_account.bytes.size ()); in the ledger_constants constructor. This ultimately uses a default argument of const NameValuePairs &params = g_nullNameValuePairs but g_nullNameValuePairs has not been initialized yet.

I have modified it to use lazy initialization when the nano::not_an_account is used and made it thread safe as well.

not_a_block is not used so has been removed

@wezrule wezrule changed the title Fix ASAN warning by doing lazy initialization Fix ASAN warning caused by global initialization order using CrytoPP functions Feb 19, 2019
@wezrule wezrule self-assigned this Feb 19, 2019
@cryptocode
Copy link
Contributor

ledger_constants

This changes completely with #1729

@SergiySW
Copy link
Contributor

Probably in upgrade_v12_to_v13 it could be local variable copy?

@wezrule wezrule added the bug label Feb 19, 2019
@wezrule wezrule added this to the V18.0 milestone Feb 19, 2019
@wezrule
Copy link
Contributor Author

wezrule commented Feb 19, 2019

@cryptocode I think it still needs to go in as we want this in v18

@SergiySW Good idea, and done.

I've removed the circular_buffer changes to make the diff more concise

Copy link
Contributor

@cryptocode cryptocode left a comment

Choose a reason for hiding this comment

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

@wezrule yeah, comment made before milestoning. LGTM.

@wezrule wezrule changed the title Fix ASAN warning caused by global initialization order using CrytoPP functions Fix ASAN warning caused by global initialization order using CryptoPP functions Feb 19, 2019
Copy link
Contributor

@SergiySW SergiySW left a comment

Choose a reason for hiding this comment

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

LGTM

@wezrule wezrule merged commit c0356a4 into nanocurrency:master Feb 19, 2019
@wezrule wezrule deleted the cryptopp_asan_warning branch February 19, 2019 13:13
wezrule added a commit that referenced this pull request Feb 20, 2019
… functions (#1747)

* Fix ASAN warning by doing lazy initialization

* Make a local reference and remove unrelated circular buffer changes

* Remove other unrelated change
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants