Prevent UB in DeleteLock() function #18881

hebasto · 2020-05-05T06:05:34Z

Tracking our instrumented mutexes (Mutex and RecursiveMutex types) requires that all involved objects should not be destroyed until after their last use. On master (ec79b5f) we have two problems related to the object destroying order:

the function-local static lockdata object that is destroyed at program exit
the thread_local g_lockstack that is destroyed at thread exit

Both cases could cause UB at program exit in so far as mutexes are used in other static object destructors.

Fix #18824

hebasto · 2020-05-05T06:06:02Z

cc @sipa

maflcko

ACK

src/sync.cpp

hebasto · 2020-05-05T10:52:54Z

Updated 5a406f7 -> 7e3f40d (pr18881.01 -> pr18881.02, diff):

addressed @MarcoFalke's comment:

not sure why auto needs to hide both the type and the pointer-ness. The additional 5 chars don't seem overly verbose.

doc/developer-notes.md

hebasto · 2020-05-05T11:49:10Z

Updated 7e3f40d -> a223407 (pr18881.02 -> pr18881.03, diff):

addressed @jonatack's comment:

approach guarantees

jonatack

Concept ACK/ACK

doc/developer-notes.md

src/logging.cpp

src/random.cpp

doc/developer-notes.md

src/sync.cpp

hebasto · 2020-05-05T12:14:27Z

Updated a223407 -> d3c2686 (pr18881.03 -> pr18881.04, diff):

addressed @jonatack's comments

jonatack

Iterative re-review (apologies)

doc/developer-notes.md

hebasto · 2020-05-05T13:26:22Z

Updated d3c2686 -> 86ce03c (pr18881.04 -> pr18881.05, diff):

addressed @jonatack's comments

jonatack

ACK

doc/developer-notes.md

laanwj · 2020-05-07T12:52:21Z

doc/developer-notes.md

+
+Both cases lead to undefined behavior.
+
+To prevent the static initialization order problem, be sure to Construct On First Use:


I don't think you should make this suggestion in general. This solution, while valuable in rare cases such as logging and tracking locks, is a brute-force one that does not belong in actual application code. It's messy and assumes we don't actually know what's going on (in regard to initialization dependencies) or what are all the edge cases.

You're supposed to expliclty allocate things in a deterministic order during initialization and destroy it in a (usually reversed) deterministic order on shutdown in the appropriate functions in init.cpp, not rely on global initialization order nor first use.

laanwj · 2020-05-07T12:53:07Z

ACK on using this solution in this specific place but NACK on suggesting its use in general.

jonatack · 2020-05-07T14:23:03Z

looked more deeply into this to try to understand it better

I didn't find a direct reference to a "COFU" or "Construct on First Use" idiom in any of these books. The best references online seemed to be:

in the isocpp wiki faq starting from https://isocpp.org/wiki/faq/ctors#static-init-order
as an exception to https://isocpp.github.io/CppCoreGuidelines/CppCoreGuidelines#i3-avoid-singletons
and as idiom 16 in https://upload.wikimedia.org/wikipedia/commons/5/55/MoreCppIdioms.pdf

and I agree with not suggesting it in general.

hebasto · 2020-05-07T14:40:51Z

Updated 86ce03c -> 6bc6868 (pr18881.05 -> pr18881.06, diff):

only the fix of UB in the DeleteLock() function remains

maflcko · 2020-05-07T14:41:51Z

ACK 6bc6868

jonatack

Light code review ACK 6bc6868, this commit appears to be an implementation of the solution in https://isocpp.org/wiki/faq/ctors#static-init-order-on-first-use-members

hebasto · 2020-05-07T15:19:43Z

Light code review ACK 6bc6868, this commit appears to be an implementation of the solution in https://isocpp.org/wiki/faq/ctors#static-init-order-on-first-use-members

To be exact: https://isocpp.org/wiki/faq/ctors#static-init-order-on-first-use

jonatack · 2020-05-07T15:23:26Z

To be exact: https://isocpp.org/wiki/faq/ctors#static-init-order-on-first-use

Yes.

maflcko · 2020-05-19T01:00:05Z

As we are not going to destroy LockData instance, this requirement is not relevant

Why not? Assuming ~LockData had side effects, then not calling the destructor is going to miss those side effects.

DrahtBot · 2020-05-19T01:27:00Z

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Conflicts

Reviewers, this pull request conflicts with the following ones:

Replace -Wthread-safety-analysis with broader -Wthread-safety #18635 (Replace -Wthread-safety-analysis with broader -Wthread-safety by hebasto)

If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

maflcko · 2020-05-19T12:25:57Z

And if this was somehow "incorrect" then LogInstance should also be adjusted to be consistent

hebasto · 2020-05-20T05:35:46Z

I'll try to elaborate my opinion :)

