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

Implement mt_pooled_secure_allocator and use it for BLS secure allocation #2375

Merged
merged 6 commits into from
Oct 26, 2018
Merged

Implement mt_pooled_secure_allocator and use it for BLS secure allocation #2375

merged 6 commits into from
Oct 26, 2018

Conversation

codablock
Copy link

Description from second commit:

The old solution relied on thread-local-storage and was thus not compatible
to libc6 2.11 (which is the minimum supported version we use).

Also, the old solution turned out to be erroneous. It would have crashed
or memory leaked when ownership of CBLSPrivateKey would be handled over
to another thread.

UdjinM6
UdjinM6 reviewed Oct 24, 2018
View reviewed changes
src/support/allocators/mt_pooled_secure.h Outdated
@@ -0,0 +1,86 @@
// Copyright (c) 2009-2010 Satoshi Nakamoto
// Copyright (c) 2009-2015 The Bitcoin Core developers
Copy link

Choose a reason for hiding this comment

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

a copy/paste error I guess :)

src/support/allocators/pooled_secure.h Outdated
@@ -0,0 +1,72 @@
// Copyright (c) 2009-2010 Satoshi Nakamoto
// Copyright (c) 2009-2015 The Bitcoin Core developers
Copy link

Choose a reason for hiding this comment

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

same

@UdjinM6 UdjinM6 added this to the 12.4 milestone Oct 24, 2018
UdjinM6
UdjinM6 previously approved these changes Oct 24, 2018
View reviewed changes
Copy link

@UdjinM6 UdjinM6 left a comment

Choose a reason for hiding this comment

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

utACK

@UdjinM6 UdjinM6 dismissed their stale review October 24, 2018 14:22

test_dash failure

@UdjinM6
Copy link

UdjinM6 commented Oct 24, 2018

Hmmm

> ./src/test/test_dash 
Running 250 test cases...
unknown location:0: fatal error: in "evo_simplifiedmns_tests/simplifiedmns_merkleroots": memory access violation at address: 0xfffffffffffffff0: no mapping at fault address
test/evo_simplifiedmns_tests.cpp:15: last checkpoint: "simplifiedmns_merkleroots" entry.

*** 1 failure is detected in the test module "Dash Test Suite"

@codablock
Copy link
Author

Fixed (didn't check for nullptr in deallocation, resulting in 0-sizeof(size_t))

UdjinM6
UdjinM6 previously approved these changes Oct 24, 2018
View reviewed changes
Copy link

@UdjinM6 UdjinM6 left a comment

Choose a reason for hiding this comment

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

utACK

PastaPastaPasta
PastaPastaPasta previously approved these changes Oct 24, 2018
View reviewed changes
Copy link
Member

@PastaPastaPasta PastaPastaPasta left a comment

Choose a reason for hiding this comment

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

utACK

@codablock codablock dismissed stale reviews from PastaPastaPasta and UdjinM6 via 87aa3b5 October 25, 2018 07:03
@codablock
Copy link
Author

Fixed another crash that happened on shutdown. This should fix the rpc failures.

@codablock
Copy link
Author

RPC tests are failing for another reason now that is unrelated to this PR. I assume that this is related to spork15 being loaded too late, which I fixed as part of #2366 in 2a41d97

@UdjinM6
Copy link

UdjinM6 commented Oct 25, 2018 •
edited
Loading

which I fixed as part of #2366 in 2a41d97

Just merged 2366. Let's maybe rebase this one to make sure it's fixed now?

@codablock
Add pooled_secure_allocator and mt_pooled_secure_allocator
2a67ddb
@codablock
Use mt_pooled_secure_allocator for BLS secure allocation
1678d75 
The old solution relied on thread-local-storage and was thus not compatible
to libc6 2.11 (which is the minimum supported version we use).

Also, the old solution turned out to be erroneous. It would have crashed
or memory leaked when ownership of CBLSPrivateKey would be handled over
to another thread.
@codablock
Add new header files to Makefile.am
fdbf68a
@codablock
Fix copyright headers of new files
37ff5f1
@codablock
Bail out early from secure deallocation
f63cfd5
@codablock
Clean up global BLS keys when shutting down
d6d1403
UdjinM6
UdjinM6 approved these changes Oct 25, 2018
View reviewed changes
Copy link

@UdjinM6 UdjinM6 left a comment

Choose a reason for hiding this comment

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

utACK

(looks like tests are failing for other reasons)

@codablock codablock merged commit d1debfc into dashpay:develop Oct 26, 2018
@codablock codablock deleted the pr_mt_pooled_alloc branch October 26, 2018 05:04
CryptoCentric pushed a commit to absolute-community/absolute that referenced this pull request Jun 29, 2020
@codablock @CryptoCentric
Implement mt_pooled_secure_allocator and use it for BLS secure alloca…
33aa30e 
…tion (dashpay#2375)

* Add pooled_secure_allocator and mt_pooled_secure_allocator

* Use mt_pooled_secure_allocator for BLS secure allocation

The old solution relied on thread-local-storage and was thus not compatible
to libc6 2.11 (which is the minimum supported version we use).

Also, the old solution turned out to be erroneous. It would have crashed
or memory leaked when ownership of CBLSPrivateKey would be handled over
to another thread.

* Add new header files to Makefile.am

* Fix copyright headers of new files

* Bail out early from secure deallocation

* Clean up global BLS keys when shutting down
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@UdjinM6 UdjinM6 UdjinM6 approved these changes

@PastaPastaPasta PastaPastaPasta PastaPastaPasta left review comments

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

Successfully merging this pull request may close these issues.
3 participants
@codablock @UdjinM6 @PastaPastaPasta