Skip to content

qa: Initialize lockstack to prevent null pointer deref #13300

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
Merged
laanwj merged 1 commit into from
May 28, 2018
Merged

qa: Initialize lockstack to prevent null pointer deref #13300

laanwj merged 1 commit into from
May 28, 2018

Conversation

@maflcko
Copy link
Member

@maflcko maflcko commented May 22, 2018

It is currently impossible to call debug methods such as AssertLock(Not)Held on a thread without running into undefined behavior, unless a lock was pushed on the stack in this thread.

Initializing the global lockstack seems to fix both issues.

@maflcko maflcko added the Tests label May 22, 2018
@maflcko
Copy link
Member Author

maflcko commented May 22, 2018

Fixes: #13263

@mruddy
Copy link
Contributor

mruddy commented May 22, 2018

Nice, thanks for this fix. I don't feel qualified to review the code change itself. But, I did verify that the patch appears to fix #13263 for me too. Thanks!

@Empact
Copy link
Contributor

Empact commented May 22, 2018

If it's a static variable initialized at startup, maybe do away with the pointer?

promag
promag reviewed May 22, 2018
View reviewed changes
Copy link
Contributor

@promag promag left a comment

Choose a reason for hiding this comment

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

@Empact that should work but maybe consider this patch as a "fix". Also keeping it as a smart pointer is more flexible IMO.

utACK fa6a9b5.

src/sync.cpp Outdated
Copy link
Contributor

@promag promag May 22, 2018

Choose a reason for hiding this comment

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

I guess MakeUnique/std::make_unique is not that relevant here.

Copy link
Contributor

@promag promag May 22, 2018

Choose a reason for hiding this comment

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

Also, const static?

@maflcko maflcko force-pushed the Mf1805-qaLockDebug branch from fa6a9b5 to fa9da85 Compare May 22, 2018 10:34
@promag
Copy link
Contributor

promag commented May 22, 2018

utACK fa9da85, statically allocated does look better. Not sure about the new variable name - is a static variable also global?

@jamesob
Copy link
Contributor

jamesob commented May 22, 2018

utACK fa9da85

@maflcko
Copy link
Member Author

maflcko commented May 22, 2018

@promag My rule of thumb is "everything is a global unless it is a class member". Couldn't yet find any issues with that rule :p

@Empact
Copy link
Contributor

Empact commented May 23, 2018

utACK fa9da85

@laanwj
Copy link
Member

laanwj commented May 28, 2018

Thread-local is a very special kind of global, but as for variable naming it's fine, it has global scope from the perspective of the compiler.

utACK fa9da85

@laanwj laanwj mentioned this pull request May 28, 2018
Closed
@laanwj laanwj merged commit fa9da85 into bitcoin:master May 28, 2018
laanwj added a commit that referenced this pull request May 28, 2018
@laanwj
Merge #13300: qa: Initialize lockstack to prevent null pointer deref
14a4b49 
fa9da85 qa: Initialize lockstack to prevent null pointer deref (MarcoFalke)

Pull request description:

  It is currently impossible to call debug methods such as `AssertLock(Not)Held` on a thread without running into undefined behavior, unless a lock was pushed on the stack in this thread.

  Initializing the global `lockstack` seems to fix both issues.

Tree-SHA512: 8cb76b22cb31887ddf15742fdc790f01e8f04ed837367d0fd4996535748d124342e8bfde68952b903847b96ad33406c64907a53ebab9646f78d97fa4365c3061
@Bushstar Bushstar mentioned this pull request May 29, 2018
Merged
@maflcko maflcko deleted the Mf1805-qaLockDebug branch May 29, 2018 18:20
maflcko pushed a commit to maflcko/bitcoin-core that referenced this pull request Jul 14, 2018
qa: Initialize lockstack to prevent null pointer deref
d9c5630 
Github-Pull: bitcoin#13300
Rebased-From: fa9da85
@maflcko maflcko added this to the 0.16.2 milestone Jul 14, 2018
HashUnlimited pushed a commit to chaincoin/chaincoin that referenced this pull request Jan 11, 2019
qa: Initialize lockstack to prevent null pointer deref
ecc7f47 
Github-Pull: bitcoin#13300
Rebased-From: fa9da85
@AkioNak AkioNak mentioned this pull request Jan 28, 2019
Merged
PastaPastaPasta pushed a commit to PastaPastaPasta/dash that referenced this pull request Jul 1, 2020
@laanwj @PastaPastaPasta
Merge bitcoin#13300: qa: Initialize lockstack to prevent null pointer…
a1c9327 
… deref

fa9da85 qa: Initialize lockstack to prevent null pointer deref (MarcoFalke)

Pull request description:

  It is currently impossible to call debug methods such as `AssertLock(Not)Held` on a thread without running into undefined behavior, unless a lock was pushed on the stack in this thread.

  Initializing the global `lockstack` seems to fix both issues.

Tree-SHA512: 8cb76b22cb31887ddf15742fdc790f01e8f04ed837367d0fd4996535748d124342e8bfde68952b903847b96ad33406c64907a53ebab9646f78d97fa4365c3061
@str4d str4d mentioned this pull request Apr 12, 2021
Merged
@bitcoin bitcoin locked as resolved and limited conversation to collaborators Sep 8, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

1 more reviewer

@promag promag promag left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

Tests

Projects

None yet

Milestone

0.16.2

Development

Successfully merging this pull request may close these issues.

6 participants

@maflcko @mruddy @Empact @promag @jamesob @laanwj