bitcoin/src/sync.cpp

Line 98 in 068264f

    
           // This is only safe to do because the *lock_data object and its subobjects happen to have trivial destructors.

@MarcoFalke

What do you mean with "technically incorrect"? https://en.cppreference.com/w/cpp/language/destructor#Trivial_destructor

I mean that among LockData type and its subtypes only using LockPair = std::pair<void*, void*>; and std::mutex have trivial desctructors. This could be easily verified with the std::is_trivially_destructible type trait.

An implicitly-defined destructor, i.e., generated by the compiler, is not the same as a trivial one.

Agree that if we are going do not destruct a heap-allocated object until the program termination, it is pointless to provide a user-defined destructor with any side effects. If this statement is required to be added to the code as a comment, that comment should refer to "not having an explicit desctructor" or "having a implicit desctructor", but not to "a trivial desctructor".

And if this was somehow "incorrect" then LogInstance should also be adjusted to be consistent

The only difference is that the function-local static g_logger is a raw pointer that, in theory, should be deleted.

In this PR the function-local static lock_data is a reference, and this approach explicitly shows our intention do not delete the heap-allocated object at all.

If the above arguments are convincing for our community, I agree that making g_logger a reference is good for consistency.

maflcko · 2020-05-22T11:38:25Z

If this statement is required to be added to the code as a comment, that comment should refer to "not having an explicit desctructor" or "having a implicit desctructor", but not to "a trivial desctructor".

The statement has been added to the logger, so I don't see why it shouldn't be added here.

reference, and this approach explicitly shows our intention do not delete the heap-allocated object at all.

I wasn't aware that this style is used to indicate that delete will not be applied, but fine with me. Functionally * and & are identical.

#include <iostream>

struct A {
  A() { std::cout << __func__ << std::endl; }
  ~A() { std::cout << __func__ << std::endl; }
};

int main() {
  std::cout << "start " << __func__ << std::endl;
  static A a;
  static A *b = new A();
  static A &c = *new A();
  std::cout << "end   " << __func__ << std::endl;
}

start main
A
A
A
end   main
~A

maflcko · 2020-05-22T11:50:37Z

re-ACK 26c093a only change since last review is removing undefined behaviour 🚗

Show signature and timestamp

Signature:

Timestamp of file with hash 27744a6864db90d25a36be70b128da2a91330c24db6afd49a7dd4c0d2c53111c -

hebasto · 2020-05-22T12:03:55Z

@MarcoFalke

If this statement is required to be added to the code as a comment, that comment should refer to "not having an explicit desctructor" or "having a implicit desctructor", but not to "a trivial desctructor".

The statement has been added to the logger, so I don't see why it shouldn't be added here.

bitcoin/src/logging.cpp

Lines 25 to 26 in b5c423c

    
            * Since the destructor is never called, the logger and all its members must 
        
            * have a trivial destructor.

That is not the case in so far as the Logger class has no trivial destructor. One could check this with a patch:

--- a/src/logging.cpp
+++ b/src/logging.cpp
@@ -8,6 +8,8 @@
 #include <util/time.h>
 
 #include <mutex>
+#include <type_traits>
+static_assert(std::is_trivially_destructible<BCLog::Logger>::value, "Logger has no trivial ctor.");
 
 const char * const DEFAULT_DEBUGLOGFILE = "debug.log";

and compile with an error:

logging.cpp:12:1: error: static assertion failed: Logger has no trivial ctor.

maflcko · 2020-05-22T12:09:16Z

src/sync.cpp

-    static LockData lockdata;
-    return lockdata;
+    // This approach guarantees that the object is not destroyed until after its last use.
+    // The operating system automatically reclaims all the memory in a program's heap when that program exits.


Suggested change

// The operating system automatically reclaims all the memory in a program's heap when that program exits.

// The operating system automatically reclaims all the memory in a program's heap when that program exits.

// Since the destructor is never called, the object and all its members must have an implicit destructor.

hebasto · 2020-05-22T12:49:27Z

Updated 26c093a -> 90eb027 (pr18881.14 -> pr18881.15, diff):

added commit that implements @MarcoFalke's suggestion

maflcko · 2020-05-22T13:41:41Z

re-ACK 90eb027, only change is new doc commit 👠

Show signature and timestamp

Signature:

Timestamp of file with hash 5124182a0b09070ce2424a477f0c8ebc1bc2c756ae24de2a66e6948ffd673c33 -

maflcko · 2020-05-22T13:42:36Z

Would be nice to get this merged soon, so that I don't have to pull in this branch every time for fuzzing.

ryanofsky

Concept ACK +0.5. The DeleteLock fix makes sense and I really like the code cleanups, but I don't understand the thread_local change.

Code review ACK 90eb027 because all the changes look correct and safe. But I don't know the purpose of commit 26c093a "Replace thread_local g_lockstack with a mutex-protected map (5/6)." It seems like it could have a bad impact on debug performance, and the commit message and PR description don't give a reason for the change.

re: PR Description #18881 (comment)

This PR:

fixes #18824

~~adds Construct On First Use idiom documentation~~

Related to #15233.

Would suggest updating the PR description to:

Summarize what the UB behavior is so it isn't necessary to read a different github issue
Give a rationale for replacing the thread_local in the last commit
Drop the construct idiom text if it's no longer relevant
Link to #18824 and #15233 at the bottom, and clarify if only the first commit is needed to fix bug #18824, or if the thread_local commit is needed too

maflcko · 2020-05-22T16:51:39Z

Several cases of UB are fixed here.

The first commit fixes the one described in Possible UB in DeleteLock() function #18824
The 26c093a commit fixes one described in Prevent UB in DeleteLock() function #18881 (review)

ryanofsky · 2020-05-22T17:05:14Z

Several cases of UB are fixed here.

The first commit fixes the one described in Possible UB in DeleteLock() function #18824

The 26c093a commit fixes one described in #18881 (review)

Thanks, I definitely think the PR description needs to be improved here and summarize what it is changing and give a clue about how the thread_local change fixes the second problem. If we don't know why the thread local change fixes it, the PR description should say that and not try to be mysterious

hebasto · 2020-05-22T17:05:23Z

@ryanofsky

Would suggest updating the PR description to:

Summarize what the UB behavior is so it isn't necessary to read a different github issue

Give a rationale for replacing the thread_local in the last commit

Drop the construct idiom text if it's no longer relevant

Link to Possible UB in DeleteLock() function #18824 and Prevent mutex lock fail even if --enable-debug #15233 at the bottom, and clarify if only the first commit is needed to fix bug Possible UB in DeleteLock() function #18824, or if the thread_local commit is needed too

Thank you for review. The PR description has been updated.
Sometimes it seems the C++ is much easier to express ideas than English :)

ryanofsky · 2020-05-22T17:16:00Z

re-ACK. Thanks for updating the PR description, this looks good to me. I'm pretty sure there should be a more efficient way to stop referencing the thread_local after it is destroyed (perhaps just resetting the reference in the thread_local destructor) without replacing the thread_local with a map with frequently added & removed entries. But this only affects debugging and probably isn't a big deal.

maflcko · 2020-05-22T18:04:52Z

I am planning to merge this next week unless there are objections or outstanding action items

ryanofsky · 2020-05-22T18:40:53Z

I am planning to merge this next week unless there are objections or outstanding action items

👍 and seems fine to merge earlier, too. None of this code is even compiled unless debugging is turned on.

