Skip to content

test: Avoid accessing free'd memory in validation_chainstatemanager_tests #18615

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
maflcko merged 1 commit into from
Apr 15, 2020
Merged

test: Avoid accessing free'd memory in validation_chainstatemanager_tests #18615

maflcko merged 1 commit into from
Apr 15, 2020

Conversation

@maflcko
Copy link
Member

@maflcko maflcko commented Apr 13, 2020

No description provided.

@maflcko maflcko force-pushed the 2004-testNoAccessFreeMem branch from fa6b144 to fa87f1a Compare April 13, 2020 00:15
@fanquake fanquake added the Tests label Apr 13, 2020
@maflcko maflcko force-pushed the 2004-testNoAccessFreeMem branch 3 times, most recently from fa22c66 to fac4a97 Compare April 13, 2020 00:36
@practicalswift
Copy link
Contributor

practicalswift commented Apr 13, 2020

Concept ACK

Did any of the sanitizers find this issue?

@maflcko
Copy link
Member Author

maflcko commented Apr 13, 2020

Yes, this should be hit in sanitizers when additionally the validationinterface debug log category is enabled.

@jamesob
Copy link
Contributor

jamesob commented Apr 14, 2020

ACK fac4a97

Thanks for fixing this.

ryanofsky
ryanofsky reviewed Apr 14, 2020
View reviewed changes
ryanofsky
ryanofsky approved these changes Apr 14, 2020
View reviewed changes
Copy link
Contributor

@ryanofsky ryanofsky left a comment

Choose a reason for hiding this comment

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

Code review ACK fac4a9757b3bfd131a78570866c349b590f492ca, but see question below. I think this may not be the ideal fix

@maflcko maflcko force-pushed the 2004-testNoAccessFreeMem branch from fac4a97 to fa4cb83 Compare April 15, 2020 15:05
ryanofsky
ryanofsky approved these changes Apr 15, 2020
View reviewed changes
Copy link
Contributor

@ryanofsky ryanofsky left a comment

Choose a reason for hiding this comment

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

Code review ACK fa4cb833037a8a59dc511a9773aa5d64a416066e

@maflcko maflcko force-pushed the 2004-testNoAccessFreeMem branch from fa4cb83 to fa176e2 Compare April 15, 2020 15:46
@jamesob jamesob mentioned this pull request Apr 15, 2020
Merged
ryanofsky
ryanofsky approved these changes Apr 15, 2020
View reviewed changes
Copy link
Contributor

@ryanofsky ryanofsky left a comment

Choose a reason for hiding this comment

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

Code review ACK fa176e2, though if you have to update this again, would suggest separating txindex test cleanup and the chainstatemanager test fix in separate commits, or identifying which part of the change is the bugfix fix in the commit description. Also to clean up the txindex test it might make sense to call SyncWithValidationInterfaceQueue in the test destructor to prevent nondeterminism in other tests

@maflcko
Copy link
Member Author

maflcko commented Apr 15, 2020

Also to clean up the txindex test it might make sense to call SyncWithValidationInterfaceQueue in the test destructor to prevent nondeterminism in other tests

It needs to happen before the TestingSetup is destructed, because txindex is scoped inside the test case.

@ryanofsky
Copy link
Contributor

ryanofsky commented Apr 15, 2020

It needs to happen before the TestingSetup is destructed, because txindex is scoped inside the test case.

Missed that, thanks.

@maflcko maflcko merged commit 4bd6bc5 into bitcoin:master Apr 15, 2020
@maflcko maflcko mentioned this pull request Apr 15, 2020
Merged
@maflcko maflcko deleted the 2004-testNoAccessFreeMem branch April 15, 2020 19:06
Fabcien pushed a commit to Bitcoin-ABC/bitcoin-abc that referenced this pull request Dec 9, 2020
@Fabcien
test: Avoid accessing free'd memory in validation_chainstatemanager_t…
3d4316d 
…ests

Summary: Backport of core [[bitcoin/bitcoin#18615 | PR18615]].

Test Plan:
With TSAN:
  ninja check

Reviewers: #bitcoin_abc, majcosta

Reviewed By: #bitcoin_abc, majcosta

Differential Revision: https://reviews.bitcoinabc.org/D8639
@bitcoin bitcoin locked as resolved and limited conversation to collaborators Feb 15, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

@ryanofsky ryanofsky ryanofsky approved these changes

Assignees

No one assigned

Labels

Tests

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@maflcko @practicalswift @jamesob @ryanofsky @fanquake