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

Avoid nullptr dereference when constructing Blockchain and tx_memory_pool #8033

Closed

Conversation

DarkWingMcQuack
Copy link

@DarkWingMcQuack DarkWingMcQuack commented Oct 31, 2021

Until now Blockchain and tx_memory_pool is constructed like the following:

std::unique_ptr<Blockchain> core_storage;
tx_memory_pool m_mempool(*core_storage);
core_storage.reset(new Blockchain(m_mempool));

This contains a nullptr dereference.

This PR fixes this by introducing a new struct BlockchainAndPool

struct BlockchainAndPool {
  cryptonote::Blockchain blockchain;
  cryptonote::tx_memory_pool tx_pool;

  BlockchainAndPool() : blockchain(tx_pool), tx_pool(blockchain) {}
};

which is used to construct Blockchain and tx_memory_pool to avoid the nullptr dereferencation.

@DarkWingMcQuack DarkWingMcQuack changed the title Avoid nullptr dereference when constructing Blockchainand tx_memory_pool Avoid nullptr dereference when constructing Blockchain and tx_memory_pool Oct 31, 2021
Comment on lines +39 to +44
struct BlockchainAndPool {
cryptonote::Blockchain blockchain;
cryptonote::tx_memory_pool tx_pool;

BlockchainAndPool() : blockchain(tx_pool), tx_pool(blockchain) {}
};
Copy link
Contributor

@UkoeHB UkoeHB Oct 31, 2021

Choose a reason for hiding this comment

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

It seems strange to initialize an object with an uninitialized other object. Would this pattern be more intuitive?

struct BlockchainAndPool {
  cryptonote::Blockchain blockchain;
  cryptonote::tx_memory_pool tx_pool;

  BlockchainAndPool() : blockchain(), tx_pool(blockchain) {
    blockchain.set_tx_pool(tx_pool);
  }
};

Copy link
Author

Choose a reason for hiding this comment

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

I was following how cryptonode::core does it here:
https://github.com/monero-project/monero/blob/528cd729687fcc73e2c082225af203d5f67b3eb6/src/cryptonote_core/cryptonote_core.cpp#L228

@moneromooo-monero has proposed this pattern on IRC/Matrix

Maybe a problem with using a setter would be that if the setter is not called the object would be in an invalid state.
Another reason is that Blockchain and tx_memory_pool hold a reference to each other and those references need to be initialized in the constructor.

Copy link
Contributor

Choose a reason for hiding this comment

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

I see. The original design is dubious but I guess yours is the best solution without more intrusive changes.

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.

Please undo the unrelated indentation changes :)

@selsta
Copy link
Collaborator

selsta commented Nov 3, 2021

Please squash.

@DarkWingMcQuack
Copy link
Author

done

@UkoeHB
Copy link
Contributor

UkoeHB commented Dec 8, 2021

This PR does not fix all cases of the problematic pattern.

  • tests/core_tests/chaingen.cpp init_blockchain() (nice dangling reference there...)
  • tests/block_weight/block_weight.cpp PREFIX_WINDOW()
  • tests/unit_tests/output_distribution.cpp get_output_distribution()

Copy link
Contributor

@mj-xmr mj-xmr left a comment

Choose a reason for hiding this comment

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

Thanks for handling this. It was bothersome.

@mj-xmr
Copy link
Contributor

mj-xmr commented Jan 18, 2022

As @UkoeHB noted, in case the PR is supposed to be merged as it is now, let's not forget to address later:

  • tests/core_tests/chaingen.cpp init_blockchain()
  • tests/block_weight/block_weight.cpp PREFIX_WINDOW()
  • tests/unit_tests/output_distribution.cpp get_output_distribution()

@selsta
Copy link
Collaborator

selsta commented Jul 1, 2023

Continuing in #8924

@selsta selsta closed this Jul 1, 2023
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.

5 participants