90eb027 doc: Add and fix comments about never destroyed objects (Hennadii Stepanov) 26c093a Replace thread_local g_lockstack with a mutex-protected map (Hennadii Stepanov) 58e6881 refactor: Refactor duplicated code into LockHeld() (Hennadii Stepanov) f511f61 refactor: Add LockPair type alias (Hennadii Stepanov) 8d8921a refactor: Add LockStackItem type alias (Hennadii Stepanov) 458992b Prevent UB in DeleteLock() function (Hennadii Stepanov) Pull request description: Tracking our instrumented mutexes (`Mutex` and `RecursiveMutex` types) requires that all involved objects should not be destroyed until after their last use. On master (ec79b5f) we have two problems related to the object destroying order: - the function-local `static` `lockdata` object that is destroyed at [program exit](https://en.cppreference.com/w/cpp/utility/program/exit) - the `thread_local` `g_lockstack` that is destroyed at [thread exit](https://en.cppreference.com/w/cpp/language/destructor) Both cases could cause UB at program exit in so far as mutexes are used in other static object destructors. Fix bitcoin#18824 ACKs for top commit: MarcoFalke: re-ACK 90eb027, only change is new doc commit 👠 ryanofsky: Code review ACK 90eb027 because all the changes look correct and safe. But I don't know the purpose of commit 26c093a "Replace thread_local g_lockstack with a mutex-protected map (5/6)." It seems like it could have a bad impact on debug performance, and the commit message and PR description don't give a reason for the change. Tree-SHA512: 99f29157fd1278994e3f6eebccedfd9dae540450f5f8b980518345a89d56b635f943a85b20864cef087027fd0fcdb4880b659ef59bfe5626d110452ae22031c6

Summary: * refactor: Add LockStackItem type alias * refactor: Add LockPair type alias * refactor: Refactor duplicated code into LockHeld() * Replace thread_local g_lockstack with a mutex-protected map This change prevents UB in case of early g_lockstack destroying. Co-authored-by: Wladimir J. van der Laan <laanwj@protonmail.com> * doc: Add and fix comments about never destroyed objects This is a backport of Core [[bitcoin/bitcoin#18881 | PR18881]] Test Plan: ninja all check-all Also in debug mode. Reviewers: #bitcoin_abc, majcosta Reviewed By: #bitcoin_abc, majcosta Subscribers: majcosta Differential Revision: https://reviews.bitcoinabc.org/D7525

DrahtBot added the Docs label May 5, 2020

maflcko approved these changes May 5, 2020

View reviewed changes

src/sync.cpp Outdated Show resolved Hide resolved

maflcko removed the Docs label May 5, 2020

hebasto force-pushed the 200505-cofu branch from 5a406f7 to 7e3f40d Compare May 5, 2020 10:49

jonatack reviewed May 5, 2020

View reviewed changes

doc/developer-notes.md Outdated Show resolved Hide resolved

hebasto force-pushed the 200505-cofu branch from 7e3f40d to a223407 Compare May 5, 2020 11:47

jonatack reviewed May 5, 2020

View reviewed changes

hebasto force-pushed the 200505-cofu branch from a223407 to d3c2686 Compare May 5, 2020 12:12

DrahtBot added the Docs label May 5, 2020

jonatack reviewed May 5, 2020

View reviewed changes

hebasto force-pushed the 200505-cofu branch from d3c2686 to 86ce03c Compare May 5, 2020 13:24

jonatack reviewed May 5, 2020

View reviewed changes

sipa reviewed May 5, 2020

View reviewed changes

doc/developer-notes.md Outdated Show resolved Hide resolved

promag reviewed May 6, 2020

View reviewed changes

doc/developer-notes.md Outdated Show resolved Hide resolved

laanwj reviewed May 7, 2020

View reviewed changes

hebasto force-pushed the 200505-cofu branch from 86ce03c to 6bc6868 Compare May 7, 2020 14:37

jonatack reviewed May 7, 2020

View reviewed changes

maflcko added Refactoring and removed Docs labels May 12, 2020

DrahtBot mentioned this pull request May 19, 2020

Replace -Wthread-safety-analysis with broader -Wthread-safety #18635

Merged

maflcko reviewed May 22, 2020

View reviewed changes

doc: Add and fix comments about never destroyed objects

90eb027

ryanofsky approved these changes May 22, 2020

View reviewed changes

maflcko merged commit fe1357a into bitcoin:master May 26, 2020

hebasto deleted the 200505-cofu branch May 26, 2020 12:52

hebasto mentioned this pull request May 26, 2020

Add local thread pool to CCheckQueue #18710

Merged

PastaPastaPasta pushed a commit to PastaPastaPasta/dash that referenced this pull request Oct 12, 2021

merge bitcoin#18881: Prevent UB in DeleteLock() function

08418c5

pravblockc pushed a commit to pravblockc/dash that referenced this pull request Nov 18, 2021

merge bitcoin#18881: Prevent UB in DeleteLock() function

6b43103

bitcoin locked as resolved and limited conversation to collaborators Feb 15, 2022


		Both cases lead to undefined behavior.

		To prevent the static initialization order problem, be sure to Construct On First Use:

	// The operating system automatically reclaims all the memory in a program's heap when that program exits.
	// The operating system automatically reclaims all the memory in a program's heap when that program exits.
	// Since the destructor is never called, the object and all its members must have an implicit destructor.

Prevent UB in DeleteLock() function #18881

Prevent UB in DeleteLock() function #18881

Uh oh!

Conversation

hebasto commented May 5, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

hebasto commented May 5, 2020

Uh oh!

maflcko left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

hebasto commented May 5, 2020

Uh oh!

Uh oh!

hebasto commented May 5, 2020

Uh oh!

jonatack left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

hebasto commented May 5, 2020

Uh oh!

jonatack left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

hebasto commented May 5, 2020

Uh oh!

jonatack left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

laanwj May 7, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

laanwj commented May 7, 2020

Uh oh!

jonatack commented May 7, 2020

Uh oh!

hebasto commented May 7, 2020

Uh oh!

maflcko commented May 7, 2020

Uh oh!

jonatack left a comment

Choose a reason for hiding this comment

Uh oh!

hebasto commented May 7, 2020

Uh oh!

jonatack commented May 7, 2020

Uh oh!

maflcko commented May 19, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

DrahtBot commented May 19, 2020

Conflicts

Uh oh!

maflcko commented May 19, 2020

Uh oh!

hebasto commented May 20, 2020

Uh oh!

maflcko commented May 22, 2020

Uh oh!

maflcko commented May 22, 2020

Uh oh!

hebasto commented May 22, 2020

hebasto commented May 5, 2020 •

edited

Loading

laanwj May 7, 2020 •

edited

Loading

maflcko commented May 19, 2020 •

edited

Loading

maflcko commented May 22, 2020 •

edited

Loading