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.

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

Fix use of rtxn without a mdb_txn_safe wrapper #8379

Merged
merged 1 commit into from
Aug 23, 2022

Conversation

hyc
Copy link
Collaborator

@hyc hyc commented Jun 7, 2022

Fix use of rtxn without a mdb_txn_safe wrapper

@hyc
Copy link
Collaborator Author

hyc commented Jun 7, 2022

this is not an ideal fix, just a quick'n'dirty to make sure we're on the right track.

the problem with this patch is that it means TXN_PREFIX_RDONLY uses mdb_txn_safe redundantly.

@hyc hyc force-pushed the rtxnguard branch 2 times, most recently from 211a0e7 to 897ce7d Compare June 7, 2022 19:56
Copy link
Collaborator

@selsta selsta left a comment

Choose a reason for hiding this comment

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

This seems to cause a deadlock / infinite loop in core tests on CI, but I didn't run core tests locally yet to confirm it.

@hyc
Copy link
Collaborator Author

hyc commented Jun 8, 2022

ok. I see that it's looping, will check it out.

if (readonly)
db->block_rtxn_stop();
else
if (!readonly)
Copy link
Contributor

Choose a reason for hiding this comment

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

Should this be if (!readonly && active) ?

Copy link
Contributor

Choose a reason for hiding this comment

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

This is the only relevant comment, and this code is shippable otherwise.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

It is already only invoked if (active), see line 1886.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I guess, to eliminate all doubt, could move the if (active) check here from there.

#define TXN_PREFIX_RDONLY() \
MDB_txn *m_txn; \
mdb_txn_cursors *m_cursors; \
mdb_txn_safe auto_txn; \
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this still needed if block_rtxn_start uses it too now? This should be why the code worked in most situations - the bulk of the calls were using TXN_PREFIX_RDONLY.

I don't think it hurts to double up, but it looks unnecessary.

Copy link
Contributor

Choose a reason for hiding this comment

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

Oooops, looks like you commented on this above already. I figured it would work, but I think you'd have to go back through and do a major re-haul to "get it correct", whereas this needs push sooner rather than later.

@luigi1111 luigi1111 merged commit 2056ef7 into monero-project:master Aug 23, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants