refactor: call GetBestBlock() before NewIterator() #22266

pull endjkv wants to merge 1 commits into bitcoin:master from endjkv:master changing 1 files +2 −1

endjkv commented at 8:24 AM on June 17, 2021: none

This fixes the potential memory leak caused by NewIterator() when GetBestBlock() throws an exception. (When compiled with clang, the parameters are evaluated from left to right)
maflcko commented at 8:33 AM on June 17, 2021: member

Isn't this solved by std::make_unique already? #22263 (review)

Edit: Oh never mind, it is not.
DrahtBot added the label Refactoring on Jun 17, 2021
DrahtBot added the label UTXO Db and Indexes on Jun 17, 2021
DrahtBot commented at 6:08 PM on June 17, 2021: contributor



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



Conflicts

No conflicts as of last run.
DrahtBot cross-referenced this on Jun 17, 2021 from issue refactor: wrap CCoinsViewCursor in unique_ptr by jamesob
Talkless commented at 6:50 PM on June 17, 2021: none

Edit: Oh never mind, it is not.

Right, it there would be no need for this PR if CDBWrapper::NewIterator() would return std::unique_ptr.

in src/txdb.cpp:197 in 6dc9aefea7 outdated

 169 | @@ -170,7 +170,8 @@ bool CBlockTreeDB::ReadLastBlockFile(int &nFile) {
 170 |  
 171 |  CCoinsViewCursor *CCoinsViewDB::Cursor() const
 172 |  {
 173 | -    CCoinsViewDBCursor *i = new CCoinsViewDBCursor(const_cast<CDBWrapper&>(*m_db).NewIterator(), GetBestBlock());
 174 | +    uint256 hashBestChain = GetBestBlock();

Talkless commented at 6:53 PM on June 17, 2021:

nit: could be const

in src/txdb.cpp:174 in 6dc9aefea7 outdated

 169 | @@ -170,7 +170,8 @@ bool CBlockTreeDB::ReadLastBlockFile(int &nFile) {
 170 |  
 171 |  CCoinsViewCursor *CCoinsViewDB::Cursor() const
 172 |  {
 173 | -    CCoinsViewDBCursor *i = new CCoinsViewDBCursor(const_cast<CDBWrapper&>(*m_db).NewIterator(), GetBestBlock());
 174 | +    uint256 hashBestChain = GetBestBlock();
 175 | +    CCoinsViewDBCursor *i = new CCoinsViewDBCursor(const_cast<CDBWrapper&>(*m_db).NewIterator(), hashBestChain);

Talkless commented at 6:56 PM on June 17, 2021:

nit: could be * const i if we are changing that line in any case :) . Pointer itself is never changed in the function.

Talkless commented at 6:56 PM on June 17, 2021: none

Concept ACK
endjkv commented at 1:56 AM on June 23, 2021: none

Thanks for the reply. I will check this PR again after #22263 has been merged. I'm not sure whether CDBWrapper::NewIterator() will be wrapped by a std::unique_ptr or not. If so, there's no need for this PR.
DrahtBot added the label Needs rebase on Jun 23, 2021
refactor: call GetBestBlock() before NewIterator() ae29fffbc7
endjkv force-pushed on Jun 24, 2021
DrahtBot removed the label Needs rebase on Jun 24, 2021
achow101 commented at 6:32 PM on October 12, 2022: member

Closing this as it has not had any activity in a while. If you are interested in continuing work on this, please leave a comment so that it can be reopened.
achow101 closed this on Oct 12, 2022
bitcoin locked this on Oct 12, 2023

Contributors

Labels

Refactoring UTXO Db and Indexes

Linked (view graph)

#22263 refactor: wrap CCoinsViewCursor in unique_ptr #22687 .

refactor: call GetBestBlock() before NewIterator() #22266

Conflicts